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
Add -Wnonunit-statement
to warn about discarded values in statement position
#9893
Merged
som-snytt
merged 4 commits into
scala:2.13.x
from
som-snytt:topic/warn-nonunit-statement
Jul 29, 2022
+695
−41
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
nonunit-if.scala:58: warning: discarded non-Unit value of type U | ||
if (!isEmpty) f(a) // warn, check is on | ||
^ | ||
nonunit-if.scala:62: warning: discarded non-Unit value of type Boolean | ||
f(a) // warn, check is on | ||
^ | ||
nonunit-if.scala:13: warning: unused value of type scala.concurrent.Future[Int] (add `: Unit` to discard silently) | ||
improved // warn | ||
^ | ||
nonunit-if.scala:20: warning: unused value of type String (add `: Unit` to discard silently) | ||
new E().toString // warn | ||
^ | ||
nonunit-if.scala:26: warning: unused value of type scala.concurrent.Future[Int] (add `: Unit` to discard silently) | ||
Future(42) // warn | ||
^ | ||
nonunit-if.scala:30: warning: unused value of type K (add `: Unit` to discard silently) | ||
copy() // warn | ||
^ | ||
nonunit-if.scala:37: warning: unused value of type List[Int] (add `: Unit` to discard silently) | ||
27 +: xs // warn | ||
^ | ||
nonunit-if.scala:44: warning: a pure expression does nothing in statement position; multiline expressions may require enclosing parentheses | ||
null // warn for purity | ||
^ | ||
nonunit-if.scala:58: warning: unused value of type U (add `: Unit` to discard silently) | ||
if (!isEmpty) f(a) // warn, check is on | ||
^ | ||
nonunit-if.scala:62: warning: unused value of type Boolean (add `: Unit` to discard silently) | ||
f(a) // warn, check is on | ||
^ | ||
nonunit-if.scala:73: warning: unused value of type U (add `: Unit` to discard silently) | ||
if (!fellback) action(z) // warn, check is on | ||
^ | ||
nonunit-if.scala:81: warning: unused value of type Int (add `: Unit` to discard silently) | ||
g // warn, check is on | ||
^ | ||
nonunit-if.scala:79: warning: unused value of type Int (add `: Unit` to discard silently) | ||
g // warn block statement | ||
^ | ||
nonunit-if.scala:86: warning: unused value of type Int (add `: Unit` to discard silently) | ||
g // warn | ||
^ | ||
nonunit-if.scala:84: warning: unused value of type Int (add `: Unit` to discard silently) | ||
g // warn | ||
^ | ||
nonunit-if.scala:96: warning: unused value of type Int (add `: Unit` to discard silently) | ||
if (b) { // warn, at least one branch looks interesting | ||
^ | ||
nonunit-if.scala:116: warning: unused value of type scala.collection.mutable.LinkedHashSet[A] (add `: Unit` to discard silently) | ||
set += a // warn because cannot know whether the `set` was supposed to be consumed or assigned | ||
^ | ||
nonunit-if.scala:146: warning: unused value of type String (add `: Unit` to discard silently) | ||
while (it.hasNext) it.next() // warn | ||
^ | ||
error: No warnings can be incurred under -Werror. | ||
18 warnings | ||
1 error |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part I don't understand... I don't see anything about
var
oraddOne
.IIUC, it catches any method call on a field, no?
How about calls on local values / variables?
Also
addOne
and+=
are defined with return typethis.type
- why do you need to handle them separately?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a value is not stable, in
v().addOne(x)
, I can witness different values inv()
and the result. I think that is the gist of why, even ifaddOne
is side-effecting and returnsthis
, I've lost the value produced byv
; I can't assume I can recover it by callingv
again.I see a test class
Variant
. I'll add more variants to ensure this check is not too wide a net. I think it applies only tothis
-typed methods. But is that even correct? I'm sure I noticed it by testing on this code base. But maybev.addOne(x)
is bad form.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks. what i missed is that we're already narrowed down to a method with a this.type / singleton type here.