Skip to content
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

Closed

Conversation

babakks
Copy link
Contributor

@babakks babakks commented Sep 1, 2022

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 than Normal as an indicator of suppressing type-checking errors.

const [a, b = a] = [0];
// or
const [a, b = a + 1] = [0];

One gotcha in this PR was to revert the updated links (i.e., links.type) of a symbol. Because once a circular relationship error was encountered the type of the symbol is inferred to be any and then somewhere in the call stack, the symbol's associated links.type would be assigned to that returned any type. So, in cases other than CheckMode.Normal we'd replace back the links.type old value. (Sorry, that was not up-to-date)

The gotcha is that I had to suppress updating the symbol's links.type (to any type) for other than CheckMode.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 is any and then do the suppression. I didn't find any better approach than this, so let me know if there's one:

function getTypeOfVariableOrParameterOrProperty(symbol: Symbol, checkMode?: CheckMode): Type {
    const links = getSymbolLinks(symbol);
    let result = links.type;
    if (!result) {
        result = getTypeOfVariableOrParameterOrPropertyWorker(symbol, checkMode);
        // For a contextually typed parameter it is possible that a type has already
        // been assigned (in assignTypeToParameterAndFixTypeParameters), and we want
        // to preserve this type.
        if (!links.type && !(checkMode && checkMode !== CheckMode.Normal)) {
            links.type = result;
        }
    }
    return result;
}

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.

const [a, b = (() => 1 + a)()] = [0];

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>
@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Sep 1, 2022
@babakks babakks marked this pull request as ready for review September 1, 2022 12:48
@sandersn sandersn added this to Not started in PR Backlog Sep 12, 2022
@sandersn sandersn moved this from Not started to Waiting on reviewers in PR Backlog Sep 21, 2022
@@ -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)));
Copy link
Member

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.

PR Backlog automation moved this from Waiting on reviewers to Waiting on author Mar 16, 2023
@sandersn
Copy link
Member

sandersn commented Dec 5, 2023

@babakks Do you want to keep working on this? I'd like to close it otherwise to avoid stale PRs.

@babakks
Copy link
Contributor Author

babakks commented Dec 6, 2023

@sandersn I'll try to address @weswigham's comment soon (before the weekend). Is that okay?

@sandersn
Copy link
Member

sandersn commented Dec 6, 2023

Sure, no big rush, it's been months since I looked at this =)

@babakks
Copy link
Contributor Author

babakks commented Dec 17, 2023

@sandersn Since this PR was based on an old commit of the main branch, I re-applied the changes here (with a slight modification to restrict the side effects) on another PR, #56753. Please close this one and review the other when you deem fit.

PR Backlog automation moved this from Waiting on author to Done Dec 19, 2023
@babakks babakks deleted the improve-binding-element-type-inference-2 branch December 19, 2023 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Using variable defined by array destructuring assignment in default value of variables defined afterwards.
5 participants