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

[Vicente Romero]

ping,

Thanks,
Vicente

On 5/8/19 6:25 PM, Vicente Romero 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20190513/1846e413/attachment.html>