RFR: 8328536: javac - crash on unknown type referenced in yield statement [v2]

[Hannes Greule]

On Thu, 16 May 2024 17:01:08 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> Hannes Greule has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>> 
>>  - Merge branch 'refs/heads/master' into fix/switch-yield-unknown-type-crash
>>  - add bug id
>>  - Fix jshell crash on unknown type in switch yield
>
> I apologize for a belated reply. I agree with not adding a null check, that is usually a wrong/insufficient approach. And I think I agree with your approach, but I am not sure if it goes far enough.
> 
> I think I would try to go with something like:
> 
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
> index d0f3ae7464a..4fe1b4430ec 100644
> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
> @@ -4623,9 +4623,6 @@ Type checkIdInternal(JCTree tree,
>                       Type pt,
>                       Env<AttrContext> env,
>                       ResultInfo resultInfo) {
> -            if (pt.isErroneous()) {
> -                return types.createErrorType(site);
> -            }
>              Type owntype; // The computed type of this identifier occurrence.
>              switch (sym.kind) {
>              case TYP:
> @@ -4734,6 +4731,10 @@ else if (ownOuter.hasTag(CLASS) && site != ownOuter) {
>                  chk.checkPreview(tree.pos(), env.info.scope.owner, sym);
>              }
>  
> +            if (pt.isErroneous()) {
> +                owntype = types.createErrorType(owntype);
> +            }
> +
>              // If symbol is a variable, check that its type and
>              // kind are compatible with the prototype and protokind.
>              return check(tree, owntype, sym.kind.toSelector(), resultInfo);
> 
> 
> I.e., let the `checkIdInternal` run as normally, just wrap the type with an error type. When I ran tests with this change, three failed for me:
> 
> FAILED: tools/javac/generics/diamond/7188968/T7188968.java
> FAILED: tools/javac/lambda/methodReference/MethodRefToInnerWithoutOuter.java
> FAILED: tools/javac/lambda/MethodReference23.java
> 
> 
> For the `tools/javac/lambda` tests, the new errors seemed better/more appropriate than the original ones, so those are not a problem I think (but an independent check would be good).
> 
> The `tools/javac/generics/diamond/7188968/T7188968.java` is producing more unchecked warnings with that change, and I am not quite sure if that's appropriate.
> 
> As for a test - test inside JShell is good, but there should be a test inside somewhere `tools/javac`. A good place might be `test/langtools/tools/javac/recovery/AttrRecovery.java`:
> 
>     @Test
>     public void testErroneousTarget() throws Exception {
>         String code = """
>                       public class C {
>                  ...

Thanks for the comment @lahodaj.

Not bailing out so early makes sense I guess. I also looked into the new warnings of `T7188968` with your suggested changes. Adding a `Object unknown = null;` to make the code compile will cause the same warnings, so if they make sense with compiling code, I assume they also make sense with broken code.

Let me know if you want to go forward with your suggestion then. I will, however, most likely only find time to do the changes next week.

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

PR Comment: https://git.openjdk.org/jdk/pull/18383#issuecomment-2116916391