Error when property is specified more than once via a spread #36727
Conversation
Still probably wrong; probably doesn't even compile beacuse I'm just typing on my laptop.
|
@andrewbranch Mind taking a look at this, especially with an eye on the JSX errors? |
Definitely not - having a |
|
I would just not remove the extra JSX error - it's there because it's essentially like you wrote |
|
I couldn't write code that produced the JSX error in my editor. It's not dependent on a particular version of React being installed is it? Maybe I was just using the wrong language service... |
| declare var abq: { a: number, b?: number }; | ||
| var unused1 = { b: 1, ...ab } // error | ||
| var unused2 = { ...ab, ...ab } // ok, overwritten error doesn't apply to spreads | ||
| var unused3 = { b: 1, ...abq } // ok, abq might have b: undefined |
There was a problem hiding this comment.
It would be nice to add
var unused4 = { ...ab, b: 1 };
var unused5 = { ...abq, b: 1 };just so the scope of the new error is clear to someone reading this test (me) (these should not be errors, right?)
There was a problem hiding this comment.
Yep, those are not errors. I added the cases.
| // Error children are specified twice | ||
| return (<InnerButton {...this.props} children="hi"> | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| !!! error TS2710: 'children' are specified twice. The attribute named 'children' will be overwritten. |
There was a problem hiding this comment.
It’s unclear what this test was supposed to be testing. children are actually specified three times: first in the spread, then in the named attribute, then in the JSX children. IMO, it should never be an error to overwrite a spread property with a named declaration (in JSX props or object literals), but it would be quite reasonable to issue an error when a) a named declaration overwrites a named declaration, or b) a spread property overrides a named declaration (which is what this PR is about). And I’m counting JSX children (like the tag contents) as named, even though the name is implicit.
By way of example, here are the tests I think should probably exist at some point:
interface AnchorProps { href: string; children: string }
declare const props: AnchorProps;
const a1 = <a {...props} href="#" />; // ok: analgous to `p = { ...props, href: '#' }`
const a2 = <a {...props} children="Link" />; // ok: same as above
const a3 = <a {...props}>Link</a>; // ok: same as above
const a4 = <a href="#" {...props} />; // not ok (as of this PR): analgous to `p = { href: '#', ...props }`
const a5 = <a children="Link" {...props} />; // not ok: same as above
const a6 = <a children="Link">Link</a>; // I guess not ok, but kinda linty and unrelated to this PR
There was a problem hiding this comment.
const a6 = <a children="Link">Link</a>; // I guess not ok, but kinda linty and unrelated to this PR
is the case the old error was mostly concerned with catching~
There was a problem hiding this comment.
Yeah, I just reverted the JSX changes since it's just going to hold up this PR to try to unify what appears to be an actually-quite-different-and-complex check.
| @@ -2683,7 +2683,7 @@ | |||
| "category": "Error", | |||
| "code": 2733 | |||
| }, | |||
| "It is highly likely that you are missing a semicolon.": { | |||
| "Are you missing a semicolon?": { | |||
There was a problem hiding this comment.
“$20 says you’re missing a semicolon”
|
Because the error reporting distributes across unions, this gives an extraneous error for types like |
Add an error when we can be certain that a property declaration in an object literal will be overwritten by a later spread in the same object literal.
Updated from #27645
Notably, we can't be certain when:
Also, duplicate property declarations in a property declaration without spreads is track
This PR currently has some cleanup and open questions:
getSpreadType.?.syntax applies in one place.Edit: It's not just that strictNullChecks needs to be on -- the JSX errors are gone for some other reason.
Fixes #27355