Merge branch 'pr-1849'

docs/pages/release_notes.md

@@ -16,6 +16,9 @@ This is a {{ site.pmd.release_type }} release.
 
 ### Fixed Issues
 
+*   java
+    *   [#1848](https://github.com/pmd/pmd/issues/1848): \[java] Local classes should preserve their modifiers
+
 ### API Changes
 
 #### Deprecated APIs

pmd-java/etc/grammar/Java.jjt

@@ -1,4 +1,7 @@
 /**
+ * Fix #1848 Local classes should preserve their modifiers
+ * Clément Fournier 05/2019
+ *====================================================================
  * Add support for Java 12 switch expressions and switch rules.
  * Andreas Dangel, Clément Fournier 03/2019
  *====================================================================
@@ -442,6 +445,17 @@ public class JavaParser {
     return getToken(1).kind == IDENTIFIER && getToken(1).image.equals(keyword);
   }
 
+  private boolean shouldStartStatementInSwitch() {
+    switch (getToken(1).kind) {
+        case _DEFAULT:
+        case CASE:
+        case RBRACE:
+            return false;
+        default:
+            return true;
+    }
+  }
+
   public Map<Integer, String> getSuppressMap() {
     return token_source.getSuppressMap();
   }
@@ -1698,8 +1712,7 @@ void ClassOrInterfaceDeclaration(int modifiers):
     inInterface = false;
 }
 {
-  ( /* See note about this optional final modifier in BlockStatement */
-    ["final"|"abstract"] "class" | "interface" { jjtThis.setInterface(); inInterface = true; } )
+  ( "class" | "interface" { jjtThis.setInterface(); inInterface = true; } )
   t=<IDENTIFIER> { checkForBadTypeIdentifierUsage(t.image); jjtThis.setImage(t.image); }
   [ TypeParameters() ]
   [ ExtendsList() ]
@@ -2423,13 +2436,26 @@ void BlockStatement():
 |
   Statement()
 |
-  /*
-  TODO: Seems like we should be discarding the "final"
-  after using it in the lookahead; I added a ["final|abstract"] inside
-  ClassOrInterfaceDeclaration, but that seems like a hack that
-  could break other things...
-  */
-  LOOKAHEAD( (Annotation())* ["final"|"abstract"] "class") (Annotation())* ClassOrInterfaceDeclaration(0)
+  // we don't need to lookahead further here
+  // the ambiguity between start of local class and local variable decl
+  // is already handled in the lookahead guarding LocalVariableDeclaration above.
+  LocalClassDecl()
+}
+
+void LocalClassDecl() #void:
+{int mods = 0;}
+{
+    // this preserves the modifiers of the local class.
+    // it allows for modifiers that are forbidden for local classes,
+    // but anyway we are *not* checking modifiers for incompatibilities
+    // anywhere else in this grammar (and indeed the production Modifiers
+    // accepts any modifier explicitly for the purpose of forgiving modifier errors,
+    // and reporting them later if needed --see its documentation).
+
+    // In particular, it unfortunately allows local class declarations to start
+    // with a "default" modifier, which introduces an ambiguity with default
+    // switch labels. This is guarded by a custom lookahead around SwitchLabel
+    mods=Modifiers() ClassOrInterfaceDeclaration(mods)
 }
 
 /*
@@ -2484,7 +2510,11 @@ void SwitchBlock() #void :
             (
                 "->" SwitchLabeledRulePart() (SwitchLabeledRule())*
             |
-                ":" (LOOKAHEAD(2) SwitchLabel() ":")* (BlockStatement())* (SwitchLabeledStatementGroup())*
+                ":" (LOOKAHEAD(2) SwitchLabel() ":")*
+                    // the lookahead is to prevent choosing BlockStatement when the token is "default",
+                    // which could happen as local class declarations accept the "default" modifier.
+                    (LOOKAHEAD({shouldStartStatementInSwitch()}) BlockStatement())*
+                    (SwitchLabeledStatementGroup())*
             )
         )?
     "}"
@@ -2512,7 +2542,10 @@ void SwitchLabeledRulePart() #void:
 void SwitchLabeledStatementGroup() #void:
 {}
 {
-    (LOOKAHEAD(2) SwitchLabel() ":")+ ( BlockStatement() )*
+    (LOOKAHEAD(2) SwitchLabel() ":")+
+        // the lookahead is to prevent choosing BlockStatement when the token is "default",
+        // which could happen as local class declarations accept the "default" modifier.
+        (LOOKAHEAD({shouldStartStatementInSwitch()}) BlockStatement() )*
 }
 
 void SwitchLabel() :

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTClassOrInterfaceDeclaration.java

@@ -55,6 +55,10 @@ public class ASTClassOrInterfaceDeclaration extends AbstractAnyTypeDeclaration {
         return visitor.visit(this, data);
     }
 
+    @Override
+    public boolean isPackagePrivate() {
+        return super.isPackagePrivate() && !isLocal();
+    }
 
     /**
      * Returns true if the class is declared inside a block other

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ASTClassOrInterfaceDeclarationTest.java

@@ -29,6 +29,12 @@ public class ASTClassOrInterfaceDeclarationTest {
     private static final String LOCAL_CLASS_IN_INITIALIZER
             = "class Foo { { class Local {} } }";
 
+    private static final String LOCAL_CLASS_WITH_MODIFIERS
+            = "class Foo { { abstract class Local {} } }";
+
+    private static final String LOCAL_CLASS_WITH_MIXED_MODIFIER_ANNOTATIONS
+            = "class Foo { { final @F class Local {} } }";
+
     private static final String LOCAL_CHILDREN_ARE_NOT_ALWAYS_LOCAL
             = "class Foo { { class Local { class Nested {} void bar() {class Local2 {}}}}}";
 
@@ -53,6 +59,44 @@ public class ASTClassOrInterfaceDeclarationTest {
     }
 
 
+
+    @Test
+    public void testLocalAbstractClass() {
+        List<ASTClassOrInterfaceDeclaration> classes = ParserTstUtil.getOrderedNodes(ASTClassOrInterfaceDeclaration.class, LOCAL_CLASS_WITH_MODIFIERS);
+        assertTrue(classes.size() == 2);
+
+        assertFalse("Local class false-positive", classes.get(0).isLocal());
+        assertTrue("Local class false-negative", classes.get(1).isLocal());
+        assertTrue("Local class should preserve its modifiers", classes.get(1).isAbstract());
+    }
+
+
+    @Test
+    public void testLocalClassWithMixedModifiers() {
+        List<ASTClassOrInterfaceDeclaration> classes = ParserTstUtil.getOrderedNodes(ASTClassOrInterfaceDeclaration.class, LOCAL_CLASS_WITH_MIXED_MODIFIER_ANNOTATIONS);
+        assertTrue(classes.size() == 2);
+
+        assertFalse("Local class false-positive", classes.get(0).isLocal());
+        assertTrue("Local class false-negative", classes.get(1).isLocal());
+        assertTrue("Local class should preserve its modifiers", classes.get(1).isFinal());
+    }
+
+
+
+    @Test
+    public void testLocalClassVisibility() {
+        List<ASTClassOrInterfaceDeclaration> classes = ParserTstUtil.getOrderedNodes(ASTClassOrInterfaceDeclaration.class, LOCAL_CLASS_WITH_MODIFIERS);
+        assertTrue(classes.size() == 2);
+
+        assertFalse("Local class false-positive", classes.get(0).isLocal());
+        assertTrue("Local class false-negative", classes.get(1).isLocal());
+        assertFalse("Local class is not public", classes.get(1).isPublic());
+        assertFalse("Local class is not private", classes.get(1).isPrivate());
+        assertFalse("Local class is not protected", classes.get(1).isProtected());
+        assertFalse("Local class is not package-private", classes.get(1).isPackagePrivate());
+    }
+
+
     @Test
     public void testNestedClassIsNotLocal() {
         List<ASTClassOrInterfaceDeclaration> classes = ParserTstUtil.getOrderedNodes(ASTClassOrInterfaceDeclaration.class, NESTED_CLASS_IS_NOT_LOCAL);