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

Unable to discard non-unit value using : Unit #21557

Open
WojciechMazur opened this issue Sep 6, 2024 · 17 comments
Open

Unable to discard non-unit value using : Unit #21557

WojciechMazur opened this issue Sep 6, 2024 · 17 comments
Labels
area:linting Linting warnings enabled with -W or -Xlint itype:enhancement Spree Suitable for a future Spree

Comments

@WojciechMazur
Copy link
Contributor

Scala 2 allows to discard non-unit value using : Unit, however this behaviour seems to be missing in Scala 3.

Compiler version

All Scala 3 versions

Minimized code

//> using options -Wvalue-discard -Wnonunit-statement

object Tests {
  class Assertion(assert: => Any){
    def shouldPass(): Assertion = ???
  }
  def Test: Assertion = {
    new Assertion("").shouldPass(): Unit
    new Assertion("another").shouldPass()
  }
}

Output

[warn] ./test.scala:8:5
[warn] discarded non-Unit value of type Tests.Assertion
[warn]     new Assertion("").shouldPass(): Unit
[warn]     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Expectation

Should ignore non-uni statements with explicit :Unit
When : Unit is missing compiler should hint how to silence the warning similarly as it's done in Scala 2

Compiling project (Scala 2.13.14, JVM (17))
[warn] ./test.scala:9:5
[warn] unused value of type Tests.Assertion (add `: Unit` to discard silently)
[warn]     new Assertion("").shouldPass()
[warn]     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@WojciechMazur WojciechMazur added itype:bug itype:enhancement stat:needs triage Every issue needs to have an "area" and "itype" label area:linting Linting warnings enabled with -W or -Xlint labels Sep 6, 2024
@dwijnand
Copy link
Member

dwijnand commented Sep 6, 2024

Scala 2 PR: scala/scala#7563

@dwijnand dwijnand removed itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Sep 6, 2024
@SethTisue
Copy link
Member

SethTisue commented Sep 6, 2024

@som-snytt in 2.13.15, iirc you removed the "(add : Unit to discard silently)" advice that 2.13.14 used to give. if this gets fixed in Scala 3 (I hope it does), should we add that advice back?

@som-snytt
Copy link
Contributor

I consider that "warn allowance" to be a relic of pre-nowarn, pre-Wconf years.

In particular, it bestows extra semantics on ordinary syntax used in a weird way.

@SethTisue
Copy link
Member

SethTisue commented Sep 6, 2024

hmm.

: Unit does have the advantage of being short and only involving ordinary syntax (even if arguably in a “weird way”)

the design discussion at scala/scala#7563 is worth reviewing. Adriaan made some pretty persuasive arguments in favor of : Unit

bare@nowarn is not great because then I'm suppressing all warnings, not just the warning I had in mind

and writing out a more targeted @nowarn expression is the sort of thing that sends pretty much all of us running off to consult reference materials to figure out how to do it. whereas : Unit, I can remember.

so my initial reaction is that #7563 wasn't just a mistake or an artifact of @nowarn not existing yet. but perhaps I should do a fresh round of pondering.

@som-snytt
Copy link
Contributor

I wonder if @discard is a better path. Annotations are a last resort, but are tool-aligned.

Worth adding that we use phrases "ascription" and "annotation" interchangeably, which suggests these are the same use case, but just different syntaxes.

@dwijnand
Copy link
Member

dwijnand commented Sep 6, 2024

I think I recently found out that you can have multiple val _ = .. in scala 3, and they work - which don't work in Scala 2. So that's another fairly handy way to expressly discard a non-unit value.

@som-snytt
Copy link
Contributor

@dwijnand that works in Scala 2. I think in Scala 3 it is ignored as local store, but in Scala 2 introduces a field (result is retained).

class C {
  val _ = 42
  val _ = 26
}

in Scala 2

under.scala:3: warning: Pattern definition introduces Unit-valued member of C; consider wrapping it in `locally { ... }`.
  val _ = 42
      ^
under.scala:4: warning: Pattern definition introduces Unit-valued member of C; consider wrapping it in `locally { ... }`.
  val _ = 26
      ^
2 warnings

@dwijnand
Copy link
Member

dwijnand commented Sep 6, 2024

I remember some "already named _" errors, but who knows. Maybe they're 2.12 or something.

@som-snytt
Copy link
Contributor

Yes, it was "improved". But I sympathize with your cognitive dissonance. You have led so many Scala lives.

@mbovel
Copy link
Member

mbovel commented Sep 23, 2024

If we decide to implement this, it seems like it could be a nice Spree-sized issue.

What is needed to take decision? Should this be further discussed here, or during a compiler or core meeting?

@mbovel mbovel added the Spree Suitable for a future Spree label Sep 23, 2024
@dwijnand
Copy link
Member

It seems small and innocent and has been a part of the Scala 2 language for a while, so unless @odersky come around and strongly argues against this before the next spree, I think we can go ahead and propose a PR for this.

@odersky
Copy link
Contributor

odersky commented Sep 24, 2024

I am not sure what exactly is proposed here.

@mbovel
Copy link
Member

mbovel commented Sep 27, 2024

We discussed this today during the compiler meeting and decided we should implement this. Let's tackle it during the next Spree 🥳

@mbovel
Copy link
Member

mbovel commented Oct 20, 2024

This issue was picked for the Scala Issue Spree of tomorrow, Monday, October 21st. @rochala and @nmcb will be working on it. If you have any insight into the issue or guidance on how to fix it, please leave it here.

@som-snytt
Copy link
Contributor

Sorry I have no insight, but "if it makes Seth happy, it can't be that baaad."

@mbovel
Copy link
Member

mbovel commented Nov 9, 2024

This issue was picked again for the Scala Issue Spree of Monday, November 11th. @mbovel and @nmcb will continue working on it.

@goshacodes
Copy link

This is not only Seth hapiness :)
E.g. we use scalapb which generates code with warnings and we would be happy to disable it on grpc module

Sorry I have no insight, but "if it makes Seth happy, it can't be that baaad."

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:linting Linting warnings enabled with -W or -Xlint itype:enhancement Spree Suitable for a future Spree
Projects
None yet
Development

No branches or pull requests

7 participants