Fix(50751): Undefined entity with immediate and prototype expression causing crash #52646
Conversation
This file contains hidden or 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
For Milestone Bug
jakebailey
previously approved these changes
Feb 7, 2023
There was a problem hiding this comment.
@typescript-bot test this
@typescript-bot test top100
@typescript-bot user test this
@typescript-bot user test tsserver
@typescript-bot test tsserver top100
@typescript-bot run dt
@typescript-bot perf test this
src/compiler/binder.ts
Outdated
| @@ -3340,6 +3341,9 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void { | |||
| } | |||
| }); | |||
| } | |||
| if (isPrototypeProperty && namespaceSymbol && namespaceSymbol.valueDeclaration) { | |||
| if (isAssignmentDeclaration(namespaceSymbol.valueDeclaration)) containerIsClass = false; | |||
There was a problem hiding this comment.
Nit, but:
Suggested change
| if (isAssignmentDeclaration(namespaceSymbol.valueDeclaration)) containerIsClass = false; | |
| if (isAssignmentDeclaration(namespaceSymbol.valueDeclaration)) { | |
| containerIsClass = false; | |
| } |
In general my impression is that we avoid one-liner things like this unless they are just return or break or something
There was a problem hiding this comment.
Also consider just smushing the two ifs together.
|
Well, I guess the bot doesn't read the "main" review comment. @typescript-bot test this |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 01e9982. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 01e9982. 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 (tsserver) on this PR at 01e9982. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at 01e9982. 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 01e9982. 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 01e9982. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 01e9982. You can monitor the build here. |
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
1 similar comment
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@jakebailey Here they are:
CompilerComparison Report - main..52646
System
Hosts
Scenarios
TSServerComparison Report - main..52646
System
Hosts
Scenarios
StartupComparison Report - main..52646
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailsapache/echarts
|
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
| @@ -3340,6 +3341,9 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void { | |||
| } | |||
| }); | |||
| } | |||
| if (isPrototypeProperty && namespaceSymbol && namespaceSymbol.valueDeclaration && isAssignmentDeclaration(namespaceSymbol.valueDeclaration)) { | |||
| containerIsClass = false; | |||
| } | |||
| if (containerIsClass && namespaceSymbol && namespaceSymbol.valueDeclaration) { | |||
There was a problem hiding this comment.
I think you can combine the checks from the above as well and just do an early return in the "bad" case.
There was a problem hiding this comment.
I'm not sure I understand this. Do you mean I should add it as an else if with return namespaceSymbol; ?
There was a problem hiding this comment.
Something like that - you're setting containerIsClass to false, which makes you indirectly skip the next check; maybe that's fine, but you also have to repeat the same checks for namespaceSymbol. So I would write something like
if (namespaceSymbol?.valueDeclaration) {
// We may have needed to make an assumption that the container symbol is a class
// due to an assignment like `Foo.prototype.method = ...`. However,
// if we never found a valueDeclaration that's suited as an old-style class,
// we should bail out here.
if (isPrototypeProperty && isAssignmentDeclaration(namespaceSymbol.valueDeclaration)) {
return namespaceSymbol;
}
if (containerIsClass) {
addDeclarationToSymbol(namespaceSymbol, namespaceSymbol.valueDeclaration, SymbolFlags.Class);
}
}You can even simplify further, but I think this is fine.
There was a problem hiding this comment.
On that note - maybe for the bail-out case it would be better to say
If the
valueDeclarationis not a class or constructor function
instead of
If the
valueDeclarationis an assignment function
That way we can't end up with more accidental cases like this. But it's up to you.
| @@ -3340,6 +3341,9 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void { | |||
| } | |||
| }); | |||
| } | |||
| if (isPrototypeProperty && namespaceSymbol && namespaceSymbol.valueDeclaration && isAssignmentDeclaration(namespaceSymbol.valueDeclaration)) { | |||
There was a problem hiding this comment.
I think this check needs an explanation of how we could've gotten into this state and what this does to fix it - 2-3 sentences should do.
There was a problem hiding this comment.
something like "A prototype property assignment to something that looks class-like shouldn't actually count if the class-likeness came from an assignment declaration. The checker requires an actual class/constructor function to attach the prototype property to."
sandersn
reviewed
Feb 10, 2023
There was a problem hiding this comment.
Suggestion for wording of the comment @DanielRosenwasser requested.
| @@ -3340,6 +3341,9 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void { | |||
| } | |||
| }); | |||
| } | |||
| if (isPrototypeProperty && namespaceSymbol && namespaceSymbol.valueDeclaration && isAssignmentDeclaration(namespaceSymbol.valueDeclaration)) { | |||
There was a problem hiding this comment.
something like "A prototype property assignment to something that looks class-like shouldn't actually count if the class-likeness came from an assignment declaration. The checker requires an actual class/constructor function to attach the prototype property to."
Comment on lines
+3345
to
+3346
| //A prototype property assignment to something that looks class-like shouldn't actually count if the class-likeness came from an assignment declaration. | ||
| //The checker requires an actual class/constructor function to attach the prototype property to. |
There was a problem hiding this comment.
Just saw that you also left a comment - if you want to stick to this comment, try to shorten the line and add a space after each //
Suggested change
| //A prototype property assignment to something that looks class-like shouldn't actually count if the class-likeness came from an assignment declaration. | |
| //The checker requires an actual class/constructor function to attach the prototype property to. | |
| // A prototype property assignment to something that looks class-like | |
| // shouldn't actually count if the class-likeness came from an assignment declaration. | |
| // The checker requires an actual class/constructor function to attach the prototype property to. | |
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 #50751