RFR: JDK-8222795: post inc operator inside compute function of HashMap results in Exception

[B. Blaser]

Hi Vicente,

I worked on postfix unary operators long time ago and I believe your
fix looks reasonable.

However, the need of relaxing the assertion as you suggested is still
tickling me a bit because 'visitLetExpr()' seems to correctly set the
start position:
http://hg.openjdk.java.net/jdk/jdk/file/fcf83b204c27/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java#l2313
which should be sufficient for this particular let-expression.

Does the problem come from the conditional expression?
Bernard


On Thu, 9 May 2019 at 00:26, Vicente Romero <vicente.romero at oracle.com> wrote:
>
> Please review fix for regression [1] at [2]. The regression was introduced by [3] which introduced the support for switch expressions. The compiler is failing with an assertion error for code:
>
> import java.util.HashMap;
> import java.util.Map;
>
> public class LetExpressionAssertionTest {
>     void m() {
>         Map<String, Integer> m = new HashMap<>();
>         m.put("a",2);
>         m.compute("a", (k, v) -> (v > 5) ? v-- : v++);
>     }
> }
>
> Where `v--` and `v++` are converted to let expressions. Recall that in this case `v` is of type integer so the let expressions are similar to:
>
> `(let /*synthetic*/ final Integer $373182087 = v in (let v = Integer.valueOf((int)(v.intValue() - 1)); in $373182087))`
>
> notice the cast, the ::intValue invocation etc.
>
> This patch, [3], also made some changes related to let expressions. In particular it changed a common idiom in Gen:
>
> `Assert.check(code.state.stacksize == 0);` by:
> Assert.check(code.isStatementStart());
>
> where Code::isStatementStart is:
>
> public boolean isStatementStart() {
>     return !alive || state.stacksize == letExprStackPos;
> }
>
> which is basically the same check as before, modulo the `alive` check. The problem is that at Gen::visitVarDef we had a more relaxed assertion check before [3]:
>
> Assert.check(letExprDepth != 0 || code.state.stacksize == 0); which was also substituted by:
> Assert.check(code.isStatementStart());
>
> this implies a semantic change. In the old code `letExprDepth` was just a counter of "nestedness" of let expressions and the original condition in Gen::visitVarDef was mostly to check if the compiler was generating a let expression or not. Enforcing that code.state.stacksize has to be equal to the stack position of the let expression when we started generating the let expression, which is what the current assertion is checking is a too strong condition. Which can be false if, as in this case, the let expression has several method invocations inside, etc that will move the stack as get generated. This patch is proposing going back to a more relaxed assertion at Gen::visitVarDef,
>
> Thanks,
> Vicente
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8222795
> [2] http://cr.openjdk.java.net/~vromero/8222795/webrev.00/
> [3] https://bugs.openjdk.java.net/browse/JDK-8206986