SI-9013 SI-1459 Inherit @varargs annotations #5631
Conversation
|
Review by @retronym (for the part he didn't author ;-)) |
| // causing `super.exclude` to veto it from our bridge generation here, unless we explicitly include it (hence `!sym.hasFlag(VBRIDGE) &&`). | ||
| // TODO: could we generate just one bridge method that does both the java-vararg-array <-> scala-repeated-param-seq conversion | ||
| // as well as the casts from the erased types of the overridden generic method (regular bridge method)? | ||
| override def exclude(sym: Symbol) = !sym.isMethod || (!sym.hasFlag(VBRIDGE) && super.exclude(sym)) |
There was a problem hiding this comment.
The overloaded meaning of the ARTIFACT flag (ACC_SYNTHETIC in bytecode, affects overriding pairs) is surprising, I wasn't aware of that. Without knowing the details, it seems that more explicit checks would be better in places where isArtifact impacts type-checking (the doc comment on ARTIFACT says "should be ignored when typechecking" - what ever that means).
Maybe instead of this fix here, we could skip the ARTIFACT flag for VBRIDGEs? But that's probably a bad idea in 2.11.x, as it changes the bytecode (ACC_SYNTHETIC).
There was a problem hiding this comment.
I think artifacts are intended to be skipped while type checking (for performance? because they may have incorrect types? because their types are trivially correct?)
| @@ -811,7 +811,7 @@ abstract class UnCurry extends InfoTransform | |||
| } | |||
|
|
|||
| // check if the method with that name and those arguments already exists in the template | |||
| enteringUncurry(currentClass.info.member(forwSym.name).alternatives.find(s => s != forwSym && s.tpe.matches(forwSym.tpe))) match { | |||
| enteringUncurry(currentClass.info.member(forwSym.name).filter(s => s != forwSym && s.tpe.matches(forwSym.tpe)).alternatives.headOption) match { | |||
There was a problem hiding this comment.
I don't understand how this fixes the bug - why are the two versions not the same?
There was a problem hiding this comment.
You're right -- the tests pass with both variations. I'll revert the code change.
|
I wonder if we need a corresponding change (special case for What behaviour changes if we remove the |
I'd prefer to delay this experiment to 2.12 or 2.13. For 2.11, I've refactored a bit to make the choice a more explicit one. |
Examples in javac: This method is synthesized by javac because of the class Test {
private Object x;
class I
8000
nner {
{ x.toString(); }
}
} static java.lang.Object access$000(Test);
descriptor: (LTest;)Ljava/lang/Object;
flags: ACC_STATIC, ACC_SYNTHETICStatic accessors are only called by the compiler on behalf of the user, so are marked Another example: abstract class Test implements java.util.function.Function<String, String> {
abstract public String apply(String s);
} public abstract java.lang.String apply(java.lang.String);
descriptor: (Ljava/lang/String;)Ljava/lang/String;
flags: ACC_PUBLIC, ACC_ABSTRACT
public java.lang.Object apply(java.lang.Object);
descriptor: (Ljava/lang/Object;)Ljava/lang/Object;
flags: ACC_PUBLIC, ACC_BRIDGE, ACC_SYNTHETICSo javac marks generic bridges are scalac emits this flag for the benefit of Java clients. AFAIK, it has no semantics to the VM itself. When typechecking calls to Scala-defined classes, we use the |
|
Finally, we also have the This does not prevent calls to these members. |
|
Finally (part deux) I remembered SI-9482 that our typechecker actually does not honour |
A Scala method that implements a generic, Java-defined varargs method, needs two bridges: - to convert the collections for the repeated parameters (VBRIDGE) - to bridge the generics gap (BRIDGE) Refchecks emits the varargs "bridges", and erasure takes care of the other gap. Because a VBRIDGE was also an ARTIFACT, it was wrongly considered inert with respect to erasure, because `OverridingPairs` by default excluded artifacts. Removed the artifact flag from those VBRIDGES, so that they qualify for a real bridge. It would also work to include VBRIDGE methods that are artifacts in BridgesCursor.
|
I've pushed a different solution, which simply does not add the ARTIFACT flag, just VBRIDGE |
|
@retronym, @lrytz, does this approach look better to you? |
|
LGTM, I like this approach much more. |
This is needed if an abstract method is annotated this way.
In addition to the abstract varargs forwarder that we already emit,
we have to emit a concrete forwarder in the subclass.