Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify ambiguous reference error message #16137

Merged
merged 2 commits into from
Feb 18, 2023

Conversation

som-snytt
Copy link
Contributor

Fixes #12682

Name binding is tricky because of the intersection of binding precedence and scope.

I think this states the matter most succinctly: Lower precedence cannot shadow higher.

Also modernized the syntax.

It's arguably weird for the compiler to refer to itself in the third person:

The compiler can't decide which of the possible choices you are referencing

Less nerdy and more former schoolmarm:

to which of the possible choices you could possibly be referring

@som-snytt som-snytt force-pushed the issue/12682-ambiguous-8622-message branch 3 times, most recently from e5bd505 to bc978cd Compare October 5, 2022 23:36
@som-snytt som-snytt closed this Oct 9, 2022
@som-snytt som-snytt reopened this Nov 5, 2022
@som-snytt som-snytt marked this pull request as ready for review November 5, 2022 21:24
@som-snytt som-snytt requested a review from lrytz November 6, 2022 00:20
@som-snytt som-snytt force-pushed the issue/12682-ambiguous-8622-message branch from bc978cd to 4ed780a Compare February 2, 2023 19:09
@som-snytt som-snytt requested review from dwijnand and removed request for lrytz February 2, 2023 19:41
@som-snytt
Copy link
Contributor Author

@dwijnand please review? Surprisingly difficult rebase over refactor of messages.scala, so if it is not accepted, I will drop it.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

LGTM, sorry you waited this long... :(

I have a question, but I don't mind merging this without that change.

Comment on lines 1348 to 1349
| - Names introduced by imports
| - Named imports take precedence over wildcard imports
Copy link
Member

Choose a reason for hiding this comment

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

Should we change this?

Suggested change
| - Names introduced by imports
| - Named imports take precedence over wildcard imports
| - Names introduced by named imports
| - Names introduced by wildcard imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I was trying to strike a balance of informality.

@som-snytt som-snytt force-pushed the issue/12682-ambiguous-8622-message branch from 4ed780a to 363996c Compare February 18, 2023 20:36
@som-snytt
Copy link
Contributor Author

Added a commit which finds words for Names introduced by import of a specific name. (Where "specific" means "specified".)

"Named import" is too brief to ring with clarity.

There must be a grammatical term for "by import" (similar to "call by name"), as opposed to "by imports" or "by importing".

The follow-up also restores the previous words in the note: the rule of thumb that defs are preferred to imports, and the special mention of the "new" rule, with a clause explaining when it is relevant:

+        | - Definitions in an enclosing scope take precedence over inherited definitions,
+        |   which can result in ambiguities in nested classes.

Thanks, Dale, for the nudge. No one will say, We didn't adopt Scala 3 because of that stupid message, but I must believe that a collective, cumulative effort must make a difference, not only in aggregate but in the life of some Scala learner yet unborn.

@dwijnand dwijnand merged commit b67daed into scala:main Feb 18, 2023
@som-snytt som-snytt deleted the issue/12682-ambiguous-8622-message branch February 18, 2023 22:05
@SethTisue
Copy link
Member

I must believe that a collective, cumulative effort must make a difference, not only in aggregate but in the life of some Scala learner yet unborn.

@Kordyjan Kordyjan added this to the 3.3.1 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ambiguous reference error message does not know about rule change in #8622
4 participants