Addressing some issues in AntiAliasing #1245
Conversation
01cd938 to
667ef3d
Compare
| case Assignment(v, expr) => | ||
| // We explicitly override this case to not have the recursion go through `v` and potentially taking the above case | ||
| // (otherwise, we could get something like `env.rewritings(v.toVal) = expr` which may not be well-defined). | ||
| Assignment(v, transform(expr, env)) |
There was a problem hiding this comment.
If v belongs to env.rewritings, doesn't that mean that we've added a LetVar to the rewritings map? That would be incorrect in itself, so if this case is cropping up we might have an underlying issue.
There was a problem hiding this comment.
It appears that the only vs for which this case is problematic are those that are of a mutable type and val-defined (such as val x = if (b1) a1 else a2 in invalid/i916a1).
Such rewriting in env is added by:
and seems to be intended.
There was a problem hiding this comment.
But when are we seeing an Assignment(x, ...)? Scala vals can't be assigned, are we applying a transformation twice?
There was a problem hiding this comment.
val x is transformed into a var x and it is re-assigned each time we modify one of its components.
If we e.g. see x.a = 123, it is transformed into an assignment: x = A(123)
There was a problem hiding this comment.
Ah, I see. Thanks for bearing with me :)
I don't like the fact that we're applying transform to assignments produced during the phase since it makes it difficult to distinguish issues in the phase from issues in the input program. I think it would be worth cleaning up these cases to avoid applying transform to the generated Assignment expressions. For example,
should probably be something like below instead
val applied = updatedTarget(target, v)
Assignment(target.receiver, transform(applied, env)).copiedFrom(up)There was a problem hiding this comment.
Good note, I'll address that in a separate PR
| val combinedCond = Some(cond).zip(effectCond).map { | ||
| case (BooleanLiteral(b1), c2) => | ||
| if (b1) c2 else BooleanLiteral(false) | ||
| case (c1, BooleanLiteral(b2)) => | ||
| if (b2) c1 else BooleanLiteral(false) | ||
| case (c1, c2) => | ||
| And(c1, c2) | ||
| }.getOrElse(cond) |
There was a problem hiding this comment.
I think you can replace this by andJoin(cond +: effectCond.toSeq) for better readability.
Hopefully fixes the underlying problems of #916, #1099, and #1122 without introducing new bugs...