scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

Forbid `final object` #11094

Open Blaisorblade opened 6 years ago

Blaisorblade commented 6 years ago

First reported in https://github.com/lampepfl/dotty/issues/4936; both sealed and final are redundant on object, but final object seems pretty common (though it's redundant).

scala> trait A { object A }
defined trait A

scala> trait B extends A { object A }
<console>:12: error: overriding object A in trait A;
 object A cannot override final member
       trait B extends A { object A }
                                  ^

OTOH, final object seems pretty common, and it even appears in the new collections, maybe because it is sometimes used in such pattern (from Dotty) — and final on case classes does make sense.

sealed trait Status
final case class Success(output: String) extends Status
final case class Failure(output: String) extends Status
final case object Timeout extends Status

We're considering changing this in Dotty in https://github.com/lampepfl/dotty/pull/4973, but it'd be good to do the change in 2.14 as well I guess — or (EDIT) at least agree that we want to do it.

SethTisue commented 6 years ago

relevant: https://stackoverflow.com/a/26079667/86485

Blaisorblade commented 5 years ago

@nrinaudo asks on Gitter:

@SethTisue and @Blaisorblade , here's a question for you (related to your scala/bug#11094): if final object is going to be forbidden in 2.14, will all objects be flagged as final in the bytecode, or will we still have the current weirdness that case objects in a non-final class are non final in 2.12 (when they were in 2.11)?

Wasn't aware. Is that so??? In Dotty objects are always final, hence the flag is redundant, hence we now have a warning (the flag was used often enough that an error sounded too harsh). I don't know good reasons why Scalac would have non-final objects, but I have no insight in any transition issues here — I wasn't aware there was a transition.

nrinaudo commented 5 years ago

Sure. Take the following code:

class Test {
  case object foo
}

Compiled with 2.12.7, this yields the following decompiled bytecode:

public class Test$foo$ implements scala.Product,scala.Serializable {
 // [...]

On the other hand, the same code using 2.11.8, or the following code in 2.12.7:

class Test {
  final case object foo
}

Both compile to:

public final class Test$foo$ implements scala.Product,scala.Serializable {
  // [...]
Blaisorblade commented 5 years ago

Thanks! FWIW this is even advice: https://nrinaudo.github.io/scala-best-practices/adts/final_case_objects.html.

So, the last Dotty release gets this right. (Somebody should add a test to ensure that stays the case). I won't speculate on how hard changing this will be in Scalac 2.14, if 2.13 doesn't fix it already.

dwijnand commented 5 years ago

I don't know good reasons why Scalac would have non-final objects, but I have no insight in any transition issues here — I wasn't aware there was a transition.

Because of -Yoverride-objects?

(edit: oh, mentioned in Seth's SO)

Blaisorblade commented 5 years ago

Happy it’s gone in 2.13. Since Dotty can’t shadow classes, it can’t do that either.