- Notifications
You must be signed in to change notification settings - Fork664
Unify the way anonymous class symbols are printed in error messages#731
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
...a/baselines/reference/submodule/conformance/privateNameMethodClassExpression.errors.txt.diffShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| C.getInstance().#field; // Error | ||
| ~~~~~~ | ||
| !!! error TS18013: Property '#field' is not accessible outside class '(Missing)' because it has a private identifier. | ||
| !!! error TS18013: Property '#field' is not accessible outside class 'C' because it has a private identifier. |
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.
Strada would print(anonymous) here but this has an inferrable~ name from the declaration and I think it's better to use it. Using it is not a new thing to do.
| !!! error TS2322: Type '(Anonymous class)' is not assignable to type 'A'. | ||
| !!! error TS2322: Property '#foo' in type '(Anonymous class)' refers to a different member that cannot be accessed from within type 'A'. |
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.
Strada would print(Anonymous class) in the first line here and then(anonymous) in the second line. So I think this change unifying it is a good thing.
Note this is a new test case, not one from Strada.
| @@ -4272,8 +4271,8 @@ func (r *Relater) reportUnmatchedProperty(source *Type, target *Type, unmatchedP | |||
| privateIdentifierDescription := unmatchedProperty.ValueDeclaration.Name().Text() | |||
| symbolTableKey := binder.GetSymbolNameForPrivateIdentifier(source.symbol, privateIdentifierDescription) | |||
| if r.c.getPropertyOfType(source, symbolTableKey) != nil { | |||
| sourceName := scanner.DeclarationNameToString(ast.GetNameOfDeclaration(source.symbol.ValueDeclaration)) | |||
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.
the previous behavior here was already diverging slightly from Strada for those anonymous classes (IMHO, in a good way)
I'm not 100% clear why |
Andarist commentedMar 31, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This isn't particularly consistent:TS playground. I am surprised now that the first error mentions
|
Old PR, but it would be worth updating this PR I think, at least into a form that reduces baseline diffs. I'd rather that be the outcome. |
…-print# Conflicts:#testdata/baselines/reference/submoduleAccepted/conformance/privateNameMethodClassExpression.errors.txt.diff#testdata/submoduleAccepted.txt
@jakebailey done |
Er, well sort of, but it's not reducing diffs, it's still changing the behavior. |
It changes one diff and adds it to the accepted changes. I could port thisfaithfully (to print |
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.
On second thought, I feel like this is a reasonable improvement, however it would probably make sense to stick this in Strada too, given the improvement should apply there?
Sure, I can port this to Strada |
I created a PR porting this to Strada:microsoft/TypeScript#61943 so I have reverted new tests etc from here given they will come to Corsa automatically once you bump the submodule after that port lands |
I'd leave the tests here; it's fine for tests to be "duplicated" between the two temporarily. The names will not conflict. |
This is slightly different from Strada in two ways:
GetNameOfDeclarationis preferred now(anonymous)becomes(Anonymous class)- which is just more consistent