-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Improve binding element type inference using CheckMode
#50586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve binding element type inference using CheckMode
#50586
Conversation
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@@ -9552,7 +9552,7 @@ namespace ts { | |||
// contextual type or, if the element itself is a binding pattern, with the type implied by that binding | |||
// pattern. | |||
const contextualType = isBindingPattern(element.name) ? getTypeFromBindingPattern(element.name, /*includePatternInType*/ true, /*reportErrors*/ false) : unknownType; | |||
return addOptionality(widenTypeInferredFromInitializer(element, checkDeclarationInitializer(element, CheckMode.Normal, contextualType))); | |||
return addOptionality(widenTypeInferredFromInitializer(element, checkDeclarationInitializer(element, reportErrors ? CheckMode.Normal : CheckMode.SkipContextSensitive, contextualType))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get why this'd be SkipContextSensitive
, since that sets a lot of other stuff to skip work. Contextual
would be the one that basically just means non-cacheable and no errors.
@babakks Do you want to keep working on this? I'd like to close it otherwise to avoid stale PRs. |
@sandersn I'll try to address @weswigham's comment soon (before the weekend). Is that okay? |
Sure, no big rush, it's been months since I looked at this =) |
Fixes #49989
Alternative to PR #50241.
Here, to prevent false circular-relationship on binding elements initialized with their siblings (like the code segment below), we've utilized
CheckMode
values other thanNormal
as an indicator of suppressing type-checking errors.One gotcha in this PR was to revert the updated links (i.e.,(Sorry, that was not up-to-date)links.type
) of a symbol. Because once a circular relationship error was encountered the type of the symbol is inferred to beany
and then somewhere in the call stack, the symbol's associatedlinks.type
would be assigned to that returnedany
type. So, in cases other thanCheckMode.Normal
we'd replace back thelinks.type
old value.The gotcha is that I had to suppress updating the symbol's
links.type
(toany
type) for other thanCheckMode.Normal
cases, because that would block further efforts to find the correct type for the symbol. Now, I'm thinking maybe it's better to also check if the returned type isany
and then do the suppression. I didn't find any better approach than this, so let me know if there's one:Further issues
Although this PR resolves common cases like the one above, but in cases like below false errors still occur. I didn't dig more to fix such issues because handling them would, in my opinion, clutter the type-checker without significant value in return due to the rarity of such usages. Please correct me if I'm wrong.