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

[Jan Lahoda]

On Thu, 28 Mar 2024 08:37:00 GMT, Hannes Greule <hgreule at openjdk.org> wrote:

>> Pasting  e.g.
>> 
>> I m(I i, int x) {
>>    return switch (x) {
>>         default -> i;
>>    };
>> }
>> 
>> in jshell will cause a crash if `I` is not declared already. This comes down to javac not creating an error type for the value of the (implicit) yield from the switch.
>> 
>> Javac will not crash but swallow the exception, and create a file containing the command line options.
>> 
>> I first thought about just checking for null of the type here https://github.com/openjdk/jdk/blob/9ca4ae3d3b746f1d75036d189ff98f02b73b948f/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java#L1640 but after a closer look, the `checkIdInternal` method seems a better fit, as it also updates the type normally.
>
> 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 {
                          public Undefined g(Undefined u) {
                              return switch (0) {
                                  default -> u;
                              };
                          }
                      }
                      """;
        Path curPath = Path.of(".");
        List<String> actual = new JavacTask(tb)
                .options("-XDrawDiagnostics")
                .sources(code)
                .outdir(curPath)
                .run(Expect.FAIL, 1)
                .writeAll()
                .getOutputLines(OutputKind.DIRECT);

        List<String> expected = List.of(
                "C.java:2:24: compiler.err.cant.resolve.location: kindname.class, Undefined, , , (compiler.misc.location: kindname.class, C, null)",
                "C.java:2:12: compiler.err.cant.resolve.location: kindname.class, Undefined, , , (compiler.misc.location: kindname.class, C, null)",
                "2 errors"
        );

        if (!Objects.equals(actual, expected)) {
            error("Expected: " + expected + ", but got: " + actual);
        }
    }


(Note the explicit use of the expected exit code, which should guard against crashes. An alternative is to use `-XDdev` in the list of parameters, see other tests in the class - that should print the exception if it happens.)

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

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