add pushTypeResolution to getTypeOfAlias #52642
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
the
For Milestone Bug
|
Shouldn't every call to |
|
That being said, all of the other pops appear to have specific error messages, so we probably will need one for this case (if it ends up fixing the bug). |
jakebailey
reviewed
Feb 8, 2023
DanielRosenwasser
requested changes
Feb 8, 2023
|
It is sort of funny that there's a comment in the middle of this function: // It only makes sense to get the type of a value symbol. If the result of resolving
// the alias is not a value, then it has no type. To get the type associated with a
// type symbol, call getDeclaredTypeOfSymbol.
// This check is important because without it, a call to getTypeOfSymbol could end
// up recursively calling getTypeOfAlias, causing a stack overflow.And the specific thing going wrong here is that |
There's also commented in |
sandersn
requested changes
Feb 10, 2023
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @jakebailey, the results of running the DT tests are ready. |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
@jakebailey Here they are:
CompilerComparison Report - main..52642
System
Hosts
Scenarios
TSServerComparison Report - main..52642
System
Hosts
Scenarios
StartupComparison Report - main..52642
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sandersn
requested changes
May 25, 2023
|
@typescript-bot test top200 |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 2e126a6. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the regular perf test suite on this PR at 2e126a6. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 2e126a6. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 2e126a6. You can monitor the build here. Update: The results are in! |
jakebailey
reviewed
Oct 26, 2023
src/compiler/checker.ts
Outdated
Comment on lines
11884
to
11885
| else if ((symbol.flags & SymbolFlags.Alias) && symbol.declarations?.length) { | ||
| error(symbol.declarations[0], Diagnostics.Circular_definition_of_import_alias_0, symbolToString(symbol)); |
There was a problem hiding this comment.
It doesn't seem like there's anything "importy" about Alias itself; can we get here via non-import means such that this message will be incorrect?
There was a problem hiding this comment.
I'm not sure if we can currently get here via non-import means, but it seems like a good check to add, incase more circularity errors are added in the future.
There was a problem hiding this comment.
I guess this is more of a comment on the specificity of the error; e,g, would the more generic _0_is_referenced_directly_or_indirectly_in_its_own_type_annotation suffice?
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sandersn
approved these changes
Oct 26, 2023
|
Hey @jakebailey, the results of running the DT tests are ready. |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
jakebailey
reviewed
Oct 27, 2023
src/compiler/checker.ts
Outdated
Comment on lines
11885
to
11888
| const symbolImport = symbol.declarations?.find(getAnyImportSyntax); | ||
| if (symbolImport) { | ||
| error(symbolImport, Diagnostics.Circular_definition_of_import_alias_0, symbolToString(symbol)); | ||
| } |
There was a problem hiding this comment.
I'm not quite sure getAnyImportSyntax is the right helper; that seems to be something specific to visibility rules.
Reading the other place where Circular_definition_of_import_alias_0 is used (resolveAlias), probabably getDeclarationOfAliasSymbol is a good location to put the symbol.
Suggested change
| const symbolImport = symbol.declarations?.find(getAnyImportSyntax); | |
| if (symbolImport) { | |
| error(symbolImport, Diagnostics.Circular_definition_of_import_alias_0, symbolToString(symbol)); | |
| } | |
| const node = getDeclarationOfAliasSymbol(symbol); | |
| if (node) { | |
| error(node, Diagnostics.Circular_definition_of_import_alias_0, symbolToString(symbol)); | |
| } |
There was a problem hiding this comment.
Are imports always the last declaration? getDeclarationOfAliasSymbol returns the last declaration that is true for isAliasSymbolDeclaration, which from
TypeScript/src/compiler/types.ts
Line 5725 in a25321a
seems like it is similar to only checking for
SymbolFlags.Alias and may not be specific enough to make sure that the declaration is an import?
There was a problem hiding this comment.
No idea; but getDeclarationOfAliasSymbol also only checks & SymbolKind.Symbol and then uses this helper to issue the same error.
jakebailey
approved these changes
Oct 27, 2023
|
@typescript-bot test top200 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #50928