Fix #2202: Check bridge clash in super classes. #2285
Conversation
tests/neg/i2202.scala
Outdated
| @@ -0,0 +1,12 @@ | |||
| // nopos-error | |||
There was a problem hiding this comment.
It should be possible to get a position for this error using Symbol#pos, no?
There was a problem hiding this comment.
Yes, it is possible. I will add that to this PR.
nicolasstucki
force-pushed
the
fix-#2202
branch
from
9ea920a to
56eefb0
Compare
I think @DarkDimius should review this since he understands this code more than I do. |
| emittedBridges.find(stat => (stat.name == bridge.name) && stat.tpe.widen =:= bridge.info.widen) | ||
| val clash: Option[Symbol] = oldSymbol.owner.info.memberExcluding(bridge.name, Flags.Bridge).alternatives.find { | ||
| denot => { | ||
| !denot.symbol.owner.is(Flags.Trait) && (denot.symbol.owner ne defn.ObjectClass) && |
There was a problem hiding this comment.
!denot.symbol.owner.is(Flags.Trait)
why is clashing with trait methods fine?
denot.symbol.owner ne defn.ObjectClass
Why?
!denot.symbol.owner.derivesFrom(bridge.owner)
Is denot.symbol.owner ever a non-trivial subclass of bridge.owner ?
If yes, would be nice to add a test.
There was a problem hiding this comment.
All these conditions where added to make the compilation of the standard library work properly. I will have to explore in detail why I had to exclude traits and object. The non trivial subclass of bridge.owner are present in scala.collection. Nevertheless I will add a small test that has non-trivial subclass.
| !denot.symbol.owner.derivesFrom(bridge.owner) && denot.symbol.info.widen =:= bridge.info.widen | ||
| } | ||
| }.map(_.symbol).orElse( | ||
| emittedBridges.find(stat => (stat.name eq bridge.name) && stat.tpe.widen =:= bridge.info.widen) |
There was a problem hiding this comment.
I'd prefer to change eq back to == to keep the diff minimal. This method has a complex history and it's important to keep it clean.
nicolasstucki
force-pushed
the
fix-#2202
branch
2 times, most recently
from
71faea7 to
8f04d16
Compare
|
Out of date, needs a second look. |
|
@DarkDimius this issue was fixed with the new bridge implementation. |
|
@DarkDimius why close it? Now I'm only adding the regression test for #2202. |
No description provided.