-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #1665: Check that != has an operand on the left. #1685
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
Conversation
@@ -0,0 +1,5 @@ | |||
|
|||
object Test { | |||
!=(1) // error: Binary operation != should have an operand on the left hand side |
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.
why? this is supported by scalac.
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.
In scalac there is a warning and then it is converted to true
<console>:12: warning: comparing values of types type and Int using `!=' will always yield true
!=(1)
^
res0: Boolean = true
I could:
- Make it a warning and collapse it to
true
if in Scala 2 mode. - Or make it a warning and collapse it to
true
in all cases.
lazy val Select(qual, _) = tree.fun | ||
lazy val qual = tree.fun match { | ||
case Select(qual, _) => qual | ||
case Ident(name) => EmptyTree |
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 should also handle idents. Here's an example illustrating how to do it: https://github.com/lampepfl/dotty/blob/master/src/dotty/tools/backend/jvm/DottyBackendInterface.scala#L428-L438
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.
Ok, I understand the point. The I misinterpreted the scalac error message where it says types type
.
ab93bc8
to
e8228d7
Compare
assert(tree.args.size == 1) | ||
val lhs = qual.tpe | ||
val rhs = tree.args.head.tpe | ||
if (!(lhs <:< rhs || rhs <:< lhs) && rhs != defn.NullType && lhs != defn.NullType) { |
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.
Here if I don't add rhs != defn.NullType && lhs != defn.NullType
I start getting failures when comparing java.lang.Object
some other types including types in scala.collection
.
@DarkDimius I wonder if this is a bug or am I testing it the wrong way?
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.
I'm not sure I understood what failures did you get. Could you please elaborate?
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.
For example if I run partest
without the rhs != defn.NullType && lhs != defn.NullType
I get the following two warnings (with partest-only-no-bootsrtap
I only get the second one):
[info] Test test.TestBCode.nullChecks started
-- Warning:
class Foo {
def foo(x: AnyRef): Int = {
val bool = x == null
if (x != null) 1
else 0
}
}
5 | if (x != null) 1
| ^^^^^^^^^^^
| comparing values of types Object and Null using `!=' will always yield true
[info] Test dotc.tests.repl_all started
input differs from transcript:
scala> val Const,x = 0
Const: Int = 0
x: Int = 0
scala> val (Const, List(`x`, _, a), b) = (0, List(0, 1337, 1), 2)
-- Warning: <console> --------------------------------------------------------------------------------------------------
6 |val (Const, List(`x`, _, a), b) = (0, List(0, 1337, 1), 2)
| ^
| comparing values of types scala.collection.immutable.List[Int] and Null using `!=' will always yield true
a: Int = 1
b: Int = 2
scala> val a@b = 0
a: Int @unchecked = 0
b: Int @unchecked = 0
scala> :quit
Are those non nullable AnyRef
s?
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.
I think this is a bug in IsInstanceOfEvaluator
.
We should have a discussion whether we need IsInstanceOfEvaluator or whether this can be rolled into multiversal equality checks. |
5914caf
to
1fa7698
Compare
/rebuild |
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.
Otherwise LGTM
case Select(qual, _) => qual | ||
case ident @ Ident(_) => | ||
ident.tpe match { | ||
case TermRef(prefix: TermRef, name) => |
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.
name isn't used in either of two cases.
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.
Removed
|
||
object Test extends App { | ||
val mutant = new { val x = 2 } | ||
val c = cm.classSymbol(mutant.getClass) |
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.
why does this test need scala2 reflection?
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.
Just because it is the code that was in issue #1665.
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.
@nicolasstucki - could you update this test to use Java reflection instead? Cheers from Dima
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.
I removed the test as the other test file contains all the tests needed.
12ce065
to
c5f4e0d
Compare
c5f4e0d
to
929efec
Compare
LGTM |
No description provided.