Skip to content

Conversation

@som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Dec 7, 2025

Warns on definitions named with embedded dollars, which should must be written in backquotes, if at all.

Fixes #18234

Test from #18563

@sjrd
Copy link
Member

sjrd commented Dec 7, 2025

which should be written in backquotes

They should not be written at all, in fact. ;)

@som-snytt som-snytt force-pushed the issue/18234-dollar-idents branch from 965c5f3 to 3690e96 Compare December 7, 2025 13:32
@som-snytt
Copy link
Contributor Author

som-snytt commented Dec 7, 2025

This proposal aims to accommodate the two views, which are not entirely incompatible, that strict enforcement is required and yet Java doesn't need it so why does Scala?, where the question is posed with a large dosage of skepticism about warnings.

Here, there is no forking compiler option. Definitions with suspect names are disallowed, with a migration warning now and an error later. However, the workaround, to permit the identifier in backquotes, is quite natural and pleasant, as we have all come to savor backquotes in new syntactic contexts. Backquotes are not required for references (usages).

CB c includes the original izumi-reflect example.

A rewrite will be offered in a follow-up. The corner case is pattern variables, where it's not sufficient to simply wrap the identifier in backquotes.

@som-snytt som-snytt force-pushed the issue/18234-dollar-idents branch from 3690e96 to 3fe96a5 Compare December 7, 2025 20:29
@som-snytt som-snytt force-pushed the issue/18234-dollar-idents branch from 3fe96a5 to 9754e00 Compare December 7, 2025 20:30
@som-snytt som-snytt marked this pull request as ready for review December 8, 2025 18:13
@som-snytt som-snytt requested a review from a team as a code owner December 8, 2025 18:13
@Gedochao Gedochao requested a review from sjrd December 10, 2025 10:05
case GivenSyntax extends MigrationVersion(future, future)
case ImplicitParamsWithoutUsing extends MigrationVersion(`3.7`, future)
case Scala2Implicits extends MigrationVersion(future, future)
case IdentifierDollars extends MigrationVersion(`3.8`, `3.9`)
Copy link
Member

Choose a reason for hiding this comment

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

@Gedochao What's the warning policy for the feature freeze?

Should this be

Suggested change
case IdentifierDollars extends MigrationVersion(`3.8`, `3.9`)
case IdentifierDollars extends MigrationVersion(`3.10`, `3.11`)

at this point?

Or can we get away with

Suggested change
case IdentifierDollars extends MigrationVersion(`3.8`, `3.9`)
case IdentifierDollars extends MigrationVersion(`3.8`, `3.10`)

even though the warning would then be introduced in a patch version?

Copy link
Contributor

Choose a reason for hiding this comment

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

my gut says the former (3.10, 3.11), but I probably could be convinced otherwise.
We haven't had a precedent for this, I guess.
let's discuss it on today's Scala Core.

if checkable && name.toSimpleName.contains('$') then
report.errorOrMigrationWarning(IllegalIdentifier(name), start, MigrationVersion.IdentifierDollars)

extension (nameTree: NameTree) def checkName: nameTree.type =
Copy link
Member

Choose a reason for hiding this comment

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

Sometimes this method is called as a function, sometimes as an extension. That's confusing. Given that you sometimes need it just for the side effect, consider standardizing on the function variant:

Suggested change
extension (nameTree: NameTree) def checkName: nameTree.type =
def checkName(nameTree: NameTree): nameTree.type =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will consider it. This was my first opportunity to use an extension "both ways", which I think is a feature of extensions.

@Gedochao Gedochao added the stat:needs decision Some aspects of this issue need a decision from the maintainance team. label Dec 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat:needs decision Some aspects of this issue need a decision from the maintainance team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enforce spec on identifiers containing $s

3 participants