Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Andarist
Copy link
Contributor

This is slightly different from Strada in two ways:

  1. GetNameOfDeclaration is preferred now
  2. (anonymous) becomes (Anonymous class) - which is just more consistent

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.
Copy link
Contributor Author

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.

Comment on lines 13 to 14
!!! 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'.
Copy link
Contributor Author

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))
Copy link
Contributor Author

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)

@Andarist Andarist requested a review from jakebailey March 29, 2025 19:20
@jakebailey
Copy link
Member

I'm not 100% clear why (Anonymous class) is more consistent. The (Missing) change seems correct because it improves a baseline, but otherwise I'm not sure.

@Andarist
Copy link
Contributor Author

Andarist commented Mar 31, 2025

This isn't particularly consistent: TS playground. I am surprised now that the first error mentions (Missing) - I could swear I've seen (anonymous) there over the weekend :o But even considering that, this isn't really consistent

(Anonymous class) seems to be way more common in Strada. In its test suite, we can find that 700+ times whereas (anonymous) appears only 4 times (and all of those are within a single test case). (Missing) appears 100+ times but none of those refer to a missing class name.

@jakebailey
Copy link
Member

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
@Andarist
Copy link
Contributor Author

Andarist commented Jun 4, 2025

@jakebailey done

@jakebailey
Copy link
Member

Er, well sort of, but it's not reducing diffs, it's still changing the behavior.

@Andarist
Copy link
Contributor Author

Andarist commented Jun 4, 2025

It changes one diff and adds it to the accepted changes. I could port this faithfully (to print (anonymous) instead of (Missing)) but then that isn't exactly consistent with some of the other presented cases (IIRC). So I chose to just unify this here while addressing this baseline diff

Copy link
Member

@jakebailey jakebailey left a 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?

@Andarist
Copy link
Contributor Author

Sure, I can port this to Strada

@Andarist
Copy link
Contributor Author

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

@jakebailey
Copy link
Member

I'd leave the tests here; it's fine for tests to be "duplicated" between the two temporarily. The names will not conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants