Check that constructor calls follow a chaining discipline. #5020
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
13a134b to
182ab54
Compare
08bb559 to
40222a0
Compare
|
@sjrd, do you know if / how the JVM validates constructor calls if it allows different calls depending on control flow? One one hand, disallowing stuff like: Seems restrictive for the IR level. On the other hand:
|
|
They basically have a different validation type for an uninitialized object and an initialized one. Then they have stack typing at each instruction. Finally after the constructor call the type of the argument transitions from uninitialized to initialized. At merge points, the JVM checks that the stack types align, and that the type of the We could do it as well, but IMO it's overkill. |
| val thisType = if (static) NoType else instanceThisType | ||
| val bodyEnv = Env.fromParams(params) | ||
| .withThisType(thisType) | ||
| .withInConstructor(isConstructor) |
There was a problem hiding this comment.
Does it make sense (maybe in a follow-up commit or even PR) to remove this env value and check StoreModule inside checkConstructorBody? (I realize this probably makes presence of the tree more restrictive).
There was a problem hiding this comment.
I think that would make sense, but I believe it belongs more to #5019 than this PR.
093a5a0 to
ba34ce5
Compare
* The constructor of `jl.Object` cannot call any other constructor. * Every other constructor must call exactly one other constructor, on `this`, either from the same class or the direct superclass. There cannot be any other call to constructors. These restrictions are also enforced by the JVM. Technically we are more restrictive because the JVM allows different code paths, each having a single constructor call, whereas we require exactly one at the top level. This is fine because both Scala and Java, as languages, enforce that more restrictive property. Unfortunately, we used to generate an illegal super constructor call in the synthetic classes that extend `DynamicImportThunk`. We patch them with a deserialization hack. In this commit, we do not yet change the compiler to fix the above.
ba34ce5 to
ee10158
Compare
And only enable the corresponding deserialization hack for IR < 1.18.
|
(rebased since we already have a commit changing the version on |
(Toned down my ambitions compared to #5019. This PR only checks the constructor chaining discipline as a first easier step.)
jl.Objectcannot call any other constructor.this, either from the same class or the direct superclass.There cannot be any other call to constructors.
These restrictions are also enforced by the JVM. Technically we are more restrictive because the JVM allows different code paths, each having a single constructor call, whereas we require exactly one at the top level. This is fine because both Scala and Java, as languages, enforce that more restrictive property.
Unfortunately, we used to generate an illegal super constructor call in the synthetic classes that extend
DynamicImportThunk. We patch them with a deserialization hack.In the first commit, we do not yet change the compiler to fix the above. In the second commit, we fix the compiler and only enable the deserialization hack on IR < 1.17.