Baseline arity checks for jsx sfc tags #36643
Conversation
weswigham
requested review from
sandersn,
DanielRosenwasser and
RyanCavanaugh
|
@typescript-bot cherrypick this into master |
Component commits: 388801e Baseline arity checks for jsx sfc tags
|
Hey @weswigham, I've opened #36644 for you. |
src/compiler/diagnosticMessages.json
Outdated
| @@ -4280,6 +4280,10 @@ | |||
| "category": "Message", | |||
| "code": 6228 | |||
| }, | |||
| "Function-like tag expects more arguments than the JSX factory can provide.": { | |||
There was a problem hiding this comment.
| "Function-like tag expects more arguments than the JSX factory can provide.": { | |
| "'{0}' expects at least '{1}' arguments, but the JSX factory '{2}' provides at most '{3}'.": { |
There was a problem hiding this comment.
at least isn't quite right for the current implementation - at least right now, we're never looking at minimum argument count here. For example, I could write
declare function MyTagWithOptionalNonJSXBits(props: Props, context: any, nonReactArg?: string): JSX.Element;and what's here right now would issue an error on that tag's usage, and I very much question if it's OK to feed that into React... Minimally, we know React will never provide that third argument...
There was a problem hiding this comment.
OK, I've adjusted the logic to match your message, even if I think it allows some questionable tags through.
src/compiler/checker.ts
Outdated
| const paramCount = getParameterCount(paramSig); | ||
| for (const tagSig of tagCallSignatures) { | ||
| const tagParamCount = getParameterCount(tagSig); | ||
| if (tagParamCount <= paramCount) { |
There was a problem hiding this comment.
This arity check is very simple and differs a lot from our other logic. It'll make better errors easier, but are we sure it's okay?
There was a problem hiding this comment.
I think it's fine; it's pretty conservative. Since we're going through like two calls and then checking some overload is maybe OK (rather than doing overload resolution), and not validating any argument types beyond the first (that's what #29818 does), choosing to only issue an error when we're absolutely sure something's wrong seems reasonable.
tests/baselines/reference/jsxIssuesErrorWhenTagExpectsTooManyArguments.errors.txt
Outdated
Show resolved
Hide resolved
|
cc @DanielRosenwasser @sandersn for updated review statuses~ |
|
ping @sandersn @DanielRosenwasser again |
|
(I noticed that this is opened against 3.8 so needs to be retargetted to master or something like that.) |
weswigham
force-pushed
the
jsx-arity-checks
branch
from
3af84d4
to
299d487
Compare
Finish comment PR feedback
weswigham
force-pushed
the
jsx-arity-checks
branch
from
299d487
to
6b84aff
Compare
|
@sandersn I've retargeted this PR to |
Fixes the worst cases of #33104 without going so far as #29818 (which is terrible for performance).
In this change, we lookup the jsx factory function during jsx signature resolution, and check if any overload expects a function-ish thing; if one does, we validate that, for at least one of those overloads, the number of arguments the FC being used as the tag is less than or equal to the number provided by the JSX library.
This will not cause an error in the two-argument case for
react, asreactspecifies FCs can have a second argument of typeanyfor the reactcontext. So the actual example from the original report in #33104 would be completely uncaught (and there's really nothing we can do about it, since the function actually looks like a completely valid FC), however the more egregious examples down-thread of 3+ arguments do get caught.Notably, I have not made this contingent on
strictFunctionTypes- normally our signature arity rules outside ofstrictFunctionTypesmode would allow the assignment (function bivariance, wooo), were we actually assigning the tag type to the argument type, but that does not come into play here, as I am decomposing the types and checking argument counts directly.cc @DanielRosenwasser for error message feedback~