changeset 3749:16f0cf12da2d

8129559: JShell: compilation fails if class, method or field is annotated and has modifiers 8080354: JShell: Runtime visible annotations cannot be retrieved Reviewed-by: jlahoda
author rfield
date Fri, 04 Nov 2016 14:47:25 -0700
parents 74a391527c2c
children 5892a55c7bf9
files src/jdk.jshell/share/classes/jdk/jshell/MaskCommentsAndModifiers.java test/jdk/jshell/ClassMembersTest.java test/jdk/jshell/ClassesTest.java test/jdk/jshell/CompletenessTest.java test/jdk/jshell/IgnoreTest.java test/jdk/jshell/ModifiersTest.java
diffstat 6 files changed, 265 insertions(+), 159 deletions(-) [+]
line wrap: on
line diff
--- a/src/jdk.jshell/share/classes/jdk/jshell/MaskCommentsAndModifiers.java	Fri Nov 04 17:52:11 2016 +0000
+++ b/src/jdk.jshell/share/classes/jdk/jshell/MaskCommentsAndModifiers.java	Fri Nov 04 14:47:25 2016 -0700
@@ -36,10 +36,14 @@
  */
 class MaskCommentsAndModifiers {
 
-    private final static Set<String> IGNORED_MODIFERS =
+    private final static Set<String> IGNORED_MODIFIERS =
             Stream.of( "public", "protected", "private", "static", "final" )
                     .collect( Collectors.toSet() );
 
+    private final static Set<String> OTHER_MODIFIERS =
+            Stream.of( "abstract", "strictfp", "transient", "volatile", "synchronized", "native", "default" )
+                    .collect( Collectors.toSet() );
+
     // Builder to accumulate non-masked characters
     private final StringBuilder sbCleared = new StringBuilder();
 
@@ -52,24 +56,28 @@
     // Entire input string length
     private final int length;
 
-    // Should leading modifiers be masked away
-    private final boolean maskModifiers;
-
-    // The next character
+    // The next character position
     private int next = 0;
 
-    // We have past any point where a top-level modifier could be
-    private boolean inside = false;
+    // The current character
+    private int c;
+
+    // Do we mask-off ignored modifiers?  Set by parameter and turned off after
+    // initial modifier section
+    private boolean maskModifiers;
 
     // Does the string end with an unclosed '/*' style comment?
     private boolean openComment = false;
 
-    @SuppressWarnings("empty-statement")
     MaskCommentsAndModifiers(String s, boolean maskModifiers) {
         this.str = s;
         this.length = s.length();
         this.maskModifiers = maskModifiers;
-        do { } while (next());
+        read();
+        while (c >= 0) {
+            next();
+            read();
+        }
     }
 
     String cleared() {
@@ -90,24 +98,33 @@
      * Read the next character
      */
     private int read() {
-        if (next >= length) {
-            return -1;
-        }
-        return str.charAt(next++);
+        return c = (next >= length)
+                ? -1
+                : str.charAt(next++);
     }
 
-    private void write(StringBuilder sb, int ch) {
+    private void unread() {
+        if (c >= 0) {
+            --next;
+        }
+    }
+
+    private void writeTo(StringBuilder sb, int ch) {
         sb.append((char)ch);
     }
 
     private void write(int ch) {
-        write(sbCleared, ch);
-        write(sbMask, Character.isWhitespace(ch) ? ch : ' ');
+        if (ch != -1) {
+            writeTo(sbCleared, ch);
+            writeTo(sbMask, Character.isWhitespace(ch) ? ch : ' ');
+        }
     }
 
     private void writeMask(int ch) {
-        write(sbMask, ch);
-        write(sbCleared, Character.isWhitespace(ch) ? ch : ' ');
+        if (ch != -1) {
+            writeTo(sbMask, ch);
+            writeTo(sbCleared, Character.isWhitespace(ch) ? ch : ' ');
+        }
     }
 
     private void write(CharSequence s) {
@@ -122,99 +139,105 @@
         }
     }
 
-    private boolean next() {
-        return next(read());
-    }
-
-    private boolean next(int c) {
-        if (c < 0) {
-            return false;
-        }
-
-        if (c == '\'' || c == '"') {
-            inside = true;
-            write(c);
-            int match = c;
-            c = read();
-            while (c != match) {
-                if (c < 0) {
-                    return false;
-                }
-                if (c == '\n' || c == '\r') {
+    private void next() {
+        switch (c) {
+            case '\'':
+            case '"':
+                maskModifiers = false;
+                write(c);
+                int match = c;
+                while (read() >= 0 && c != match && c != '\n' && c != '\r') {
                     write(c);
-                    return true;
-                }
-                if (c == '\\') {
-                    write(c);
-                    c = read();
-                }
-                write(c);
-                c = read();
-            }
-            write(c);
-            return true;
-        }
-
-        if (c == '/') {
-            c = read();
-            if (c == '*') {
-                writeMask('/');
-                writeMask(c);
-                int prevc = 0;
-                while ((c = read()) != '/' || prevc != '*') {
-                    if (c < 0) {
-                        openComment = true;
-                        return false;
-                    }
-                    writeMask(c);
-                    prevc = c;
-                }
-                writeMask(c);
-                return true;
-            } else if (c == '/') {
-                writeMask('/');
-                writeMask(c);
-                while ((c = read()) != '\n' && c != '\r') {
-                    if (c < 0) {
-                        return false;
-                    }
-                    writeMask(c);
-                }
-                writeMask(c);
-                return true;
-            } else {
-                inside = true;
-                write('/');
-                // read character falls through
-            }
-        }
-
-        if (Character.isJavaIdentifierStart(c)) {
-            if (maskModifiers && !inside) {
-                StringBuilder sb = new StringBuilder();
-                do {
-                    write(sb, c);
-                    c = read();
-                } while (Character.isJavaIdentifierPart(c));
-                String id = sb.toString();
-                if (IGNORED_MODIFERS.contains(id)) {
-                    writeMask(sb);
-                } else {
-                    write(sb);
-                    if (id.equals("import")) {
-                        inside = true;
+                    if (c == '\\') {
+                        write(read());
                     }
                 }
-                return next(c); // recurse to handle left-over character
-            }
-        } else if (!Character.isWhitespace(c)) {
-            inside = true;
+                write(c); // write match // line-end
+                break;
+            case '/':
+                read();
+                switch (c) {
+                    case '*':
+                        writeMask('/');
+                        writeMask(c);
+                        int prevc = 0;
+                        while (read() >= 0 && (c != '/' || prevc != '*')) {
+                            writeMask(c);
+                            prevc = c;
+                        }
+                        writeMask(c);
+                        openComment = c < 0;
+                        break;
+                    case '/':
+                        writeMask('/');
+                        writeMask(c);
+                        while (read() >= 0 && c != '\n' && c != '\r') {
+                            writeMask(c);
+                        }
+                        writeMask(c);
+                        break;
+                    default:
+                        maskModifiers = false;
+                        write('/');
+                        unread();
+                        break;
+                }
+                break;
+            case '@':
+                do {
+                    write(c);
+                    read();
+                } while (Character.isJavaIdentifierPart(c));
+                while (Character.isWhitespace(c)) {
+                    write(c);
+                    read();
+                }
+                // if this is an annotation with arguments, process those recursively
+                if (c == '(') {
+                    write(c);
+                    boolean prevMaskModifiers = maskModifiers;
+                    int parenCnt = 1;
+                    while (read() >= 0) {
+                        if (c == ')') {
+                            if (--parenCnt == 0) {
+                                break;
+                            }
+                        } else if (c == '(') {
+                            ++parenCnt;
+                        }
+                        next(); // recurse to handle quotes and comments
+                    }
+                    write(c);
+                    // stuff in annotation arguments doesn't effect inside determination
+                    maskModifiers = prevMaskModifiers;
+                } else {
+                    unread();
+                }
+                break;
+            default:
+                if (Character.isJavaIdentifierStart(c)) {
+                    StringBuilder sb = new StringBuilder();
+                    do {
+                        writeTo(sb, c);
+                        read();
+                    } while (Character.isJavaIdentifierPart(c));
+                    unread();
+                    String id = sb.toString();
+                    if (maskModifiers && IGNORED_MODIFIERS.contains(id)) {
+                        writeMask(sb);
+                    } else {
+                        write(sb);
+                        if (maskModifiers && !OTHER_MODIFIERS.contains(id)) {
+                            maskModifiers = false;
+                        }
+                    }
+                } else {
+                    if (!Character.isWhitespace(c)) {
+                        maskModifiers = false;
+                    }
+                    write(c);
+                }
+                break;
         }
-
-        if (c < 0) {
-            return false;
-        }
-        write(c);
-        return true;
     }
 }
--- a/test/jdk/jshell/ClassMembersTest.java	Fri Nov 04 17:52:11 2016 +0000
+++ b/test/jdk/jshell/ClassMembersTest.java	Fri Nov 04 14:47:25 2016 -0700
@@ -38,6 +38,9 @@
 import jdk.jshell.SourceCodeAnalysis;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
+import jdk.jshell.TypeDeclSnippet;
+import static jdk.jshell.Snippet.Status.OVERWRITTEN;
+import static jdk.jshell.Snippet.Status.VALID;
 
 public class ClassMembersTest extends KullaTesting {
 
@@ -141,29 +144,36 @@
                 new ExpectedDiagnostic("compiler.err.non-static.cant.be.ref", 0, 8, 1, -1, -1, Diagnostic.Kind.ERROR));
     }
 
-    @Test(enabled = false) // TODO 8080354
-    public void annotationTest() {
+    @Test(dataProvider = "retentionPolicyTestCase")
+    public void annotationTest(RetentionPolicy policy) {
         assertEval("import java.lang.annotation.*;");
+        String annotationSource =
+                "@Retention(RetentionPolicy." + policy.toString() + ")\n" +
+                "@interface A {}";
+        assertEval(annotationSource);
+        String classSource =
+                "@A class C {\n" +
+                "   @A C() {}\n" +
+                "   @A void f() {}\n" +
+                "   @A int f;\n" +
+                "   @A class Inner {}\n" +
+                "}";
+        assertEval(classSource);
+        String isRuntimeVisible = policy == RetentionPolicy.RUNTIME ? "true" : "false";
+        assertEval("C.class.getAnnotationsByType(A.class).length > 0;", isRuntimeVisible);
+        assertEval("C.class.getDeclaredConstructor().getAnnotationsByType(A.class).length > 0;", isRuntimeVisible);
+        assertEval("C.class.getDeclaredMethod(\"f\").getAnnotationsByType(A.class).length > 0;", isRuntimeVisible);
+        assertEval("C.class.getDeclaredField(\"f\").getAnnotationsByType(A.class).length > 0;", isRuntimeVisible);
+        assertEval("C.Inner.class.getAnnotationsByType(A.class).length > 0;", isRuntimeVisible);
+    }
+
+    @DataProvider(name = "retentionPolicyTestCase")
+    public Object[][] retentionPolicyTestCaseGenerator() {
+        List<Object[]> list = new ArrayList<>();
         for (RetentionPolicy policy : RetentionPolicy.values()) {
-            String annotationSource =
-                    "@Retention(RetentionPolicy." + policy.toString() + ")\n" +
-                    "@interface A {}";
-            assertEval(annotationSource);
-            String classSource =
-                    "@A class C {\n" +
-                    "   @A C() {}\n" +
-                    "   @A void f() {}\n" +
-                    "   @A int f;\n" +
-                    "   @A class Inner {}\n" +
-                    "}";
-            assertEval(classSource);
-            String isRuntimeVisible = policy == RetentionPolicy.RUNTIME ? "true" : "false";
-            assertEval("C.class.getAnnotationsByType(A.class).length > 0;", isRuntimeVisible);
-            assertEval("C.class.getDeclaredConstructor().getAnnotationsByType(A.class).length > 0;", isRuntimeVisible);
-            assertEval("C.class.getDeclaredMethod(\"f\").getAnnotationsByType(A.class).length > 0;", isRuntimeVisible);
-            assertEval("C.class.getDeclaredField(\"f\").getAnnotationsByType(A.class).length > 0;", isRuntimeVisible);
-            assertEval("C.Inner.class.getAnnotationsByType(A.class).length > 0;", isRuntimeVisible);
+            list.add(new Object[]{policy});
         }
+        return list.toArray(new Object[list.size()][]);
     }
 
     @DataProvider(name = "memberTestCase")
--- a/test/jdk/jshell/ClassesTest.java	Fri Nov 04 17:52:11 2016 +0000
+++ b/test/jdk/jshell/ClassesTest.java	Fri Nov 04 14:47:25 2016 -0700
@@ -23,7 +23,7 @@
 
 /*
  * @test
- * @bug 8145239
+ * @bug 8145239 8129559 8080354
  * @summary Tests for EvaluationState.classes
  * @build KullaTesting TestingInputStream ExpectedDiagnostic
  * @run testng ClassesTest
@@ -255,6 +255,25 @@
         assertActiveKeys();
     }
 
+    public void classesIgnoredModifiersAnnotation() {
+        assertEval("public @interface X { }");
+        assertEval("@X public interface A { }");
+        assertDeclareWarn1("@X static class B implements A { }",
+                new ExpectedDiagnostic("jdk.eval.warn.illegal.modifiers", 0, 9, 0, -1, -1, Diagnostic.Kind.WARNING));
+        assertDeclareWarn1("@X final interface C extends A { }",
+                new ExpectedDiagnostic("jdk.eval.warn.illegal.modifiers", 0, 8, 0, -1, -1, Diagnostic.Kind.WARNING));
+        assertActiveKeys();
+    }
+
+    public void classesIgnoredModifiersOtherModifiers() {
+        assertEval("strictfp public interface A { }");
+        assertDeclareWarn1("strictfp static class B implements A { }",
+                new ExpectedDiagnostic("jdk.eval.warn.illegal.modifiers", 0, 15, 0, -1, -1, Diagnostic.Kind.WARNING));
+        assertDeclareWarn1("strictfp final interface C extends A { }",
+                new ExpectedDiagnostic("jdk.eval.warn.illegal.modifiers", 0, 14, 0, -1, -1, Diagnostic.Kind.WARNING));
+        assertActiveKeys();
+    }
+
     public void ignoreModifierSpaceIssue() {
         assertEval("interface I { void f(); } ");
         // there should not be a space between 'I' and '{' to reproduce the failure
--- a/test/jdk/jshell/CompletenessTest.java	Fri Nov 04 17:52:11 2016 +0000
+++ b/test/jdk/jshell/CompletenessTest.java	Fri Nov 04 14:47:25 2016 -0700
@@ -23,7 +23,7 @@
 
 /*
  * @test
- * @bug 8149524 8131024 8165211 8080071 8130454 8167343
+ * @bug 8149524 8131024 8165211 8080071 8130454 8167343 8129559
  * @summary Test SourceCodeAnalysis
  * @build KullaTesting TestingInputStream
  * @run testng CompletenessTest
@@ -176,6 +176,7 @@
         "@interface Anno",
         "void f()",
         "void f() throws E",
+        "@A(",
     };
 
     static final String[] unknown = new String[] {
--- a/test/jdk/jshell/IgnoreTest.java	Fri Nov 04 17:52:11 2016 +0000
+++ b/test/jdk/jshell/IgnoreTest.java	Fri Nov 04 14:47:25 2016 -0700
@@ -22,7 +22,7 @@
  */
 
 /*
- * @test
+ * @test 8129559
  * @summary Test the ignoring of comments and certain modifiers
  * @build KullaTesting TestingInputStream
  * @run testng IgnoreTest
@@ -70,6 +70,39 @@
         assertVariableDeclSnippet(x5, "x5", "int", VALID, VAR_DECLARATION_SUBKIND, 0, 1);
     }
 
+    public void testVarModifierAnnotation() {
+        assertEval("@interface A { int value() default 0; }");
+        VarSnippet x1 = varKey(assertEval("@A public int x1;"));
+        assertVariableDeclSnippet(x1, "x1", "int", VALID, VAR_DECLARATION_SUBKIND, 0, 0);
+        VarSnippet x2 = varKey(assertEval("@A(14) protected int x2;"));
+        assertVariableDeclSnippet(x2, "x2", "int", VALID, VAR_DECLARATION_SUBKIND, 0, 0);
+        VarSnippet x3 = varKey(assertEval("@A(value=111)private int x3;"));
+        assertVariableDeclSnippet(x3, "x3", "int", VALID, VAR_DECLARATION_SUBKIND, 0, 0);
+        VarSnippet x4 = (VarSnippet) assertDeclareWarn1("@A static int x4;", "jdk.eval.warn.illegal.modifiers");
+        assertVariableDeclSnippet(x4, "x4", "int", VALID, VAR_DECLARATION_SUBKIND, 0, 1);
+        VarSnippet x5 = (VarSnippet) assertDeclareWarn1("@A(1111) final int x5;", "jdk.eval.warn.illegal.modifiers");
+        assertVariableDeclSnippet(x5, "x5", "int", VALID, VAR_DECLARATION_SUBKIND, 0, 1);
+    }
+
+    public void testVarModifierOtherModifier() {
+        VarSnippet x1 = varKey(assertEval("volatile public int x1;"));
+        assertVariableDeclSnippet(x1, "x1", "int", VALID, VAR_DECLARATION_SUBKIND, 0, 0);
+        VarSnippet x2 = varKey(assertEval("transient protected int x2;"));
+        assertVariableDeclSnippet(x2, "x2", "int", VALID, VAR_DECLARATION_SUBKIND, 0, 0);
+        VarSnippet x3 = varKey(assertEval("transient private int x3;"));
+        assertVariableDeclSnippet(x3, "x3", "int", VALID, VAR_DECLARATION_SUBKIND, 0, 0);
+        VarSnippet x4 = (VarSnippet) assertDeclareWarn1("volatile static int x4;", "jdk.eval.warn.illegal.modifiers");
+        assertVariableDeclSnippet(x4, "x4", "int", VALID, VAR_DECLARATION_SUBKIND, 0, 1);
+        VarSnippet x5 = (VarSnippet) assertDeclareWarn1("transient final int x5;", "jdk.eval.warn.illegal.modifiers");
+        assertVariableDeclSnippet(x5, "x5", "int", VALID, VAR_DECLARATION_SUBKIND, 0, 1);
+    }
+
+    public void testMisplacedIgnoredModifier() {
+        assertEvalFail("int public y;");
+        assertEvalFail("String private x;");
+        assertEvalFail("(protected 34);");
+    }
+
     public void testMethodModifier() {
         MethodSnippet m4 = (MethodSnippet) assertDeclareWarn1("static void m4() {}", "jdk.eval.warn.illegal.modifiers");
         assertMethodDeclSnippet(m4, "m4", "()void", VALID, 0, 1);
@@ -77,6 +110,14 @@
         assertMethodDeclSnippet(m5, "m5", "()void", VALID, 0, 1);
     }
 
+    public void testMethodModifierAnnotation() {
+        assertEval("@interface A { int value() default 0; }");
+        MethodSnippet m4 = (MethodSnippet) assertDeclareWarn1("@A static void m4() {}", "jdk.eval.warn.illegal.modifiers");
+        assertMethodDeclSnippet(m4, "m4", "()void", VALID, 0, 1);
+        MethodSnippet m5 = (MethodSnippet) assertDeclareWarn1("@A(value=66)final void m5() {}", "jdk.eval.warn.illegal.modifiers");
+        assertMethodDeclSnippet(m5, "m5", "()void", VALID, 0, 1);
+    }
+
     public void testClassModifier() {
         TypeDeclSnippet c4 = (TypeDeclSnippet) assertDeclareWarn1("static class C4 {}", "jdk.eval.warn.illegal.modifiers");
         assertTypeDeclSnippet(c4, "C4", VALID, CLASS_SUBKIND, 0, 1);
--- a/test/jdk/jshell/ModifiersTest.java	Fri Nov 04 17:52:11 2016 +0000
+++ b/test/jdk/jshell/ModifiersTest.java	Fri Nov 04 14:47:25 2016 -0700
@@ -22,7 +22,7 @@
  */
 
 /*
- * @test 8167643
+ * @test 8167643 8129559
  * @summary Tests for modifiers
  * @build KullaTesting TestingInputStream ExpectedDiagnostic
  * @run testng ModifiersTest
@@ -31,6 +31,7 @@
 import java.util.ArrayList;
 import java.util.List;
 
+import java.util.function.Consumer;
 import javax.tools.Diagnostic;
 
 import org.testng.annotations.DataProvider;
@@ -45,42 +46,53 @@
         String[] ignoredModifiers = new String[] {
             "static", "final"
         };
+        String[] silentlyIgnoredModifiers = new String[] {
+            "public", "protected", "private"
+        };
+        String[] before = new String[] {
+            "strictfp", "abstract", "@X", "@X(value=9)"
+        };
+        String context = "@interface X { int value() default 0; }";
+        Consumer<String> eval = this::assertEval;
+        Consumer<String> evalWarn = s -> assertDeclareWarn1(s, "jdk.eval.warn.illegal.modifiers");
         for (String ignoredModifier : ignoredModifiers) {
             for (ClassType classType : ClassType.values()) {
-                testCases.add(new Object[] { ignoredModifier, classType });
+                testCases.add(new Object[] { ignoredModifier, classType, evalWarn, "", null });
+            }
+        }
+        for (String ignoredModifier : ignoredModifiers) {
+            for (String preface : before) {
+                testCases.add(new Object[] { ignoredModifier, ClassType.CLASS, evalWarn, preface, context});
+            }
+        }
+        for (String ignoredModifier : silentlyIgnoredModifiers) {
+            for (ClassType classType : ClassType.values()) {
+                testCases.add(new Object[] { ignoredModifier, classType, eval, "", null });
+            }
+        }
+        for (String ignoredModifier : silentlyIgnoredModifiers) {
+            for (String preface : before) {
+                testCases.add(new Object[] { ignoredModifier, ClassType.CLASS, eval, preface, context});
             }
         }
         return testCases.toArray(new Object[testCases.size()][]);
     }
 
     @Test(dataProvider = "ignoredModifiers")
-    public void ignoredModifiers(String modifier, ClassType classType) {
-        assertDeclareWarn1(
-                String.format("%s %s A {}", modifier, classType), "jdk.eval.warn.illegal.modifiers");
-        assertNumberOfActiveClasses(1);
-        assertClasses(clazz(classType, "A"));
-        assertActiveKeys();
-    }
-
-    @DataProvider(name = "silentlyIgnoredModifiers")
-    public Object[][] getSilentTestCases() {
-        List<Object[]> testCases = new ArrayList<>();
-        String[] ignoredModifiers = new String[] {
-            "public", "protected", "private"
-        };
-        for (String ignoredModifier : ignoredModifiers) {
-            for (ClassType classType : ClassType.values()) {
-                testCases.add(new Object[] { ignoredModifier, classType });
-            }
+    public void ignoredModifiers(String modifier, ClassType classType,
+            Consumer<String> eval, String preface, String context) {
+        if (context != null) {
+            assertEval(context);
         }
-        return testCases.toArray(new Object[testCases.size()][]);
-    }
-
-    @Test(dataProvider = "silentlyIgnoredModifiers")
-    public void silentlyIgnoredModifiers(String modifier, ClassType classType) {
-        assertEval(String.format("%s %s A {}", modifier, classType));
-        assertNumberOfActiveClasses(1);
-        assertClasses(clazz(classType, "A"));
+        String decl = String.format("%s %s %s A {}", preface, modifier, classType);
+        eval.accept(decl);
+        if (context != null) {
+            assertNumberOfActiveClasses(2);
+            assertClasses(clazz(ClassType.ANNOTATION, "X"), clazz(classType, "A"));
+        } else {
+            assertNumberOfActiveClasses(1);
+            assertClasses(clazz(classType, "A"));
+        }
         assertActiveKeys();
     }