Skip to content

Conversation

eernstg
Copy link
Member

@eernstg eernstg commented Apr 15, 2021

This CL changes symbol literals to allow #void, because we have allowed that for several years.

We have had the test tests/corelib/symbol_reserved_word_test.dart since 2014, but it is badly flawed. In particular, it is a multi-test, but it tests many cases where a compile-time error is expected using //# ...: continued, which means that the test will pass as long as at least one of the desired compile-time errors occur. However, some parts of the test have been executed in a meaningful manner, so we do have a history of behavior from this test. The analyzer has no current failures on this test, so it's a non-breaking change to specify the behavior that it actually tests.

I've changed the test to express the intent stated in comments in that file (https://dart-review.googlesource.com/c/sdk/+/195503). In particular, it is no longer a multi-test, which fixes the logical error.

However, the test claims that #void and const Symbol('void') should not be a compile-time error, and that new Symbol('void') should not be a run-time error. This is the change which is reflected by this PR.

#void.foo is flagged as an error by the analyzer and expected to be an error (so it's not a breaking change to keep considering this to be an error). It does change the reason: #void is accepted, and .foo is a member access on that symbol, so the error is now that Symbol does not have a foo. However, that's not a breaking change.

#foo.void was a syntax error and remains a syntax error.

The remaining part of symbol_reserved_word_test.dart tests that #word is an error whenever word is a reserved word, and that is also true after this PR.

I have no idea why #void was allowed (even though the spec never said so), but I suspect that it could be because that symbol occurs somewhere in the result of an invocation of a dart:mirrors method. In any case, it seems safe and non-breaking to add support for #void and keep all the other reserved words an error.

Finally, this PR cleans up the formatting of the reserved words and moves a couple of lexical rules away from section 'Reserved Words' and into the section where the only reference to those rules occurs.

@eernstg eernstg requested a review from lrhn April 15, 2021 14:15
@lrhn
Copy link
Member

lrhn commented Apr 16, 2021

I have no idea why #void was allowed

That was because dart:mirrors uses symbols to represent the names of non-composite types, and with void becoming a proper type (so it could be used as a type argument), they needed something to represent the name of the reflection of the void type.

@eernstg
Copy link
Member Author

eernstg commented Apr 16, 2021

OK, thanks!

that does not begin with an underscore (`\code{\_}'),
evaluates to an instance of \code{Symbol} representing the identifier \id.
All occurrences of \code{\#\id} evaluate to the same instance
that does not begin with an underscore (`\code{\_}') or \VOID{}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rewrite as "is \VOID{} or an identifier that does not ..." to avoid the ambiguity of whether "or \VOID{}" binds to the "is" or the "not begin with". Also potentially ambiguous whether we're saying that void is an identifier.

Maybe it can be done with commas too, or by adding "either"/"or".

"where \id{} is either an identifier that does not begin with an underscore (`\code{_}'), or is the reserved word \VOID{},"

Comma after \VOID{} at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

The wording was actually rather sloppy. I revised the section as a whole, such that we only specify canonicalization twice (once for the private identifier, once for all other cases). I also moved the static analysis (just one sentence) from the end to the beginning, to bring about the ordering of topics that we've used in all other sections that were substantially revised.

@eernstg eernstg merged commit b687d3e into master Apr 16, 2021
@eernstg eernstg deleted the specify_symbol_literal_apr21 branch April 16, 2021 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants