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

confusing suggestion when non-Unit expression is adapted to Unit (expression discarded in statement position) #18408

Closed
scf37 opened this issue Aug 16, 2023 · 4 comments · Fixed by #18723
Labels
area:reporting Error reporting including formatting, implicit suggestions, etc better-errors Issues concerned with improving confusing/unhelpful diagnostic messages itype:enhancement

Comments

@scf37
Copy link

scf37 commented Aug 16, 2023

Compiler version

3.3.0

Minimized code

def fa(f: String ?=> Unit): Unit = f(using "hello")
fa(42) // compiles with warning but does nothing

Output

[warn] -- [E129] Potential Issue Warning: /Test.scala:13:11 
[warn] 13 |        fa(42) // compiles but does nothing
[warn]    |           ^^
[warn]    |A pure expression does nothing in statement position; you may be omitting necessary parentheses
[warn]    |
[warn]    | longer explanation available when compiling with `-explain`

Expectation

Compilation error. I expect this because 42 seems to not match the type String ?=> Unit

@scf37 scf37 added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Aug 16, 2023
@som-snytt
Copy link
Contributor

That is normal, but you want to use -Wvalue-discard -Wnonunit-statement -Wunused:all if you code with Unit. (As of 3.4.0-RC1-bin-SNAPSHOT.)

Possibly -Wunused:params is not implemented, which should enable -Wunused:implicits. I would expect m and n to warn, and also for the context function that disregards its context to warn.

I would expect f { summon[String] } to warn, since summon is not side-effecting, and would spuriously consume the contextual parameter. (It is summoned but discarded, so something should warn.)

def f(g: String ?=> Unit): Unit = g(using "hello")

def k(s: String): Unit = s * 2 // warns

def m(implicit s: String): Unit = println()
def n(s: String): Unit = println()

@main def test() =
  f(println("goodbye"))
  f(println(summon[String])) // expected usage
  f { summon[String] }
  f { summon[String] * 2 } // warns

@nicolasstucki
Copy link
Contributor

nicolasstucki commented Aug 21, 2023

The warning should take into account that this is a statement in the unit expression {42; ()}. If we desugar by hand, the warning becomes clearer:

  |def test = fa({42; ()})
  |               ^^
  |A pure expression does nothing in statement position; you may be omitting necessary parentheses

We have a similar case in

  |def f: Unit = 32
  |              ^^
  |A pure expression does nothing in statement position; you may be omitting necessary parentheses

@scf37
Copy link
Author

scf37 commented Aug 21, 2023

Similar cases:

def fa(f: => Unit): Unit = f
fa(42) // compiles with warning but does nothing

or even

def fa(f: Unit): Unit = f
fa(42) // compiles with warning but does nothing

I'm not sure it can or should be fixed. My 'gut feeling' was String ?=> Unit is a function while in practice it is more like by-name parameter with implicits.

@som-snytt
Copy link
Contributor

@scf37 if by "I'm not sure it can or should be fixed", you mean, it's very convenient that the language allows values to be discarded, then I agree. I think the "fix" is to warn and -Werror.

About Nicolas's comment, there is some ambiguity, since an expression may wind up in "statement position" after some rewrite such as "value discard" conversion. So both warnings are relevant. I just changed Scala 2 to emit the same "category" warning for both cases, to make it easier to silence them or make them errors, as they are really the same concern.

@nicolasstucki nicolasstucki added itype:enhancement area:reporting Error reporting including formatting, implicit suggestions, etc and removed stat:needs triage Every issue needs to have an "area" and "itype" label itype:bug labels Aug 21, 2023
@bishabosha bishabosha added the better-errors Issues concerned with improving confusing/unhelpful diagnostic messages label Oct 10, 2023
@bishabosha bishabosha changed the title Function taking contextual function as argument can take any type without compilation error confusing suggestion when non-Unit expression is adapted to Unit (expression discarded in statement position) Oct 10, 2023
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Oct 20, 2023
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Oct 20, 2023
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Oct 20, 2023
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Oct 20, 2023
WojciechMazur added a commit that referenced this issue Jun 21, 2024
WojciechMazur added a commit that referenced this issue Jun 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:reporting Error reporting including formatting, implicit suggestions, etc better-errors Issues concerned with improving confusing/unhelpful diagnostic messages itype:enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants