Skip to content
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

Compat Scala 2: Abstract override #4770

Open
allanrenucci opened this issue Jul 6, 2018 · 13 comments
Open

Compat Scala 2: Abstract override #4770

allanrenucci opened this issue Jul 6, 2018 · 13 comments
Labels
area:refchecks backlog No work planned on this by the core team for the time being. compat:scala2 itype:bug itype:question stat:needs spec

Comments

@allanrenucci
Copy link
Contributor

class Seq

trait IterableOnceOps {
  def toSeq: Seq = new Seq
}

trait IterableOps extends IterableOnceOps

trait SeqOps extends IterableOps {
  def toSeq: Seq
}

class Foo extends SeqOps

The code snippet above compiles with Scala2 but not Dotty:

-- Error: tests/allan/Test.scala:53:6 ------------------------------------------
53 |class Foo extends SeqOps
   |      ^
   | class Foo needs to be abstract, since def toSeq: => Seq is not defined 
one error found

This pattern is used in the 2.13 collection library

@allanrenucci
Copy link
Contributor Author

cc/ @julienrf

@allanrenucci
Copy link
Contributor Author

Followup, If I do decide to implement toSeq in Foo, should I be required to add the override modifier:

class Foo extends SeqOps {
  def toSeq: Seq = new Seq
}
tests/allan/Test.scala:54: error: overriding method toSeq in trait IterableOnceOps of type => Seq;
 method toSeq needs `override` modifier
  def toSeq: Seq = new Seq
      ^
one error found

@Blaisorblade
Copy link
Contributor

  1. This seems to be specified as allowed (tho I'd clarify the spec), so I'm removing the Scala2 label — see member h in this example from https://www.scala-lang.org/files/archive/spec/2.13/05-classes-and-objects.html#overriding:
trait A { def f: Int }
trait B extends A { def f: Int = 1 ; def g: Int = 2 ; def h: Int = 3 }
trait C extends A { override def f: Int = 4 ; def g: Int }
trait D extends B with C { def h: Int }

Also, the text which defines overriding never forbids this scenario:

This definition also determines the overriding relationships between matching members of a class C and its parents. First, a concrete definition always overrides an abstract definition. Second, for definitions M and M' which are both concrete or both abstract, M overrides M′ if M appears in a class that precedes (in the linearization of C) the class in which M′ is defined.

  1. Foo.toSeq overrides an existing implementation, so it seems override should be required as usual to ensure that the override is desired — luckily, that's even clear in the spec (https://www.scala-lang.org/files/archive/spec/2.13/05-classes-and-objects.html#override).

https://www.scala-lang.org/files/archive/spec/2.13/05-classes-and-objects.html#overriding

@Blaisorblade
Copy link
Contributor

Apparently this is an intended change and matches Java semantics (though Martin didn't go into detail), so the spec should be updated and the compat:Scala2 label stays.

@smarter
Copy link
Member

smarter commented Jul 6, 2018

We should also see if we can make a rewrite rule for this.

allanrenucci added a commit to dotty-staging/scala that referenced this issue Jul 26, 2018
If a concrete members is overridden by an abstract member, children must
provide an implementation in Dotty. See scala/scala3#4770
allanrenucci added a commit to dotty-staging/scala that referenced this issue Aug 2, 2018
If a concrete members is overridden by an abstract member, children must
provide an implementation in Dotty. See scala/scala3#4770
allanrenucci added a commit to dotty-staging/scala that referenced this issue Aug 2, 2018
If a concrete members is overridden by an abstract member, children must
provide an implementation in Dotty. See scala/scala3#4770
sjrd added a commit to sjrd/scala-js that referenced this issue Jul 26, 2020
The abstract def SetFactory.allowsNullElement shadowed the concrete
def of the same name in CollectionFactory. This is not allowed in
Dotty anymore:
scala/scala3#4770

Since this is not necessary in our tests, there is no point in
leaving that abstract def.
@dwijnand
Copy link
Member

Apparently this is an intended change and matches Java semantics (though Martin didn't go into detail), so the spec should be updated and the compat:Scala2 label stays.

This came up in the context of scala/bug#12224 (ty @Jasper-M!). Could you go more into detail on why this was changed? In #2072 it seemed like it was understood to be an issue in Dotty, but then it just fizzled out...

@som-snytt
Copy link
Contributor

I remember learning from this SO answer:

https://stackoverflow.com/questions/10213395/cake-pattern-with-overriding-abstract-type-dont-work-with-upper-type-bounds/

which now compiles on Scala 3, as promised in 2012:

trait A {
  def ping = println("ping")
}

trait Cake {
  type T
}

trait S { this: Cake =>  // S with Cake
  //type T = A   // concrete overrides abstract
  type T <: A    // but abstract was overridden by abstract in linearization order; loses the bound in Scala 2
  def t: T
  def f = t.ping  // was: error: value ping is not a member of S.this.T
}

Just to document this related change here.

@dwijnand
Copy link
Member

ping @odersky when you get a second, do you remember why this is an intended change in Scala 3?

@sjrd
Copy link
Member

sjrd commented Nov 16, 2020

Because it's what Java does, as well as what the JVM does for default methods IIRC.

@dwijnand
Copy link
Member

Any ideas why Java does it that way? Is there a reason why this is bad? For instance, in the n years that this has worked this way in Scala 2, has caused an issue? Or perhaps there's implementation reasons behind this?

@sjrd
Copy link
Member

sjrd commented Nov 16, 2020

I have no idea.

@Jasper-M
Copy link
Member

Jasper-M commented Nov 16, 2020

Only discussion related to this that I can remember is https://contributors.scala-lang.org/t/ability-to-make-a-parents-concrete-method-abstract/1255

It's also pointed out in that thread that Scala 2 already does the "Java thing" when it comes to abstract classes.

scala> class Baz { def foo: String = "string" }
class Baz

scala> trait Foo extends Baz{
     |   override def foo: String
     | }
trait Foo

scala> new Foo{}
val res5: Baz with Foo = $anon$1@4374e0

scala> abstract class Bar extends Baz{
     |   override def foo: String
     | }
class Bar

scala> new Bar{}
           ^
       error: object creation impossible. No implementation found in a subclass for deferred declaration
       override def foo: String (defined in class Bar)

In case that's news to anyone.

@lrytz
Copy link
Member

lrytz commented Nov 18, 2020

Scala 2 allows widening the type in an abstract override, but that seems a pretty obscure feature

scala> trait T { def f: String = "" }; trait U extends T { override def f: Object }
trait T
trait U

@odersky odersky added the backlog No work planned on this by the core team for the time being. label Apr 5, 2022
jtjeferreira added a commit to jtjeferreira/playframework that referenced this issue Nov 21, 2022
jtjeferreira added a commit to jtjeferreira/playframework that referenced this issue Jan 30, 2023
jtjeferreira added a commit to jtjeferreira/playframework that referenced this issue Jan 30, 2023
jtjeferreira added a commit to jtjeferreira/playframework that referenced this issue Jan 30, 2023
jtjeferreira added a commit to jtjeferreira/playframework that referenced this issue Jan 30, 2023
mkurz pushed a commit to jtjeferreira/playframework that referenced this issue Feb 17, 2023
mkurz pushed a commit to jtjeferreira/playframework that referenced this issue Feb 18, 2023
mkurz pushed a commit to jtjeferreira/playframework that referenced this issue Feb 27, 2023
mkurz pushed a commit to jtjeferreira/playframework that referenced this issue Mar 7, 2023
mkurz pushed a commit to mkurz/playframework that referenced this issue Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:refchecks backlog No work planned on this by the core team for the time being. compat:scala2 itype:bug itype:question stat:needs spec
Projects
None yet
Development

No branches or pull requests

9 participants