Skip to content

Regression: Can't override java defined method which have variable with same name #9392

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

Closed
ohze opened this issue Jul 20, 2020 · 2 comments
Closed

Comments

@ohze
Copy link

ohze commented Jul 20, 2020

Minimized code

class C extends java.util.LinkedList[String] {
  override def size(): Int = super.size()
}

Output

Compile error in 0.25.0-RC2 and 0.26.0-bin-20200719-21a90aa-NIGHTLY:

[error] -- Error: /Users/thanhbv/tmp/ex/src/main/scala/C.scala:2:15 --------------------
[error] 2 |  override def size(): Int = super.size()
[error]   |               ^
[error]   |error overriding variable size in class LinkedList of type Int;
[error]   |  method size of type (): Int cannot override final member variable size in class LinkedList

Expectation

Compile successfully as in dotty 0.24.0 & in scala 2

@ohze ohze added the itype:bug label Jul 20, 2020
@ohze
Copy link
Author

ohze commented Jul 20, 2020

Note: Here is java.util.LinkedList

public class LinkedList<E> ...
  transient int size = 0;
  public int size() { return size; }
  ...

@ohze
Copy link
Author

ohze commented Jul 20, 2020

This issue is caused by commit 945b31e0

def isEffectivelyFinal = .. || is(JavaDefinedVal, butNot = Method)

In RefChecks.checkOverride

val opc = new OverridingPairs.Cursor(clazz)
while opc.hasNext do
  checkOverride(opc.overriding, opc.overridden)
  opc.next()

opc contains both pairs (C.size(), LinkedList.size /*variable*/) and (C.size(), LinkedList.size() /*method*/)
=> error at the /*variable*/ pair because in checkOverride:

else if (other.isEffectivelyFinal) // (1.2)
  overrideError(i"cannot override final member ${other.showLocated}")

giabao added a commit to ohze/dotty that referenced this issue Jul 20, 2020
giabao added a commit to ohze/dotty that referenced this issue Jul 20, 2020
giabao added a commit to ohze/dotty that referenced this issue Jul 20, 2020
giabao added a commit to ohze/dotty that referenced this issue Jul 20, 2020
giabao added a commit to ohze/dotty that referenced this issue Jul 20, 2020
giabao added a commit to ohze/dotty that referenced this issue Jul 20, 2020
smarter added a commit to dotty-staging/dotty that referenced this issue Jul 22, 2020
…d is present

In scala#9126, Java fields were marked effectively finals to prevent false
overrides. This poses a problem when a Java field and method of the same
name are present, because a Scala val or def will match both of these
according to Denotation#matches. We could tweak `matches` further to not
consider the Java field to match any Scala definition, but that might
introduce new problems (we couldn't trust `FullMatch` anymore and would
need to check the info of any Java symbol which might cause cycles, we
would also probably need to tweak overloading resolution to avoid
ambiguity when trying to decide whether to select the Java field or the
Scala override of the Java method).

This commit takes a different approach: we allow a val or def to match a
Java field, but we tweak the logic that checks whether the `override`
modifier is correctly used to disallow definitions that _only_ match a
Java field (thus preventing tests/neg/i9115 from compiling), while
letting through definitions that both match a Java field and something
else (thus making tests/pos/i9392 compile).

This is all a bit messy but I can't think of a better solution which
wouldn't be significantly more complicated.
smarter added a commit to smarter/dotty that referenced this issue Aug 1, 2020
…d is present

In scala#9126, Java fields were marked effectively finals to prevent false
overrides. This poses a problem when a Java field and method of the same
name are present, because a Scala val or def will match both of these
according to Denotation#matches. This commit changes `matches` to
prevent Java field from matching any Scala method, but we can't prevent
them from matching Scala definitions with NotAMethod signature without
introducing more complexity, so the following still doesn't compile
unlike Scala 2:

```java
package pkg;

public class J {
    int i = 0;
    public int i() { return 1; }
}
```

```scala
class S2 extends pkg.J {
  override def i: Int = 2 // error
}
```

I have an alternative fix at scala#9412 which doesn't have this issue but is
more hacky.
@odersky odersky closed this as completed in 3bdae81 Aug 3, 2020
odersky added a commit that referenced this issue Aug 3, 2020
Fix #9392: Allow overriding a Java method when a same-named field is present
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants