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

Warn when named tuples resemble assignments #21823

Merged
merged 3 commits into from
Oct 22, 2024
Merged

Conversation

mbovel
Copy link
Member

@mbovel mbovel commented Oct 21, 2024

This PR adds a warning for named tuples that look like assignment, such as (x = 1).

This is the first half to implement #21681. The second will be to add warnings for named arguments to infix method calls (as a separate PR?).

Started during the Spree of October 21st.

Closes #21770.

@som-snytt
Copy link
Contributor

My quick test shows an unexpected error

Compilation failed for: tests/warn/infix-named-args.scala
-> following the diagnostics:
 at 5: None of the overloaded alternatives of method + in class Int with types
 (x: Double): Double
 (x: Float): Float
 (x: Long): Long
 (x: Int): Int
 (x: Char): Int
 (x: Short): Int
 (x: Byte): Int
 (x: String): String
match arguments ((x : Int))

for the test in

https://github.com/scala/scala3/pull/21565/files

@mbovel
Copy link
Member Author

mbovel commented Oct 21, 2024

My quick test shows an unexpected error

def f = 42 + (x = 1)
-- [E134] Type Error: playground.scala:1:11 ------------------------------------
1 |def f = 42 + (x = 1)
  |        ^^^^
  |   None of the overloaded alternatives of method + in class Int with types
  |    (x: Double): Double
  |    (x: Float): Float
  |    (x: Long): Long
  |    (x: Int): Int
  |    (x: Char): Int
  |    (x: Short): Int
  |    (x: Byte): Int
  |    (x: String): String
  |   match arguments ((x : Int))

It is expected to have an error in this case, isn't it? I also get in on main.

We should add a warning to tell the user that (x = 1) is interpreted as a tuple in this case as suggested in #21681, but we haven't had the time to implement it yet.

@mbovel
Copy link
Member Author

mbovel commented Oct 21, 2024

Also, we found that our naive use of ctx.scope.lookup it not complete enough. It seems it only looks in the current scope.

It doesn't work for example for:

object Test:
  var age: Int = 28
  (age = 29) // warn

@nicolasstucki suggests that we move our check to Typer and type the identifier there to resolve the symbol properly.

@mbovel mbovel linked an issue Oct 21, 2024 that may be closed by this pull request
@mbovel mbovel force-pushed the mb/21681 branch 2 times, most recently from 44708ec to dd22105 Compare October 21, 2024 19:22
@mbovel
Copy link
Member Author

mbovel commented Oct 21, 2024

@nicolasstucki @bracevac I moved the check to Typer and tried to type the key as an Ident as discussed: ca5f467. However Symbol.setter is still not defined. Maybe it only is once the whole class is type-checked?

I then tried to type the whole thing as Assign, with a temporary context: dd22105. It seems to work locally, let's see what the CI says.

@mbovel mbovel marked this pull request as ready for review October 21, 2024 21:16
@SethTisue
Copy link
Member

I hit this issue in my own code just this morning, during a 3.6.1 upgrade — locating the cause took me a while, so I’m glad to see a warning being added

@SethTisue SethTisue requested a review from noti0na1 October 22, 2024 03:41
@noti0na1
Copy link
Member

Can you add a test for #21770 as well?

def f(g: Int  => Unit) = g(0)

def test = 
  var cache: Option[Int] = None
  f(i => (cache = Some(i)))

Thanks!

@mbovel mbovel changed the title Warn for named tuples that look like assignments Warn when named tuples resemble assignments Oct 22, 2024
@mbovel
Copy link
Member Author

mbovel commented Oct 22, 2024

Can you add a test for #21770 as well?

Done.

mbovel and others added 3 commits October 22, 2024 14:16
Co-Authored-By: Nicolas Stucki <[email protected]>
Co-Authored-By: Oliver Bračevac <[email protected]>
Co-Authored-By: Nicolas Stucki <[email protected]>
Co-Authored-By: Oliver Bračevac <[email protected]>
Copy link
Member

@noti0na1 noti0na1 left a comment

Choose a reason for hiding this comment

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

LGTM

@mbovel mbovel enabled auto-merge October 22, 2024 12:36
@mbovel mbovel merged commit 276d0a3 into scala:main Oct 22, 2024
28 checks passed
@mbovel mbovel deleted the mb/21681 branch October 22, 2024 14:49
@WojciechMazur WojciechMazur added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Nov 18, 2024
@WojciechMazur WojciechMazur added backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Nov 18, 2024
WojciechMazur added a commit that referenced this pull request Nov 18, 2024
Backports #21823 to the 3.6.2.

PR submitted by the release tooling.
[skip ci]
@WojciechMazur WojciechMazur added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Nov 18, 2024
@WojciechMazur WojciechMazur added this to the 3.6.2 milestone Nov 18, 2024
WojciechMazur added a commit that referenced this pull request Nov 28, 2024
Resolves #22042

* Reverts most of the stabilization changes introduced in
#21680 excluding bugfixes introduced
when stabilizing the name tuples
* Adapts #21823 and
#21949 warnings to make them both
syntax deprecations instead of ambiguous syntax.
* Adds automatic rewrite to #21823
to replace `(foo = bar)` into `{foo = bar}`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:done This PR was successfully backported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in typer for closure body treated as named tuple Add migration helpers for named tuples
5 participants