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

Don't warn twice: unreachability warning should suppress nonsensical-comparison warning #5897

Closed
scabug opened this issue Jun 10, 2012 · 12 comments
Closed
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Jun 10, 2012

$ scala
Welcome to Scala version 2.10.0-20120609-095502-c9602af1d7 (Java HotSpot(TM) 64-Bit Server VM, Java 1.6.0_32).
Type in expressions to have them evaluated.
Type :help for more information.

scala> () match { case () => true; case _ => false }
<console>:8: warning: comparing values of types Unit and Unit using `==' will always yield true
              () match { case () => true; case _ => false }

Couldn't find other examples that trigger the warning.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 10, 2012

Imported From: https://issues.scala-lang.org/browse/SI-5897?orig=1
Reporter: @VladUreche
Affected Versions: 2.10.0

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 10, 2012

@paulp said:
Why is that a useless warning? More importantly, why doesn't it complain about unreachable code, because that's what the warning implies? I guess because it doesn't know there's only one value of type Unit. But if it can spot it for the type with two values, it can do it for the type with one value.

scala> true match { case true => true ; case false => false ; case _ => false }
<console>:8: warning: unreachable code
              true match { case true => true ; case false => false ; case _ => false }
                                                                               ^

(Is "unreachable code" being a warning rather than an error now a short term virtpatmat thing? I think it was better as an error.)

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 10, 2012

@VladUreche said:
Why is the warning useless you ask? Well, tell me if you see any == comparison in my code snippet. It there's one, I'll eat it. I know the pattern matching is rewritten with ==, but that's an implementation detail which shouldn't trigger a warning to the user.

On the other hand, I forgot about the unreachable code warning, which should actually be issued. Thanks for pointing it out!

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 10, 2012

@paulp said:
You don't want to be warned when you write this?

if (() == ()) true else false

You must have some interesting use case in mind. It's not exactly an "implementation detail" that the pattern matcher calls ==, it's the specification of what pattern matching is. I certainly consider it a feature that it warns in this spot, and would consider it a bug if it did not. The unreachable code warning is born of exactly the same calculus: "this can't possibly be what you meant."

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 10, 2012

@VladUreche said:
I want to be warned when I write () == (), but only when I myself write it. Looking at my snipped of code, it looks like the compiler just gave me a fortune cookie about Unit comparison instead of the unreachable warning I deserved. :p

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 11, 2012

@paulp said:
You yourself did write it. You wrote () match { case () => . There is no reason in the world to warn on () == () but not on that, or vice versa. They are the same thing. (You deserved the unreachable warning-should-be-error as well.)

If you are saying the error message should be tailored to the original syntax, then I would say absolutely it should. But it sounds like you're saying not to warn at all, and the logic of that escapes me.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 11, 2012

@VladUreche said:

If you are saying the error message should be tailored to the original syntax, then I would say absolutely it should. But it sounds like you're saying not to warn at all, and the logic of that escapes me.

No, no, no, I didn't make it clear enough, sorry: the warning is useless only for the syntax in question, no doubt about its usefulness in the general sense.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 11, 2012

@adriaanm said:
It's easy to add support for partitioning () into all its glorious subtypes.
I just haven't done that because I couldn't imagine anyone matching on it.

$ git diff
diff --git i/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala w/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala
index 4e8f416b16..5dc7e02781 100644
--- i/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala
+++ w/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala
@@ -2357,6 +2357,9 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL
     // TODO: domain of feasibly enumerable built-in types (enums, char?)
     def enumerateSubtypes(tp: Type): Option[List[Type]] =
       tp.typeSymbol match {
+        case UnitClass =>
+          Some(List(UnitClass.tpe))
scala> () match { case () => true; case _ => false }
<console>:8: warning: unreachable code
              () match { case () => true; case _ => false }
                                                    ^
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 11, 2012

@adriaanm said:
I agree that we shouldn't both warn about the unreachable code and the comparison.
So let's interpret this ticket that way.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 25, 2012

@VladUreche said:
Critical? Really?

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 25, 2012

@adriaanm said:
i'm just organizing my bugs in two categories: those i want to fix before RC1 (the so-called 'critical' ones) and those that can wait
generating tons of spurious warnings is not good because it hides the real warnings
it's also a regression

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 24, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.