Skip to content

Fix #2051: allow override T with => T or ()T #2096

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 3 commits into from
Mar 15, 2017

Conversation

liufengyun
Copy link
Contributor

Fix #2051: allow override T with => T or ()T.

I'm not sure if this is the right fix. Could you please advise @DarkDimius ?

@odersky
Copy link
Contributor

odersky commented Mar 14, 2017

What about the following test:

abstract class C { val x: Int }
class D extends C { def x = 1 }

We are good if that one still gives the error

          overriding value x in class C of type Int;
          method x of type => Int needs to be a stable, immutable value

@liufengyun
Copy link
Contributor Author

The case above is rejected by some other check, we still have the same error as before. I just added it as a neg test.

@odersky
Copy link
Contributor

odersky commented Mar 14, 2017

OK, I think we are good then. @smarter since you are currently playing with override val's do you see a reason why we should not do this?

@smarter
Copy link
Member

smarter commented Mar 14, 2017

Not that I can think of but this change still seems weird to me, what is special about constructor parameters here?

Copy link
Contributor

@DarkDimius DarkDimius 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

@@ -0,0 +1,2 @@
class A[T](val x:T)
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nice to add several fields to the body of the class that are overriden in a subclass.

It would also be nice to include tests with trait vals and trait constructors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @DarkDimius , I just added tests in the latest commit.

@liufengyun
Copy link
Contributor Author

@smarter the reason is that x: T is already present in the parent class, in post-typer, we just create a forwarder override def x: T = super.x.

@DarkDimius DarkDimius merged commit 72d5aaa into scala:master Mar 15, 2017
@allanrenucci allanrenucci deleted the fix-i2051 branch December 14, 2017 19:20
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.

Cannot override a val from the parent class
4 participants