Fix #18234: Warn of identifiers with $ in their name #18563
Conversation
|
@szymon-rd here's my PR from the spree |
| @@ -36,6 +36,7 @@ class Compiler { | |||
| List(new Parser) :: // Compiler frontend: scanner, parser | |||
| List(new TyperPhase) :: // Compiler frontend: namer, typer | |||
| List(new CheckUnused.PostTyper) :: // Check for unused elements | |||
| List(new CheckDollarInIdentifier) :: // 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.
| class CheckDollarInIdentifier extends MiniPhase: | ||
| import CheckDollarInIdentifier.ShouldNotContainDollarSignError | ||
|
|
||
| override def phaseName: 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.
|
|
||
| override def phaseName: String = CheckUnused.phaseNamePrefix | ||
|
|
||
| override def description: 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
| override def description: String = CheckUnused.description | ||
|
|
||
| override def isRunnable(using Context): 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 @@ | |||
| //> using options -Xfatal-warnings -Wdollar-check | |||
|
|
|||
| val goodVal = 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
|
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. |
Thanks for reviewing. And sorry it took so long to implement the suggestions |
|
@fernandofpd do you remember why the CI was failing? Could you try to re-run it? |
|
The "Dollar Check" sounds like the storefront in the strip mall where you can buy things for a dollar with a loan against your paycheck. |
|
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 |
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