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

Unify the way anonymous class symbols are printed in error messages #731

Open
wants to merge 3 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 '(anonymous)' because it has a private identifier.
+!!! 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.

I think it would be good to add this to accepted baselines but I'm not sure how to do it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add this line to submoduleAccepted.txt:

privateNameMethodClassExpression.errors.txt.diff

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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.

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