Disable the ability to use $ as an identifier head harder #5270
Conversation
CodaFi
added
the
swift evolution pending discussion
|
@lattner I defer to you for a bit of code review and whether I hit everything you discussed under my last patch. |
|
@swift-ci please smoke test. |
There was a problem hiding this comment.
Swift 3 is done, and (incorrectly!) accepted $ as an identifier. When in Swift 3 mode, we should continue to (silently) accept $ as an identifier so we don't break existing Swift 3 code bases. The bug fix that rejects $ as an identifier should kick in with Swift version >= 4... we should do that part regardless of the outcome of the swift-evolution discussion on $. If the proposal to make $ an identifier passes, this diagnostic will go away entirely.
| // It's always an error to see a standalone $ in Swift 3. | ||
| if (LangOpts.isSwiftVersion3()) { | ||
| // Offer to replace it with '`$`'. | ||
| diagnose(tokStart, diag::expected_dollar_numeric); |
There was a problem hiding this comment.
I suggest flipping the polarity of this if... more detailed comment below.
| return formToken(tok::unknown, tokStart); | ||
| } else{ | ||
| // Else form an identifier token. | ||
| return formToken(tok::identifier, tokStart); |
There was a problem hiding this comment.
We should always treat it as an identifier, even after complaining, because we should recover as-if the user had accepted the Fix-It and compiled again.
There was a problem hiding this comment.
It looks like if I take that route we backtrack over this diagnostic and double-emit while parsing the unqualified decl name for print(_:). Recovery is bad, I give you that, but the diagnostics that are emitted do give the user quite a bit of context as to where, how, and why the parser isn't happy.
| "expected named member of numeric literal", ()) | ||
| "expected named member of numeric literal", ()) | ||
| NOTE(suggest_backtick_dollar_identifier,none, | ||
| "to use '$' as an identifier use backticks to escape it", ()) |
There was a problem hiding this comment.
I think you should fold the error and note together, and put the Fix-It on the error itself. Perhaps wording of something like:
'$' is not an identifier; use backticks ($) to escape it
| // expected-note@-1 {{to use '$' as an identifier use backticks to escape it}} {{9-10=`$`}} | ||
| // expected-error@-2 {{expression resolves to an unused function}} | ||
| // expected-error@-3 {{expected identifier in class declaration}} | ||
| // expected-error@-4 {{braced block of statements is an unused closure}} |
There was a problem hiding this comment.
The recovery will be a whole lot nicer when we pretend as if it were escaped.
|
Awesome |
CodaFi
force-pushed
the
going-toe-to-toe-on-bird-law
branch
from
7bd5367 to
5047d20
Compare
CodaFi
changed the title
CodaFi
force-pushed
the
going-toe-to-toe-on-bird-law
branch
2 times, most recently
from
e651e89 to
c4a9f92
Compare
|
@swift-ci please smoke test. |
| // Else offer to replace '$' with '`$`'. | ||
| diagnose(tokStart, diag::standalone_dollar_identifier) | ||
| .fixItReplaceChars(getSourceLoc(tokStart), getSourceLoc(CurPtr), "`$`"); | ||
| return formToken(tok::unknown, tokStart); |
There was a problem hiding this comment.
Please still return this as tok::identifier for recovery purposes.
There was a problem hiding this comment.
Even with the double diagnostic?
There was a problem hiding this comment.
I was hoping we could find a way to suppress the double diagnostic. If we return tok::unknown, that code is basically a parsing train-wreck, and we get no more useful diagnostics out of it. If we return tok::identifier, everything else works fine except that there are a few Fix-Its to apply. The latter is a much better experience, particularly in Xcode where one can "Fix All In Scope".
I think the lexer has a general problem with diagnostics and backtracking. It wouldn't be totally unreasonable to hold on to a set of locations at which diagnostics were emitting, and check that right before emitting the diagnostics.
|
I'm a little concerned about this change, in very edge case. func foo(`$`: Int) {}
foo(`$`: 42)This might lead a warning loop, no? Maybe, we should add |
|
Thanks for bringing that up, I'll patch it and add it to the tests. |
|
As for "double diagnostic", |
|
Yeah, adding a |
CodaFi
force-pushed
the
going-toe-to-toe-on-bird-law
branch
from
c4a9f92 to
5f523f0
Compare
When in Swift 3 Compatibility Mode we now acceptable a standalone '$' as an identifier. In all other cases this is now disallowed and must be surrounded by backticks.
CodaFi
force-pushed
the
going-toe-to-toe-on-bird-law
branch
from
5f523f0 to
6accc59
Compare
|
Done. @swift-ci please smoke test. |
|
@DougGregor How's it look? |
AnthonyLatsis
removed
the
swift evolution pending discussion
Due to the rejection of SE-0144, this patch adds support for forming identifiers out of single dollar characters when in Swift 3 compatibility mode. Otherwise, we now suggest and allow adding backticks around dollar identifiers to make the lexer happy again.