Skip to content

Fix #1501 - Check trait inheritance condition #1931

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 7 commits into from
Feb 13, 2017

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Feb 2, 2017

We need to check a coherence condition between the superclass
of a trait and the superclass of an inheriting class or trait.

@smarter
Copy link
Member

smarter commented Feb 2, 2017

The following compiles in dotty but not scalac, is this intentional?

class A
class SubA extends A
trait TA extends A
trait TSubA extends SubA

class Foo extends TA with TSubA

In dotty the last line becomes after typer:

class Foo() extends SubA() with TA with TSubA

So it's correct, but in scalac we get:

 class Foo extends A with TA with TSubA

So it complains (superclass A is not a subclass of the superclass SubA ...).

If this behavior is intended the spec will need to be updated:

It is possible to write a list of parents that starts with a trait reference, e.g. mt1 with … with mtn. In that case the list of parents is implicitly extended to include the supertype of mt1 as first parent type.

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

@DarkDimius
Copy link
Contributor

I think that the question @smarter asked is incomplete without discussing order of constructor evaluation, ie: in which order should super-initializers be called for Foo?

@DarkDimius
Copy link
Contributor

Class Foo has to start by calling the super-call of SubA, which whould invoke A and SubA.

IE the starting sequence is fixed:

Object
A
SubA

Now, we have flexibility to continue by calling TA and TSubA.

I think this is a valid and expected order for what was created by typer and should be allowed.

val psuper = parent.superClass
val csuper = cls.superClass
val ok = csuper.derivesFrom(psuper) ||
parent.is(JavaDefined) && csuper == defn.AnyClass && psuper == defn.ObjectClass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be too permissive: as far as I can tell, scalac only allows allows this for two Java classes: Serializable and Comparable which are treated as if they were traits that extend Any: https://github.com/scala/scala/blob/cac6383e6658dc5956540b76b9b46c3b664774ac/src/reflect/scala/reflect/internal/Definitions.scala#L365-L366 If we allow any Java class to be used in universal traits, then we break the definition of universal trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll change that.

@odersky
Copy link
Contributor Author

odersky commented Feb 2, 2017

@DarkDimius Right. I just verified that this does does not lead to inconsistencies when classes take parameters.

@odersky
Copy link
Contributor Author

odersky commented Feb 2, 2017

We should note that this requires a spec change.

@odersky
Copy link
Contributor Author

odersky commented Feb 3, 2017

/rebuild

@odersky
Copy link
Contributor Author

odersky commented Feb 8, 2017

Why don't we see a drone result here? validate-main is pending as usual, but the drone should still be running, right?

@smarter
Copy link
Member

smarter commented Feb 8, 2017

No clue, I haven't seen that on other PRs, try git commit --amend && git push --force ?

We need to check a coherence condition between the superclass
of a trait and the superclass of an inheriting class or trait.
Need to take account of situations like

    extends Any with java.io.Serializable

which occur in stdlib.
The leading class should be the superclass of the first trait
(which is not always Object).

We could think of a more refined condition, (i.e. taking the least
common superclass of all extended traits), but I think it's not worth
it, as one can always spell out the right superclass manually.
Excepted are only Serializable and Comparable. This follows
scalac's behavior.
@smarter smarter merged commit b297832 into scala:master Feb 13, 2017
@allanrenucci allanrenucci deleted the fix-#1501 branch December 14, 2017 16:59
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.

3 participants