SONARJS-786 Rule DuplicatePropertyName: support classes

javascript-checks/src/main/java/org/sonar/javascript/checks/DuplicatePropertyNameCheck.java

@@ -19,11 +19,16 @@
  */
 package org.sonar.javascript.checks;
 
-import java.util.HashMap;
-import java.util.Map;
+import com.google.common.collect.LinkedListMultimap;
+import com.google.common.collect.ListMultimap;
+import java.util.List;
+import javax.annotation.Nullable;
 import org.sonar.check.Rule;
 import org.sonar.plugins.javascript.api.tree.Tree;
 import org.sonar.plugins.javascript.api.tree.Tree.Kind;
+import org.sonar.plugins.javascript.api.tree.declaration.FieldDeclarationTree;
+import org.sonar.plugins.javascript.api.tree.declaration.MethodDeclarationTree;
+import org.sonar.plugins.javascript.api.tree.expression.ClassTree;
 import org.sonar.plugins.javascript.api.tree.expression.IdentifierTree;
 import org.sonar.plugins.javascript.api.tree.expression.LiteralTree;
 import org.sonar.plugins.javascript.api.tree.expression.ObjectLiteralTree;
@@ -37,45 +42,90 @@ public class DuplicatePropertyNameCheck extends DoubleDispatchVisitorCheck {
 
   @Override
   public void visitObjectLiteral(ObjectLiteralTree tree) {
-    Map<String, Tree> keys = new HashMap<>();
+    checkProperties(tree.properties());
+    super.visitObjectLiteral(tree);
+  }
 
-    for (Tree property : tree.properties()) {
-      if (property.is(Tree.Kind.PAIR_PROPERTY)) {
-        visitPairProperty(keys, (PairPropertyTree) property);
-      }
+  @Override
+  public void visitClass(ClassTree tree) {
+    checkProperties(tree.elements());
+    super.visitClass(tree);
+  }
+
+  private void checkProperties(List<Tree> properties) {
+    ListMultimap<String, Tree> keys = LinkedListMultimap.create();
 
-      if (property.is(Kind.IDENTIFIER_REFERENCE)) {
-        IdentifierTree identifier = (IdentifierTree) property;
-        addKey(keys, identifier.name(), (IdentifierTree)property);
+    for (Tree property : properties) {
+      Tree propertyNameTree = getPropertyNameTree(property);
+      if (propertyNameTree != null) {
+        String propertyName = getPropertyName(propertyNameTree);
+        if (propertyName != null) {
+          keys.put(EscapeUtils.unescape(propertyName), property);
+        }
       }
     }
-    super.visitObjectLiteral(tree);
+
+    checkKeys(keys);
   }
 
-  private void visitPairProperty(Map<String, Tree> keys, PairPropertyTree pairProperty) {
-    Tree key = pairProperty.key();
-    if (key.is(Tree.Kind.STRING_LITERAL)) {
-      String value = ((LiteralTree) key).value();
-      value = value.substring(1, value.length() - 1);
-      addKey(keys, value, pairProperty.key());
+  private void checkKeys(ListMultimap<String, Tree> keys) {
+    for (String key : keys.keySet()) {
+      List<Tree> properties = keys.get(key);
+      if (properties.size() > 1 && !getterSetter(properties)) {
+        Tree duplicatedProperty = getPropertyNameTree(properties.remove(0));
+
+        for (Tree property : properties) {
+          Tree propertyKey = getPropertyNameTree(property);
+          addIssue(propertyKey, String.format(MESSAGE, getPropertyName(propertyKey))).secondary(duplicatedProperty);
+        }
+
+      }
     }
+  }
 
-    if (key.is(Kind.IDENTIFIER_NAME)) {
-      addKey(keys, ((IdentifierTree) key).name(), pairProperty.key());
+  private static boolean getterSetter(List<Tree> value) {
+    if (value.size() == 2) {
+      return (value.get(0).is(Kind.GET_METHOD) && value.get(1).is(Kind.SET_METHOD))
+        || (value.get(1).is(Kind.GET_METHOD) && value.get(0).is(Kind.SET_METHOD));
     }
 
-    if (key.is(Tree.Kind.NUMERIC_LITERAL)) {
-      addKey(keys, ((LiteralTree) key).value(), pairProperty.key());
+    return false;
+  }
+
+  @Nullable
+  private static Tree getPropertyNameTree(Tree property) {
+    if (property instanceof MethodDeclarationTree) {
+      return ((MethodDeclarationTree) property).name();
+
+    } else if (property.is(Kind.FIELD)) {
+      return ((FieldDeclarationTree) property).propertyName();
+
+    } else if (property.is(Kind.PAIR_PROPERTY)) {
+      return ((PairPropertyTree) property).key();
+
+    } else if (property.is(Kind.IDENTIFIER_REFERENCE)) {
+      return property;
     }
+
+    return null;
   }
 
-  private void addKey(Map<String, Tree> keys, String key, Tree keyTree) {
-    Tree duplicated = keys.get(EscapeUtils.unescape(key));
-    if (duplicated != null) {
-      addIssue(keyTree, String.format(MESSAGE, key)).secondary(duplicated);
-    } else {
-      keys.put(key, keyTree);
+  @Nullable
+  private static String getPropertyName(Tree propertyKey) {
+    if (propertyKey.is(Tree.Kind.STRING_LITERAL)) {
+      String value = ((LiteralTree) propertyKey).value();
+      return value.substring(1, value.length() - 1);
     }
+
+    if (propertyKey.is(Kind.IDENTIFIER_NAME, Kind.IDENTIFIER_REFERENCE)) {
+      return ((IdentifierTree) propertyKey).name();
+    }
+
+    if (propertyKey.is(Tree.Kind.NUMERIC_LITERAL)) {
+      return ((LiteralTree) propertyKey).value();
+    }
+
+    return null;
   }
 
 }

javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/DuplicatePropertyName.html

@@ -1,7 +1,7 @@
-<p>JavaScript allows duplicate property names in object literals, but only the last instance of a duplicated property determines the actual value that
-will be used for it. Therefore, changing values of other occurrences of a duplicated property will have no effect and may cause misunderstandings and
-bugs.</p>
-<p>What's more, using duplicate property names will generate an error in JavaScript strict mode code.</p>
+<p>JavaScript allows duplicate property names in classes and object literals, but only the last instance of a duplicated name determines the actual
+value that will be used for it. Therefore, changing values of other occurrences of a duplicated name will have no effect and may cause
+misunderstandings and bugs.</p>
+<p>What's more, using duplicate names will generate an error in JavaScript strict mode code.</p>
 <h2>Noncompliant Code Example</h2>
 <pre>
 var data = {

javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/DuplicatePropertyName.json

@@ -1,5 +1,5 @@
 {
-  "title": "Property names should not be duplicated within an object literal",
+  "title": "Property names should not be duplicated within a class or object literal ",
   "type": "BUG",
   "status": "ready",
   "remediation": {

javascript-checks/src/test/resources/checks/duplicatePropertyName.js

@@ -15,7 +15,37 @@ var data = {
   key,                 // Noncompliant
 //^^^
 
-  get key() {        // OK - accessor
+  get key() {        // Noncompliant
     return this.key;
-  }
+  },
+
+  set key(p) {      // Noncompliant
+  },
+
+  key() {},  // Noncompliant
+  ["computedProperty"] : 1
+}
+
+class A {
+  prop;
+  prop1;
+  ;
+  prop = 1;        // Noncompliant
+  static prop = 1; // Noncompliant
+  prop() {}        // Noncompliant
+  * prop() {}      // Noncompliant
+  set prop(p) {}   // Noncompliant
+  get prop() {}    // Noncompliant
+
+  set prop2(p) {}
+  prop2;           // Noncompliant
+
+  get prop3() {}
+  prop3;            // Noncompliant
+
+  get prop4() {}
+  set prop4(p) {}    // OK
+
+  set prop5(p) {}    // OK
+  get prop5() {}
 }

javascript-frontend/src/main/java/org/sonar/plugins/javascript/api/tree/expression/ClassTree.java

@@ -55,7 +55,7 @@ public interface ClassTree extends ExpressionTree, StatementTree {
 
   SyntaxToken openCurlyBraceToken();
 
-  // elements can be either method declarations or semi-colons
+  // elements can be either method declarations, class field declarations or semi-colons
   List<Tree> elements();
 
   Iterable<MethodDeclarationTree> methods();

sonar-javascript-plugin/pom.xml

@@ -109,8 +109,8 @@
             <configuration>
               <rules>
                 <requireFilesSize>
-                  <minsize>2800000</minsize>
-                  <maxsize>2900000</maxsize>
+                  <minsize>2900000</minsize>
+                  <maxsize>3000000</maxsize>
                   <files>
                     <file>${project.build.directory}/${project.build.finalName}.jar</file>
                   </files>