[jdk17] RFR: 8267610: NPE at at jdk.compiler/com.sun.tools.javac.jvm.Code.emitop

[Guoxiong Li]

On Wed, 23 Jun 2021 14:41:02 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> Hi all,
>> 
>> Currently, the class TransPatterns sometimes doesn't transform the `pattern variables` and `pattern symbols` to the normal variables and symbols, especially the places where the pattern variables are used.
>> The following phases, such as LambdaToMethod, Lower and Gen, may crash or generate some wrong results.
>> 
>> The known issues are [JDK-8267610](https://bugs.openjdk.java.net/browse/JDK-8267610) and [JDK-8268748](https://bugs.openjdk.java.net/browse/JDK-8268748).
>> 
>> **JDK-8267610 is an issue that pattern symbol causes the compiler to crash.**
>> 
>> During transforming the InstanceOfTree (JCInstanceOf) , the `BindingSymbol`  instead of the `VarSymbol` is used to make the new `JCIdent` and `JCBinary` trees. At the phase LambdaToMethod, the compiler can't capture this variable so that the lambda method has uncorrect parameters. So at the phase Gen, the compiler crashes because of NPE.
>> 
>> **JDK-8268748 is an issue that pattern symbol causes that the compiler generates wrong bytecodes.**
>> 
>> When transforming the BindingPatternTree (JCBindingPattern), the `BindingSymbol` is also handled uncorrectly and used to make the new `JCIdent` and `JCAssign` trees. At the phase Gen, the compiler find the wrong variables, so that the wrong bytecodes are generated.
>> 
>> These two issues are similar and influence each other. So I solve them at one patch.
>> The lines 208-212 are to solve JDK-8267610 with the test `LambdaCannotCapturePatternVariables`. 
>> The lines 233-239 are to solve JDK-8268748 with the test `NestedPatternVariablesBytecode`.
>> 
>> If lines 208-212 are not included, the test  `NestedPatternVariablesBytecode` can't pass. 
>> If lines 233-239 are not included, the test `LambdaCannotCapturePatternVariables` can't pass. 
>> So I put them together.
>> 
>> Thanks for taking the time to review.
>> 
>> Best Regards,
>> -- Guoxiong.
>
> Thanks for looking into this and sorry for trouble. Looking at the implementation, would it make sense to set the currentValue only after the expression is translated? E.g. something like:
> 
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java
> index 4c077968ff5..c57294bf4ab 100644
> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java
> @@ -189,12 +189,14 @@ public class TransPatterns extends TreeTranslator {
>              //=>
>              //(let T' N$temp = E; N$temp instanceof typeof($pattern) && <desugared $pattern>)
>              //note the pattern desugaring performs binding variable assignments
> -            Symbol exprSym = TreeInfo.symbol(tree.expr);
>              Type tempType = tree.expr.type.hasTag(BOT) ?
>                      syms.objectType
>                      : tree.expr.type;
>              VarSymbol prevCurrentValue = currentValue;
>              try {
> +                JCExpression translatedExpr = translate(tree.expr);
> +                Symbol exprSym = TreeInfo.symbol(translatedExpr);
> +
>                  if (exprSym != null &&
>                      exprSym.kind == Kind.VAR &&
>                      exprSym.owner.kind.matches(Kinds.KindSelector.VAL_MTH)) {
> @@ -206,7 +208,6 @@ public class TransPatterns extends TreeTranslator {
>                              currentMethodSym);
>                  }
>  
> -                JCExpression translatedExpr = translate(tree.expr);
>                  Type principalType = principalType((JCPattern) tree.pattern);
>                  result = makeBinary(Tag.AND,
>                                      makeTypeTest(make.Ident(currentValue), make.Type(principalType)),
> 
> 
> I think the `currentValue` is needed only when the pattern is being processed, not when the expression is being processed, unless I am mistaken.

@lahodaj thanks for your comment.

I have tried your way before submitting this PR. But the compiler didn't work as expected.
It is because the following translations need the right `currentValue` to complete the translation.
If we don't set the `currentValue` before these translations, the result may be wrong.


                JCExpression translatedExpr = translate(tree.expr);
                Type principalType = principalType((JCPattern) tree.pattern);


So I revised the code like this patch which seems to use unnecessary variable `actualVar`.
Actually, this redundant step is necessary to avoid the regression.

-------------

PR: https://git.openjdk.java.net/jdk17/pull/59