Skip to content

Fix #5433: check that the implemented super-accessor is valid #5604

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

Merged
merged 4 commits into from
Mar 5, 2019

Conversation

smarter
Copy link
Member

@smarter smarter commented Dec 12, 2018

Relying on matchingDenotation is not enough as demonstrated by
i5433.scala: in Fail, B#foo matches C$$super$foo
but it cannot implement it since X is a supertype of Y

Note that scalac seems to have the same bug (but at least in Dotty this
is detected by -Ycheck:all).

For reference, here's what the spec says on resolving super accessors:

A reference super.m refers statically to a method or type m in the
least proper supertype of the innermost template containing the
reference. It evaluates to the member m′ in the actual supertype of that
template which is equal to m or which overrides m.

This seems like it should be clarified to indicate that the member m'
may be found not only in "the actual supertype" (defined elsewhere) but
in any supertype of the actual supertype.

@odersky odersky assigned smarter and unassigned odersky Dec 17, 2018
@odersky
Copy link
Contributor

odersky commented Jan 12, 2019

@smarter I did the change we discussed. Can I go ahead merging this?

@allanrenucci
Copy link
Contributor

Note that it doesn't fix the original compilation order issue. What used to be a Ycheck failure is now a compilation error:

-- Error: dotty/scala2-library/src/library/scala/collection/mutable/IndexedSeqView.scala:46:37 
46 |  private[collection] abstract class AbstractTransformedX[B] extends super.AbstractTransformedS[B] with TransformedX[B]
   |                                     ^
   |                        illegal mixin super call target:
   |                        
   |                          override def tail: => scala.collection.SeqView[B, Coll]  in class AbstractTransformedS
   |                        
   |                        does not conform to expected target type
   |                        
   |                          => Repr

Can be reproduced by sorting the files from the compileStdLib test

@odersky
Copy link
Contributor

odersky commented Jan 12, 2019

@allanrenucci Yes, unfortunately the standard library is in error here. Since it would be a pain to fix, and since the standard library is also going away I would recommend to simply keep the old ordering, which compiles OK. I.e. try to work around the error in the stdlib.

@smarter
Copy link
Member Author

smarter commented Jan 12, 2019

Since it would be a pain to fix

Not really, we just need to replace some instances of super.tail by super[Something].tail.

@smarter I did the change we discussed. Can I go ahead merging this?

The fix is OK but I'd like to improve the error message to give more details (explain that the issue comes from the linearization orders, which mixins are involved, how the issue could be solved by replacing super.foo by super[X].foo, ...). This isn't critical to merge this if you want to go ahead.

@odersky
Copy link
Contributor

odersky commented Jan 12, 2019

No, I guess if you have improvements in the pipeline we can leave this open.

@adriaanm
Copy link
Contributor

This one looks related: explicitly targeting an abstract super method should also yield an error, right?

scala/bug#8693

@smarter smarter force-pushed the fix-super-mixin-2 branch 2 times, most recently from 521590b to de8b794 Compare February 28, 2019 12:23
@smarter
Copy link
Member Author

smarter commented Feb 28, 2019

Alright, I've now made the error message as helpful as I could:

-- Error: tests/neg/i5433.scala:17:6 -------------------------------------------
17 |class Fail extends B with C // error
   |      ^
   |class Fail cannot be defined due to a conflict between its parents when
   |implementing a super-accessor for foo in trait C:
   |
   |1. One of its parent (trait C) contains a call super.foo in its body,
   |   and when a super-call in a trait is written without an explicit parent
   |   listed in brackets, it is implemented by a generated super-accessor in
   |   the class that extends this trait based on the linearization order of
   |   the class.
   |2. Because B comes before C in the linearization
   |   order of Fail, and because B overrides foo,
   |   the super-accessor in Fail is implemented as a call to
   |   super[B].foo.
   |3. However,
   |   X (the type of super[B].foo in Fail)
   |   is not a subtype of
   |   Y (the type of foo in trait C).
   |   Hence, the super-accessor that needs to be generated in Fail
   |   is illegal.
   |
   |Here are two possible ways to resolve this:
   |
   |1. Change the linearization order of Fail such that
   |   C comes before B.
   |2. Alternatively, replace super.foo in the body of trait C by a
   |   super-call to a specific parent, e.g. super[A].foo
one error found

/cc @adriaanm it'd be nice to align the messages between scalac and dotty here: scala/scala#7641

@smarter smarter requested a review from odersky February 28, 2019 12:25
@smarter smarter assigned odersky and unassigned smarter Feb 28, 2019
@smarter smarter changed the title Fix #5433: use a valid member to implement a super-accessor Fix #5433: check that the implemented super-accessor is valid Feb 28, 2019
@smarter smarter force-pushed the fix-super-mixin-2 branch from 1a84a4f to 84a102f Compare March 2, 2019 19:39
smarter added 3 commits March 4, 2019 14:55
Relying on `matchingDenotation` is not enough as demonstrated by
i5433.scala: in `Fail`, `B#foo` matches `C$$super$foo`
but it cannot implement it since `X` is a supertype of `Y`

Note that scalac seems had the same bug (but at least in Dotty this is
detected by -Ycheck:all), now fixed based on the logic in this PR by
scala/scala#7641.

For reference, here's what the spec says on resolving super accessors:

  A reference super.m refers statically to a method or type m in the
  least proper supertype of the innermost template containing the
  reference. It evaluates to the member m′ in the actual supertype of that
  template which is equal to m or which overrides m.
For some mysterious reason I still need to investigate, some invalid
super-calls in the stdlib that should be detected by the previous commit
are only detected when the files are compiled in a specific order, this
is made more confusing by the order of listed files being
non-deterministic by default. As a first step towards sanity, sort the
files to avoid non-determinism and consistently detect the invalid
super-calls.
...in the 2.12 stdlib which is part of our test suite.
@smarter smarter force-pushed the fix-super-mixin-2 branch from 84a102f to ce97789 Compare March 4, 2019 13:55
@odersky odersky removed their assignment Mar 4, 2019
@smarter smarter merged commit fb68cf4 into scala:master Mar 5, 2019
@allanrenucci allanrenucci deleted the fix-super-mixin-2 branch March 5, 2019 15:26
@smarter smarter mentioned this pull request Mar 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants