Skip to content

Suppress value discarding warning when ascribed to Unit #7560

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.

Sign up for GitHub

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

Jump to bottom
Closed
dwijnand wants to merge 1 commit into from
Closed

Suppress value discarding warning when ascribed to Unit #7560

dwijnand wants to merge 1 commit into from

Conversation

@dwijnand
Copy link
Member

@dwijnand dwijnand commented Dec 21, 2018

No description provided.

@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Dec 21, 2018
@dwijnand dwijnand mentioned this pull request Dec 21, 2018
Closed
3 tasks
@dwijnand
Copy link
Member Author

dwijnand commented Dec 21, 2018

/cc @som-snytt who'll be interested 😄

@dwijnand dwijnand force-pushed the no-discard-warning-on-typed-unit branch from 3b19fe8 to 38d0b82 Compare December 21, 2018 11:58
@Ichoran
Copy link
Contributor

Ichoran commented Dec 21, 2018

👍 on the strategy. I can't evaluate the implementation, unfortunately.

som-snytt
som-snytt reviewed Dec 21, 2018
View reviewed changes
test/files/neg/t9847.check
t9847.scala:7: warning: discarded non-Unit value
def g = (42: Unit)
^
t9847.scala:7: warning: a pure expression does nothing in statement position
Copy link
Contributor

@som-snytt som-snytt Dec 21, 2018

Choose a reason for hiding this comment

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

Perhaps the pure expr warning is exactly the same use case?

Copy link
Member Author

@dwijnand dwijnand Dec 21, 2018

Choose a reason for hiding this comment

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

I don't understand the (seemingly to me) subtle differences between the two.

Copy link
Contributor

@som-snytt som-snytt Dec 21, 2018

Choose a reason for hiding this comment

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

"statement position" means not at the end of the block, which is "result position." "Value discard" (or "unit insertion") moves an expression into statement position, so it's doubly warned. I think that, for consistency, it should also not warn on "hi" in the example below. It's a lame use case, and the compiler is trying to warn only in limited cases (non-side-effecty). But it's easy to use the same notational cue and not warn.

Copy link
Member Author

@dwijnand dwijnand Dec 22, 2018

Choose a reason for hiding this comment

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

I don't see the "hi" example.

Copy link
Contributor

@som-snytt som-snytt Dec 22, 2018

Choose a reason for hiding this comment

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

I re-commented, which is what RC is for.

@som-snytt
Copy link
Contributor

som-snytt commented Dec 21, 2018 •
edited
Loading

Verifying at https://github.com/scala/scala/compare/2.13.x...som-snytt:review/unit-warn?expand=1:

@@ -233,6 +233,11 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
     // requiring both the ACCESSOR and the SYNTHETIC bits to trigger the exemption
     private def isSyntheticAccessor(sym: Symbol) = sym.isAccessor && (!sym.isLazy || isPastTyper)
 
+    private def explicitlyUnit(tree: Tree): Boolean = (
+        tree.attachments.contains[TypedExpectingUnitAttachment.type] &&
+      { tree.attachments.remove[TypedExpectingUnitAttachment.type] ; true }
+    )
+

and

if (!isThisTypeResult && !explicitlyUnit(tree)) context.warning(tree.pos, "discarded non-Unit value")

and

if (!explicitlyUnit(t) && treeInfo.isPureExprForWarningPurposes(t)) {
  val msg = "a pure expression does nothing in statement position"

means

scala> :pa
// Entering paste mode (ctrl-D to finish)

class C {
def f(): Unit = {
  "hi" : Unit
  "lo"
  42 : Unit
}}

// Exiting paste mode, now interpreting.


         "lo"
         ^
<pastie>:4: warning: a pure expression does nothing in statement position; multiline expressions might require enclosing parentheses
defined class C

I only wonder when is it safe to remove an attachment. I see the discard warning is on "last try", but retrying makes me suspicious of losing information too early.

Also I see REPL's reporter has wrong indentation for pasted code. It can leverage indenting.

@som-snytt
Copy link
Contributor

som-snytt commented Dec 22, 2018

Did I post this right?

def f(): Unit = {
  "hi" : Unit
  "lo"
  42 : Unit
}

The goal is to not warn where it says Unit.

@dwijnand
Copy link
Member Author

dwijnand commented Dec 22, 2018

@som-snytt I agree, but I'm not sure if I'll get a chance to look into that soon (too many things on the go).

som-snytt
som-snytt requested changes Dec 23, 2018
View reviewed changes
Copy link
Contributor

@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

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

I brought the commit over to a superseding PR.

src/compiler/scala/tools/nsc/typechecker/Typers.scala
if (!isThisTypeResult) context.warning(tree.pos, "discarded non-Unit value")
def wasTypedExpectingUnit = {
tree.attachments.contains[TypedExpectingUnitAttachment.type] && {
tree.attachments.remove[TypedExpectingUnitAttachment.type] // cleanup
Copy link
Contributor

@som-snytt som-snytt Dec 23, 2018

Choose a reason for hiding this comment

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

This doesn't actually clean up, because this is not mutation. Normally, tree.getAndRemoveAttachment, but here it doesn't work to remove the attachment, as the tree will be seen again after adaptation and it will be checked again for expr in statement position warning.

@som-snytt
Copy link
Contributor

som-snytt commented Dec 23, 2018

Probably it's not annoying if I close this.

@som-snytt som-snytt closed this Dec 23, 2018
@dwijnand dwijnand deleted the no-discard-warning-on-typed-unit branch February 4, 2019 21:25
@dwijnand dwijnand mentioned this pull request Feb 4, 2019
Closed
@SethTisue SethTisue removed this from the 2.13.0-RC1 milestone Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@som-snytt som-snytt som-snytt requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dwijnand @Ichoran @som-snytt @SethTisue @scala-jenkins