Skip to content

Investigate use of super-accessors in traits in the standard library #11420

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

Closed
smarter opened this issue Feb 28, 2019 · 5 comments
Closed

Investigate use of super-accessors in traits in the standard library #11420

smarter opened this issue Feb 28, 2019 · 5 comments

Comments

@smarter
Copy link
Member

smarter commented Feb 28, 2019

A super.foo call in a trait will require a super-accessor in every class that implements the trait since the super will be resolved based on the linearization order of the class. In most cases this is probably not what you want, e.g. in https://github.com/scala/scala/blob/d6455592055ce1729ab7ac11d69da3a5a5f3d9dc/src/library/scala/collection/Seq.scala#L187
I suspect what you really meant was:

super[IterableOps].concat(suffix)

Besides possible unwanted semantics and performance impact, super-accessors are also problematic because they cannot always be implemented (this is now detected as of scala/scala3#5604 / scala/scala3#5604), so I think it's worht reviewing their use before RC1 ships since adding or removing super-accesors is binary-incompatible.

@szeiger
Copy link

szeiger commented Feb 28, 2019

With traits becoming more powerful and class-like, does it even make sense that an unqualified 'super' in a trait refers to the implementing class's linearization order rather than the linearization as seen by the trait?

@smarter
Copy link
Member Author

smarter commented Feb 28, 2019

It's necessary to implement some patterns, but I think it should be more explicit, e.g. dynamicSuper.foo, that's a tricky thing to change though.

@szeiger
Copy link

szeiger commented Mar 7, 2019

I think https://github.com/scala/scala/blob/d6455592055ce1729ab7ac11d69da3a5a5f3d9dc/src/library/scala/collection/Seq.scala#L187 is correct and requires dynamic resolution. super could either be IterableOps or StrictOptimizedIterableOps. This is the case for must super delegations in collection methods.

I looked through all 184 uses of unqualified super calls in the standard library The following ones occur in traits:

scala.collection  (27 usages found)
    BitSet.scala  (5 usages found)
        172 else super.max(ord)
        178 else super.min(ord)
        244 case _ => super.concat(other)
        254 case _ => super.intersect(other)
        264 case _ => super.diff(other)
    LinearSeq.scala  (1 usage found)
        185 case _ => super.sameElements(that)
    Map.scala  (1 usage found)
        329 override def ++:[B >: (K, V)](that: IterableOnce[B]): IterableCC[B] = super.++:[B](that)
    Seq.scala  (2 usages found)
        187 def appendedAll[B >: A](suffix: IterableOnce[B]): CC[B] = super.concat(suffix)
        780 def lengthCompare(len: Int): Int = super.sizeCompare(len)
    SortedSet.scala  (2 usages found)
        91 else super.min
        97 else super.max
scala.collection.immutable  (101 usages found)
    Seq.scala  (7 usages found)
        55 case otherIndexedSeq: IndexedSeq[_] => length == otherIndexedSeq.length && super.canEqual(that)
        56 case _ => super.canEqual (that)
        89 case _ => super.sameElements(o)
        127 else super.slice(from, until)
scala.math  (1 usage found)
    Ordering.scala  (1 usage found)
        237 private[this] val _reverse = super.reverse

There is not a single one where I would feel 100% confident restricting it to a static supertrait. In most cases the dynamic look-up is clearly desirable.

@szeiger
Copy link

szeiger commented Mar 11, 2019

There doesn't appear to be anything more to do about this so I'm closing the issue.

@szeiger szeiger closed this as completed Mar 11, 2019
@SethTisue SethTisue removed this from the 2.13.0-RC1 milestone Mar 11, 2019
@smarter
Copy link
Member Author

smarter commented Mar 12, 2019

I think https://github.com/scala/scala/blob/d6455592055ce1729ab7ac11d69da3a5a5f3d9dc/src/library/scala/collection/Seq.scala#L187 is correct and requires dynamic resolution. super could either be IterableOps or StrictOptimizedIterableOps. This is the case for must super delegations in collection methods.

Hmm, in that case why is concat in that file marked as "TODO: ... Uncomment final" ? (Also the associated issue is closed now, not sure what the status of that is).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants