Fix overriding Java methods, get rid of JavaMethodType #11361
Conversation
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/11361/ to see the changes. Benchmarks is based on merging with master (ef48716) |
| if prefix eq NoPrefix then | ||
| symbol.is(JavaDefined) | ||
| else | ||
| prefix.classSymbol.is(JavaDefined) |
There was a problem hiding this comment.
What if prefix is an AndType or OrType with some classes that are Java defined?
There was a problem hiding this comment.
Good question, I don't think this was ever handled when merging a JavaMethodType and a MethodType.
- For override checks, the prefix is always a ThisType where classSymbol delegates to its underlying which could be an AndType if a self-type is present, in that case we should fallback to ThisType#cls instead
- In other situation, what matter I think is that we respect this law:
That is, whatever signature we choose must not be the signature of another overload, so for an intersection of all Java classes we need to pick the Java signature, otherwise we should be able to pick the Scala signature.
denot.prefix != NoPrefix implies denot is equivalent to denot.prefix.member(denot.name).atSignature(denot.signature)
I'll add some test cases and tweak the implementation as needed.
There was a problem hiding this comment.
Actually we know that law is violated already, hence the existence of SELECTin, so as long as we consistently pick one or the other that might be good enough.
| symbol.is(Opaque) || | ||
| // SingleDenotation#signature relies on the prefix to compute | ||
| // an appropriate signature for methods. | ||
| info.isInstanceOf[MethodOrPoly] && symbol.is(JavaDefined) != pre.classSymbol.is(JavaDefined) |
There was a problem hiding this comment.
Why does the prefix isJavaDefined status override the symbol's status?
There was a problem hiding this comment.
When checking if foo in B overrides foo in A, we look at the symbol of foo in A as seen from prefix B.this and compare their signature, so we need to rely on this prefix to know what signature to use to match the signature of the override at its definition site.
|
Thinking about it more, I think it's better to restrict the use of Java signatures to situation where it's actually needed , I'll do another pass tomorrow. |
smarter
force-pushed
the
drop-javamethod-new-4
branch
from
2f002f8 to
ecedf60
Compare
|
After trying various approaches, I ended up keeping SingleDenotation#signature simple (use Java signatures if the symbol is Java-defined) which means we don't have to worry about the prefix being set, the trade-off is that SingleDenotation#matchesLoosely has to make sure it's not mixing Java and Scala signatures for override checks to work correctly, cf last commit: ecedf60. I think this is the cleanest solution given the various constraints we have here. |
smarter
force-pushed
the
drop-javamethod-new-4
branch
from
ecedf60 to
53fc349
Compare
This makes it possible to compute the Java or Scala signature of a method, regardless of whether it was defined as a JavaMethod or not. Because there's nothing left that is shareable, this required removing SignatureCachingType and duplicating the signature caching logic between MethodOrPoly and NamedType. This commit itself does not change any semantics, but this new method will be useful for the next commit.
Before this commit, SingleDenotation#signature simply delegated to MethodOrPoly#signature which used Java signatures for JavaMethodType and non-Java signatures otherwise. As a first step towards getting rid of JavaMethodType, this commit changes this to make SingleDenotation#signature itself responsible for using Java signatures.
smarter
force-pushed
the
drop-javamethod-new-4
branch
from
53fc349 to
590d587
Compare
This is possible thanks to the previous commit, in all remaining places where we called `isJavaMethod` we can rely on contextual informations to obtain the same information, so we can completely get rid of it which is a nice simplification. Also get rid of ElimRepeated#transformTypeOfTree which turned out to be a no-op: the denotation transformer already transforms method types and there's nothing to do on top of that.
Use Jave erasure to compute the result type part of Java method signatures too. I don't think that two distinct non-bridge Java overloads can only differ in their result type so this shouldn't fix anything, but it's more consistent.
To check which methods are in an overriding relationship, we rely on
denotation matching which checks that the methods have matching
signatures, but because Scala and Java have different erasure rules,
seemingly valid overrides might not be seen as such because they end up
with different signatures. To compensate for this we change
SingleDenotation#matchesLoosely to use the Scala signatures of both
methods when comparing a Scala and a Java method. This works even though
the methods end up with different erased types because Erasure will
insert bridges.
Note that even after this change, we cannot simply override a Java
generic `Array[T]` with a Scala `Array[T]`, because we intentionally
parse these Java types as `Array[T & Object]`, see
`TypeApplications.translateJavaArrayElementType` for details and
tests/{neg,pos}/i1747.scala for examples.
| case tp => | ||
| tp | ||
|
|
||
| def transformTypeOfTree(tree: Tree)(using Context): Tree = |
There was a problem hiding this comment.
Is there a rationale (except tests pass) that we can remove these? I mean, it's great that we can, but I wonder why the overrides were there in the first place.
There was a problem hiding this comment.
I think that the TimeTravellingTreeCopier (which didn't exist back when ElimRepeated was first added) takes care of any necessary type update in tree.
This was already fixed by scala#11361.
This was already fixed by scala#11361.
This was already fixed by scala#11361.
This was already fixed by scala#11361.
No description provided.