Decide whether to allow private symbol constants

[askeksa-google]

The specification does not permit the string in const Symbol(string) to construct a private symbol. The new constant evaluator enforces this limitation, causing two tests, language_2/mock_writable_final_private_field_test and language_2/vm/reflect_core_vm_test, to fail.

The issue is discussed further in #34904.

We need to decide before enabling the new constant evaluator whether this use case is essential enough to loosen up the compile-time verification of const Symbol (or, alternatively, to allow = at the end of symbol literals). The change to do the former is here: https://dart-review.googlesource.com/c/sdk/+/97927

[dgrove]

@leafpetersen @lrhn can you all talk with @askeksa-google to make a call here?

[lrhn]

I'd prefer allowing = at the end of literal symbols. It should be grammatically simple and non-breaking, since #symbol is not currently a valid left-hand-side.

There is no reasonable way to make the Symbol constructor create libary-private symbols. The symbol constructor can be invoked with new, not just const. It's not clear how this constructor should figure out which library the private symbol should belong to (checking the stack trace is not a stable solution, it's too fragile wrt refactoring and it would mean that you cannot abstractions a use of the Symbol constructor into a helper library).

[leafpetersen]

Do we need to fix this for 2.3? It sounds like the existing Symbol constructor behavior is wrong on the VM, and will go away no matter what.

[askeksa-google]

I agree with @lrhn that allowing = at the end of literal symbols is the proper solution.

We only need this for 2.3 if some important code is depending on creating these symbols. I spoke with @sjindel-google about it, and he only knew of uses in tests.

[leafpetersen]

So as I understand this:

• Any code that relies on this doesn't currently work on non-VM platforms
• The proposed fix wouldn't prevent any breakage
• The proposed fix would provide a migration path for any code that is broken

I think it sounds reasonable to me to aim to ship 2.3 without supporting = at the end of literal symbols, and follow up with the fix later. If the google3 roll breaks because something internal is using it, we will have to reconsider, but I don't think we will have lost much time if we wait for that signal.

I just searched in internal code for uses of Symbol("_ and found no uses outside of dart_lang and Jurassic Dart. Are there other useful patterns for me to search for?

cc @keertip

[vsmenon]

@leafpetersen - is there anything we need to land to unblock 2.3?

[leafpetersen]

The plan is no.

In the hopefully very unlikely case that we get failures in google3 because someone was creating a private symbol via Symbol(...), we will have to reconsider. As discussed above, I grepped for uses and didn't find any, and since any such code wouldn't work on ddc or dart2js, I think it's very unlikely that we will have an issue.

[keertip]

@leafpetersen, found uses in third party packages source_gen and matcher. The one in matcher is likely not used, source_gen is used.

[leafpetersen]

Do you know what the symbol is that it is trying to produce/where it comes from?

[leafpetersen]

Actually, @keertip I'm very confused - neither of those uses are const. @askeksa-google did the non-const behavior change as well, or was it already correct?

@keertip can you send me a log with a test failure?

[keertip]

@leafpetersen , my bad. missed the const, only saw the private symbol. Please ignore my comment.

[dgrove]

Moving this out of 2.3.

[lrhn]

There should not be any difference between the result of const Symbol(s) and new Symbol(s), except that the former is canonicalized. The two symbols should be ==, even if they are not identical.

(Not the issue here, that's whether you can create a private name using Symbol, and that doing so is currently the only way to create a private setter name. So, if any code needs to create a private setter name without using mirrors, we will now remove that ability.).

[leafpetersen]

These two tests will need to be updated to reflect this change:

• language_2/mock_writable_final_private_field_test
• language_2/vm/reflect_core_vm_test

[askeksa-google]

These tests will be marked as skipped by https://dart-review.googlesource.com/c/sdk/+/99169. Unskip when the tests are fixed.

[askeksa-google]

See also dart-lang/language#301

[mraleph]

I am deleting language_2/mock_writable_final_private_field_test because there is currently no way to port it to constant update enabled VM.

[leafpetersen]

Closing this in favor of the language tracking issue here