Base stat/expr checking on the type system in IRChecker #4439
Conversation
|
Note that |
|
|
||
| val expectedType = | ||
| if (tpe == NoType) AnyType | ||
| else tpe |
There was a problem hiding this comment.
This is a bit suspicious. I guess the deal here is that return must take an expression. I wonder if we should require UndefType instead (in a different PR, that would be a non-backward compatible change).
There was a problem hiding this comment.
I agree that it's weird that a Return node demands an expression, even when the result of the Labeled block is NoType.
For what it's worth, in the theory it was the Return node that had a specific exception, making void (NoType) invalid:
(note the S ≠ void)
Perhaps we could align the code of the IR checker with the typing rules a bit better by moving this change to the checking of Return.
If we have to change something in the future, I would rather relax the restriction on Return nodes, and make them accept "expressions" of type NoType if the type of the Labeled is NoType. That would be much nicer than requiring UndefType I believe. Plus, it would be backward compatible.
|
|
||
| case Block(trees) => | ||
| if (trees.isEmpty) | ||
| reportError("illegal empty block") |
There was a problem hiding this comment.
This check is unnecessary. A Block will fail to be created with no trees as it cannot determine it's type.
| @@ -1213,11 +1154,21 @@ private final class IRChecker(unit: LinkingUnit, logger: Logger) { | |||
| typecheckExpect(value, env, ctpe) | |||
| } | |||
|
|
|||
| case _ => | |||
| reportError(i"Invalid expression tree") | |||
| case _: JSSuperConstructorCall | _: RecordSelect | _: RecordValue | _: Transient => | |||
There was a problem hiding this comment.
| case _: JSSuperConstructorCall | _: RecordSelect | _: RecordValue | _: Transient => | |
| case _:JSSuperConstructorCall | _:RecordSelect | _:RecordValue | _:Transient => |
|
|
||
| val expectedType = | ||
| if (tpe == NoType) AnyType | ||
| else tpe |
There was a problem hiding this comment.
I agree that it's weird that a Return node demands an expression, even when the result of the Labeled block is NoType.
For what it's worth, in the theory it was the Return node that had a specific exception, making void (NoType) invalid:
(note the S ≠ void)
Perhaps we could align the code of the IR checker with the typing rules a bit better by moving this change to the checking of Return.
If we have to change something in the future, I would rather relax the restriction on Return nodes, and make them accept "expressions" of type NoType if the type of the Labeled is NoType. That would be much nicer than requiring UndefType I believe. Plus, it would be backward compatible.
| if (tpe == NoType) AnyType | ||
| else tpe | ||
|
|
||
| typecheck(body, env.withLabeledReturnType(label.name, expectedType)) |
There was a problem hiding this comment.
| typecheck(body, env.withLabeledReturnType(label.name, expectedType)) | |
| typecheckExpect(body, env.withLabeledReturnType(label.name, expectedType), tpe) |
This is important, as the type of the body must also be a subtype of the type of the Labeled block.
It's worth investigating, at least. |
|
Hmmm... this might expose some genuine failures. In fact, in this version the IR checker is much stricter: It expects intermediate trees to be properly typed as well. Consider: Block(
If(<cond>, IntLiteral(1), Skip()),
IntLiteral(2)
)In the previous version of the IR checker, the For example this failure: hints at that being the problem. The relevant if block: scala-js/javalib/src/main/scala/java/util/regex/GroupStartMapper.scala Lines 447 to 481 in 46d094d is in statement position, so the inner block may indeed be a statement. However, the |
|
Ouch, that would be bad. It seems like a plausible explanation to me, though. I wouldn't be surprised if scalac left |
|
Yes, adjusting printers to print the type, confirms this: Further, it is interesting to observe that the else branch actually contains |
786a41e to
a2c57a0
Compare
|
I'm coming back to this, after fixing the underlying problems. We'll probably need to put a deser hack in place, that changes the type of the following trees to
We have two ways to do this:
|
|
That seems like a good analysis. Overall I think I prefer the second option. It seems to be less intrusive to the design of the deserializer. It is also probably more efficient for reading newer IR (version 1.6+). |
|
@gzm0 I updated the PR with:
|
| @@ -1213,6 +1213,33 @@ object Serializers { | |||
| implicit val pos = readPosition() | |||
| val tag = readByte() | |||
|
|
|||
| def bodyHack15(body: Tree, isStat: Boolean): Tree = { | |||
There was a problem hiding this comment.
Don't we need to apply this treatment also to jsSuperClass?
There was a problem hiding this comment.
jsSuperclass is always a VarRef, when created by existing compilers, so it's not necessary.
(I'm pretty sure the names would be similarly safe, but I couldn't convince myself quickly enough.)
There was a problem hiding this comment.
Fair enough. Probably warrants a comment?
| override def transform(tree: Tree, isStat: Boolean): Tree = { | ||
| val newTree = super.transform(tree, isStat) | ||
| if (isStat && newTree.tpe != NoType) { | ||
| super.transform(newTree, isStat) match { |
There was a problem hiding this comment.
Hum, why are we transforming the tree twice?
There was a problem hiding this comment.
Oops, that's a leftover from a version before a refactoring. This should just be newTree match
|
Updated. |
Instead of distinguishing statements and expressions, we typecheck everything in the same way. Statements are just regular trees whose type happens to be NoType. We use this opportunity to re-order the match statements in the same order as the trees appear in Trees.scala. This change uncovered issues in the way we emit If, Match and TryCatch nodes in the compiler. Some of those nodes, when in statement position, were typed as something else than NoType, but had children typed as NoType. The previous IR checker did not flag those cases as errors, whereas the new one does. We introduce a deserialization hack to fix the type of If, Match and TryCatch nodes in statement position to always be NoType.
…osition In statement position, they must always be typed as NoType, since their children might be typed as such. We condition the deserialization hack introduced in the parent commit so that it only applies to old code.
This corresponds to the formal IR specification and solidifies the fact that any expression is allowed in statement position. The optimizer is only using the subtyping checks for expressions. We assert this in the code.
This will move errors to the containing trees, rather than the checked trees itself. This makes sense: Whether or not a tree is in expression position is a property of the usage, not the tree itself.
This allows us to remove some duplicate code for trees that can be in both statement or expression position.