-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Make vals with constant rhs have constant types #4763
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
Conversation
In Scala-2, val's get a constant type only if they are declared `final`. This is a horrible wart, of which I was ashamed from the beginning. In Dotty, we definitely do not want to continue with this tradition. So far, we keep the constant type if the `val` is labeled `inline`. But `inline` clashes with `inline` annotations and its unclear whether we want to keep it. So I tried to see what breaks if we simply say that `val`'s without a declared type and a constant rhs get a constant type. No additional modifier needed. Turns out, a number of small tests break, but they all seem to test this specific behavior. So it should be easy to change the tests. The main benefit of the change is: - we do not have to introduce another modifier for inlined vals - it's shorter and clearer - we can always get back the old behavior using an explicit type annotation. As a next step one could try to avoid widening all singleton types, not just constant types as in this PR. I have not tried it, since it has no relevance on the question how to retire final vals. Note that the situations in Scala-2 and Dotty are different because Dotty widens at different points. So it could well be that the change is not as easy in Scala-2.
// println(s"final inherited for $sym: ${inherited.toString}") !!! | ||
// println(s"owner = ${sym.owner}, decls = ${sym.owner.info.decls.show}") | ||
def isInline = sym.is(FinalOrInlineOrTransparent, butNot = Method | Mutable) | ||
|
||
// Widen rhs type and eliminate `|' but keep ConstantTypes if | ||
// definition is inline (i.e. final in Scala2) and keep module singleton types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this comment needs to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point
If I understand correctly, this enables inlining / constant folding for all val fields initialized with a literal? I worry about the fact that this breaks separate compilation (if a library changes a val from a literal to a different one, or some other expression, clients need to be re-compiled). |
@lrytz IIUC, indeed — with this change, But I'm not sure that makes things better for those who expect current semantics. |
Exactly, this is a silent change of existing semantics, something we should avoid as much as possible to avoid migration (and cross-compilation) headaches. The fact this is currently controlled by the |
I think it does:
|
The point is we want to encourage explicit types anyway. If you foresee a
I think that's overall clearer. I see the migration worries, though. |
The scenario where widening is actually more annoying is when trying to use path-dependent types — which isn't addressed here. And given our limited test coverage, it seems a bit circular to use it to test impact... @lrytz I know what you mean, but again technically that's NOT a violation of separate compilation, because otherwise changing from |
Actually it doesn't, at least in the Scala 2.13 implementation (try it out). The implementation makes a difference between |
@lrytz Uh that's interesting, but then I fear I don't get the point of 2.13's behavior. Let me try to understand — maybe we should replicate it in Dotty and it'd help this PR. My best guess is that this helps people who misunderstand the odd semantics of A different client, expecting object B {
val y: 1 = A.x
} So the change in question still requires recompiling clients — and then, what's the use-case for |
The point is that, for better or worse, we don't want side-effects hidden behind a a literal type to be elided. Eg. in, // A.scala
object A {
val x: 1 = { println("Hello world") ; 1 }
}
// B.scala
object B {
val y = A.x
} Here the literal typing of |
A literal-typed term doesn't need to be a compile-time constant:
|
jinx :) |
// A.scala
object A {
val x: 1 = { println("Hello world") ; 1 }
}
// B.scala
object B {
val y = A.x
}
It seems the problem is more like to be with const-folding, giving the following code: // A.scala
object A {
println("Hello world")
val x: 1 = 1
}
// B.scala
object B {
val y = A.x
} We should const-fold |
Can you say in what sense |
Indeed it's not a constant in code. But from a broader view of optimisation, it enables further optimisations that are faithful to the original program semantics. |
I don't quite get it ... if you fold this, you elide the side-effect. That isn't faithful to the original program semantics. If you can infer that the RHS is pure it would be a different matter. |
@milessabin You can safely inline (with care) something that isn't constant or has side effects, and I think that's what @liufengyun talks about — instead of cross-module constant folding, we should separate inline and only constant-fold actual constants after checking purity. So, given // A.scala
object A {
println("object init")
inline def x: 1 = { println("x init"); 1 } //`inline val` requires a val on the rhs.
}
// B.scala
object B {
val y = A.x + 1
} Then Separately, we could type the whole thing as In fact, we could restrict term-level inlining to Still, the change in this PR is still problematic. @lrytz Your examples are a good reason for distinguishing Erasure and lack of inlining means that if clients compile, they'll produce the same code. But clients might now be ill-typed and ill-behaved. Maybe this is sufficiently rare for primitive literal types (never used them, but users could disagree), but surely not so for singletons in general. |
(I think you meant I don't see that either. Suppose we also had, // C.scala
object C {
val z = A.x + 1
} Mutatis mutandis, |
@Blaisorblade ignore that ... I missed the change from I'm still unclear how this is supposed to interact with object initialization. Is the intention to check object bodies for purity and ban inline definitions if the object body contains any effects? |
Fixed thanks!
Actually I had missed that too initially, but
#4765 is exactly about that, so I'll answer there. |
I have the impression this is too controversial to get in. |
In Scala-2, val's get a constant type only if they are declared
final
. This isa horrible wart, of which I was ashamed from the beginning. In Dotty, we definitely do not
want to continue with this tradition. So far, we keep the constant type if the
val
islabeled
inline
. Butinline
clashes withinline
annotations and its unclear whetherwe want to keep it.
So I tried to see what breaks if we simply say that
val
's without a declaredtype and a constant rhs get a constant type. No additional modifier needed.
Turns out, a number of small tests break, but they all seem to test this
specific behavior. So it should be easy to change the tests.
The main benefit of the change is:
As a next step one could try to avoid widening all singleton types, not
just constant types as in this PR. I have not tried it, since it has no
relevance on the question how to retire final vals.
Note that the situations in Scala-2 and Dotty are different because Dotty
widens at different points. So it could well be that the change is not as
easy in Scala-2.