Skip to content

Warn on unqualified top-level calls to Any or AnyRef methods #17449

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

Merged
merged 2 commits into from
May 11, 2023

Conversation

mbovel
Copy link
Member

@mbovel mbovel commented May 9, 2023

Fixes #17266

@mbovel mbovel requested a review from dwijnand May 9, 2023 17:59
@mbovel
Copy link
Member Author

mbovel commented May 9, 2023

We should probably extend that to wait, notify and maybe other AnyRef methods.

@mbovel
Copy link
Member Author

mbovel commented May 10, 2023

We should probably extend that to wait, notify and maybe other AnyRef methods.

Done ✅

@som-snytt
Copy link
Contributor

As motivation, I just did this on Scala 2!

That was using eq from a value class. It doesn't root import from Predef, but the class had an explicit import.

The unreliable Scala 2 equality warning gave me a heads up, but linting all such selections seems safer. I haven't checked if eq is covered here. This Scala 2 ticket is just about the message:

scala/bug#12785

@mbovel mbovel changed the title Warn when calling synchronized on Predef or on synthetic file objects Warn when calling And or AnyRef mehods on Predef or on synthetic file objects May 10, 2023
@mbovel mbovel changed the title Warn when calling And or AnyRef mehods on Predef or on synthetic file objects Warn when calling And or AnyRef methods on Predef or on synthetic file objects May 10, 2023
@mbovel mbovel changed the title Warn when calling And or AnyRef methods on Predef or on synthetic file objects Warn when calling Any or AnyRef methods on Predef or on synthetic file objects May 10, 2023
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. Merge when you're happy to stop extending.

@mbovel mbovel requested a review from odersky May 11, 2023 12:43
@odersky
Copy link
Contributor

odersky commented May 11, 2023

I'd like us to try a different warning. Predef is really not so special, after all. Consider this example:

object O

@main def foo =
  import O.*
  println(getClass)

This will print the O$, but it arguably should not.

I think the warning should instead be triggered by this condition:

When referring to a method from Any or AnyRef using a single identifier, one must be inside a named class or object (package objects don't count).

Note that if we put the code in a class or object like this:

object O
object C:
  @main def foo =
    import O.*
    println(getClass)

we get a different warning, which is what we want:

6 |    println(getClass)
  |            ^^^^^^^^
  |            Reference to getClass is ambiguous.
  |            It is both defined in object C
  |            and imported subsequently by import O._
  |
  | longer explanation available when compiling with `-explain`

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

By contrast I see much less of an issue if we write this.getClass at the toplevel. Excluding this without excluding use of this in package objects as a whole looks unsystematic.

So either disallow all explicit uses of this in package objects or allow them all.

@mbovel
Copy link
Member Author

mbovel commented May 11, 2023

@odersky I changed to condition to:

if tree.symbol.exists
   && defn.topClasses.contains(tree.symbol.owner)
   && (!ctx.owner.enclosingClass.exists || ctx.owner.enclosingClass.isPackageObject) then
  report.warning(UnqualifiedCallToAnyRefMethod(tree, tree.symbol), tree)

Seems to do what we want. What do you think?

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

@@ -1090,6 +1090,12 @@ object RefChecks {

end checkImplicitNotFoundAnnotation

def checkSynchronizedCall(tree: Tree)(using Context) =
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename that to checkAnyMemberCall or something like that?

@mbovel mbovel changed the title Warn when calling Any or AnyRef methods on Predef or on synthetic file objects Warn on unqualified top-level calls to Any or AnyRef methods May 11, 2023
@mbovel mbovel enabled auto-merge (rebase) May 11, 2023 20:30
@mbovel mbovel merged commit 5c4e597 into scala:main May 11, 2023
@mbovel mbovel deleted the mb/i17266 branch May 11, 2023 21:48
@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.

Warn when calling synchronized on Predef or on synthetic file objects
5 participants