Fix #444: be more conservative on when Block and If types should be recomputed #5958
Conversation
There was a problem hiding this comment.
Hello, and thank you for opening this PR! 🎉
All contributors have signed the CLA, thank you! ❤️
Commit Messages
We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).
Please stick to these guidelines for commit messages:
- Separate subject from body with a blank line
- When fixing an issue, start your commit message with
Fix #<ISSUE-NBR>:- Limit the subject line to 72 characters
- Capitalize the subject line
- Do not end the subject line with a period
- Use the imperative mood in the subject line ("Add" instead of "Added")
- Wrap the body at 80 characters
- Use the body to explain what and why vs. how
adapted from https://chris.beams.io/posts/git-commit
Have an awesome day! ☀️
… be recomputed.
liufengyun
force-pushed
the
fix-if-block
branch
from
7f6afc7 to
ff7a67c
Compare
There was a problem hiding this comment.
Looking at the original issue, I see Dmitry later had a different fix in master...dotty-staging:signatures-issues, but the one you picked also seems correct and simpler, so I'm inclined to go with it. However you should remove the transformBlock and transformIf from ElimErasedValueType which were added to workaround the bug, you should also add the vcblock and vcif test cases that triggered the block, you can find them in smarter@4ddaf55
We also need to run the performance tests to make sure we're not regressing.
| @@ -572,15 +572,25 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo { | |||
| override def Block(tree: Tree)(stats: List[Tree], expr: Tree)(implicit ctx: Context): Block = { | |||
| val tree1 = untpdCpy.Block(tree)(stats, expr) | |||
| tree match { | |||
| case tree: Block if expr.tpe eq tree.expr.tpe => tree1.withTypeUnchecked(tree.tpe) | |||
| case tree: Block if (expr.tpe eq tree.expr.tpe) && (expr.tpe eq tree.tpe) => | |||
| // the second guard is needed in case avoid somehow widened the type. | |||
There was a problem hiding this comment.
More detailed comment:
// The last guard is a conservative check: if `tree.tpe` is different from `expr.tpe`, then
// it was computed from widening `expr.tpe`, and tree transforms might cause `expr.tpe.widen`
// to change even if `expr.tpe` itself didn't change, e.g:
// { val s = ...; s }
// If the type of `s` changed, then the type of the block might have changed, even though `expr.tpe`
// will still be `TermRef(NoPrefix, s)`| case tree: If if (thenp.tpe eq tree.thenp.tpe) && (elsep.tpe eq tree.elsep.tpe) => tree1.withTypeUnchecked(tree.tpe) | ||
| case tree: If if (thenp.tpe eq tree.thenp.tpe) && (elsep.tpe eq tree.elsep.tpe) && | ||
| ((tree.tpe eq thenp.tpe) || (tree.tpe eq elsep.tpe)) => | ||
| // last guard is needed in case previous if had computed a widened ORType that needs to be recomputed |
There was a problem hiding this comment.
New comment:
// The last guard is a conservative check similar to the one done in `Block` above,
// if `tree.tpe` is not identical to the type of one of its branch, it might have been
// computed from the widened type of the branches, so the same reasoning than
// in `Block` applies.
|
... I wish this wasn't merged so fast, my comments still stand. |
|
@smarter Thanks a lot, I'll address the comment in another PR. |
Fix #444: be more conservative on when Block and If types should be recomputed
It seems that @DarkDimius forgot to make a PR after the fix.