Fix #18234: Warn of identifiers with $ in their name#18563
Uh oh!
There was an error while loading. Please reload this page.
Conversation
fernandofpd commented Sep 17, 2023
@szymon-rd here's my PR from the spree |
| List(newParser) ::// Compiler frontend: scanner, parser | ||
| List(newTyperPhase) ::// Compiler frontend: namer, typer | ||
| List(newCheckUnused.PostTyper) ::// Check for unused elements | ||
| List(newCheckDollarInIdentifier) ::// Warn if identifier contains a dollar sign $ |
There was a problem hiding this comment.
Please try putting it in the same list as CheckUnused.PostTyper. That way it will run in the same megaphase.
| classCheckDollarInIdentifierextendsMiniPhase: | ||
| importCheckDollarInIdentifier.ShouldNotContainDollarSignError | ||
| overridedefphaseName:String=CheckUnused.phaseNamePrefix |
There was a problem hiding this comment.
The phase name should be declared in the companion object of this phase. it could be just checkDollarInIdentifier. And it's prefix in CheckUnused because there are two different check unused in the pipeline, there is just one of checkDollar, so it can be just phaseName val.
| overridedefphaseName:String=CheckUnused.phaseNamePrefix | ||
| overridedefdescription:String=CheckUnused.description |
There was a problem hiding this comment.
Similarly, CheckDollarInIdentifier should have its own description. It can be something really short and simple.
There was a problem hiding this comment.
Sorry, I had created those in the companion object, but left these as it was from copying from CheckUnused. I have fixed this now
| overridedefdescription:String=CheckUnused.description | ||
| overridedefisRunnable(usingContext):Boolean= | ||
| super.isRunnable && ctx.settings.WdollarCheck.value |
There was a problem hiding this comment.
Better to add a check if it's not java source (like in CheckUnused)
| @@ -0,0 +1,38 @@ | |||
| //>usingoptions-Xfatal-warnings-Wdollar-check | |||
| valgoodVal=1 | |||
There was a problem hiding this comment.
Please add pos tests for unnamed givens (e.g. given String = "foo"), lazy vals, implicit params coming from : syntax (e.g. def foo[T: Showable]), partial function definition
There was a problem hiding this comment.
Also some anonymous functions and a companion object. We have to make sure that no synthetic symbols are catched by linting. A good thing is that we may be able to access the sources to confirm if the symbol is present, but we will do it only if necessary (however, I suspect it may be).
There was a problem hiding this comment.
I think I have covered all the cases you mentioned now. It actually helped catch a case for anonymous functions. Which I added a fix for.
There was a problem hiding this comment.
actually, after adding this phase to the same list as CheckUnused.PostTyper made it be triggered on compilation of the project, and in turn revealed a lot of false positives. I'll try and look into them this week
szymon-rd commented Sep 19, 2023
Thanks for creating this PR! I left some feedback in the comments, overall looks good, but we have to make sure that these synthetic symbols won't cause false positives. |
fernandofpd commented Oct 8, 2023
Thanks for reviewing. And sorry it took so long to implement the suggestions |
mbovel commented Jan 14, 2024
@fernandofpd do you remember why the CI was failing? Could you try to re-run it? |
nicolasstucki commented Feb 16, 2024
The warning is not enough to fully fix the issue. We might need something stronger. See https://contributors.scala-lang.org/t/pre-sip-disallow-restricted-compiler-identifiers/6553 |
som-snytt commented Dec 8, 2025
Closing as stale or superseded, but if the other effort is rejected, this lint could be revived. I'm sorry I missed the first miniphase to run with |
5 participants
The Scala spect states:
We should warn the users if the use an identifier with
$s. We should not make it an error as there are some legitimate corner cases where$in the source code is used (such as in the stdlib and for backward binary compact patches).This PR add
Wdollar-checkto enable this warning$Fixes#18234