Use unitialized for wildcard initializers
#11231
Conversation
|
Whatever we do, if it touches the language and is not a critical issue, it should now wait for 3.1. We set that as a constraint to ourselves. |
odersky
force-pushed
the
fix-11225
branch
2 times, most recently
from
864be7c to
d719901
Compare
|
I think it's such a small thing and such an obvious improvement that we should not hesitate to wait with it. We want to argue that Scala 3 is a lot simpler than Scala 2. This change is a clear win. |
odersky
changed the title
notInitialized for wildcard initializersunitialized for wildcard initializers
|
Here's a reason for doing it now. The ProgFun2 MOOC contains two occurrences of the |
A lot of things are "clear wins". With clear wins like this we can still be there at Easter. It's a language change, and it is not done as feedback about Scala 3 features (in fact it's not even a Scala 3 feature in the first place). According to the decisions we took so far, we should not do this, no matter how small of clear win it is.
You said yourself that it's an obscure feature. How come ProgFun2 has two occurrences of this stuff? It seems to be in |
|
@sjrd If you want to give it a go, I'd be interested to have a look at another |
|
I also don't want to wait until Easter for 3.0. But adding this will not delay the release at all. We can merge now and be done with it. Correction: I still have to fix the slides for the MOOC and replace 40 seconds of the recording where I explain the syntax. Also we have to communicate the change to book authors. But it's infinitely simpler to do this now than post 3.0. |
https://github.com/lampepfl/moocs/pull/592 (this is a link to a private repo) |
|
Other relevant PR, this time public: #11238 |
|
@nicolasstucki Can you do a technical review for this? |
| cpy.ValDef(tree)(rhs = trivialErasedTree(tree)) | ||
| else tree | ||
| else tree.rhs match | ||
| case rhs: TypeApply |
There was a problem hiding this comment.
Should also have a case for Block(Nil, rhs: TypeApply) or possibly more nesting.
var a1: Int = uninitialized
var a2: Int = { uninitialized } // has an error
var b1: Unit = uninitialized
var b2: Unit = { uninitialized } // has an errorThere was a problem hiding this comment.
If uninitialized then we should also expect aliasing to work
inline def uninitialised[A] = uninitialized[A]
var x: Int = uninitialisedTherefore we would also need to look into Inline(_, _, _).
| ```scala | ||
| import scala.compiletime.uninitialized | ||
|
|
||
| var x: A = uninitialized |
There was a problem hiding this comment.
Currently, we also support
var a = uninitialized[A]This seems to be leaking implementation details.
There was a problem hiding this comment.
Right. We it's better to define it like this:
@compileTimeOnly("`uninitialized` can only be used as the right hand side of a mutable field definition")
def uninitialized: Nothing = ???
|
@nicolasstucki Yes, let's use Intuitively, the way I thought
|
|
@nicolasstucki I addressed the review comments. There's one remaining corner case: if |
There was a problem hiding this comment.
There are still a couple of bugs and technical inconsistencies.
Given that the following case slipped through the test cases.
class Foo:
var y0: Int = uninitialized // errorI would suggest improving test coverage by porting a few existing tests with = _ to use = uninitialized.
| * Finally, the phase replaces `compiletime.uninitialized` on the right hand side | ||
| * of a mutable field definition by `_`. This avoids a "is declared erased, but is | ||
| * in fact used" error in Erasure and communicates to Constructors that the | ||
| * variable does not have an initializer. |
There was a problem hiding this comment.
uninitialized is not erased anymore and the transformation has nothing to do with pruning erased definitions. It is also unrelated to erased in general as this operation does produce a value at runtime (even though it is elided). This should be done somewhere else. FirstTransform might be a good candidate as we are transforming var x: T = uninitialized to the canonical form that erasure understands.
| inline def g(inline x: Int): Unit = () | ||
| def f2 = g(uninitialized) // this one is ok since `uninitialized` is inlined away | ||
|
|
||
| var x7: Int = uni // error |
There was a problem hiding this comment.
This should be ok as the code we end up with is
var x7: Int = uni // ok
// after inlining
var x7: Int = uninitialized // still okThere was a problem hiding this comment.
Otherwise, we would need to make g(uninitialized) to be consistent with inlining/erased rules.
There was a problem hiding this comment.
It's an error since we end up with Inlined(uninitialized). I think that's OK. Ideally we would not allow the inline def of uni in the first place.
There was a problem hiding this comment.
Then it would be inconsistent with var x4: Int = if false then uninitialized else 1 // ok, since inlined away and def f2 = g(uninitialized) // this one is ok since uninitialized is inlined away. We would have some special inlining rules that only apply uninitialized which will confuse everyone.
There was a problem hiding this comment.
I think we need not overthink this. The intention is that you should not use uninitialized except as the RHS of a field definition. For all practical purposes that's what this PR achieves. We don't need to split hairs beyond that,
Fix recognition of uniniialized if type of var is a context function. This required a special case in typedValDef for `var x: A ?=> B = _`. Now it requires a special case in `PruneErasedDefs` in `var x: A ?=> B = undefined`.
Co-authored-by: Nicolas Stucki <[email protected]>
|
@odersky I realized today while pondering this (apparently too late) that this is more disruptive than I realized. The problem stems from the fact that this cannot be shimmed by a uniform inherited conditional compilation strategy. What I mean by that is abusing In this case, the difference which must be encapsulated is the initialization semantic in the constructor. A concrete example is instructive: https://github.com/typelevel/cats-effect/blob/bde1abdb8aa80b1d0d765db33f7ba1825d529978/core/shared/src/main/scala/cats/effect/IOFiber.scala#L91-L92 The problem here is that these definitions cannot be extracted into the parent type without materially impacting their runtime semantics. Extraction into the parent is the standard trick here because it allows the conditional compilation to be isolated to just the parts of the implementation which must be fundamentally distinct between language versions. You can see a related example of this in that same type:
Unfortunately, in this case it is impossible to isolate the changes to just a parent type, exposing them via inheritance, without sacrificing some measurable elements of the current system. This is for two reasons. First, moving variables to a supertype changes their location in the object header, which has an impact on page faults and, in the worst case, can create added indirection (I'd have to actually test it, but I believe this particular case would indeed result in added indirection due to the need to retain some variables in the child class). This is a JVM limitation. Second, and in a sense, worse: the encoding of Scala's And here we have the real crux of the problem: no one is using this syntax except in exceptionally performance-sensitive contexts, such as the file I linked. By definition, this feature is only meaningful for people who are sensitive to these kinds of performance degradations. But also by definition, it is impossible to avoid these kinds of degradations without doing one of two things:
The latter is the only option which will allow Cats Effect (and similar projects) to continue cross-publishing for Dotty, but unfortunately it also means we will be forced to needlessly duplicate the entire file in two source directories and manually keep them in sync. I invite you to peruse the file's contents further and judge for yourself how sustainable this is likely to be. All in all, I come back to my original premise: this is simply not worth it. This change imposes a perhaps-surprisingly significant burden on an already-overstretched ecosystem. It would be one thing if this were an enormously beneficial language refinement (such as removing unbounded type projection) which carried manifest benefits across multiple domains, but it's just... not. This is a tiny bit of syntax that most people never see, and arguably isn't even that terrible when you do see it. The marginal gains are small, and the marginal losses are substantial and self-compounding. Perhaps this is too late, but honestly and sincerely, I urge you to reconsider. |
|
A third option is back-porting this to 2.13 and 2.12. |
|
As a first step, why not just add a shim in def uninitialized[A]: A = null.asInstanceOf[A]Then we can see if this actually results in any visible performance regression. My guess would be that it doesn't, though one can never be sure with the JVM. |
|
@LPTK I've actually tested the performance regression in this case previously. It is quite measurable. Particularly since at least one of those fields is volatile, so you're adding a memory barrier where one did not previously exist. Backporting is an option here. Also the fact that it's only deprecated and not removed (I missed that in my PR review) helps quite a bit (and avoids the worst case scenarios I described), though it does mean that fatal warnings are off the table for a while, and we will just have this same discussion again when 3.1 or 3.2 rolls around. |
There's a bunch of things which are currently deprecated under |
|
Would a fourth option be making both compilers elide the bytecode emitted by |
It would mostly remove legitimate bytecode. Specifically, it would need to remove |
|
That could alter the meaning of some (admittedly poorly designed) programs. It could be that something assigns a non-zero value to the field before we get to its initialization code (for example, from the super constructor, through a virtual method, or more simply from previous statements in the template). The explicitly assignment to |
Fixes #11225