Add ecmascript AvoidTrailingComma rule.

pmd/etc/changelog.txt

@@ -40,6 +40,10 @@ Fixed bug 3484404: Invalid NPath calculation in return statement. Thanks to Prab
 Improved JSP parser to be less strict with not valid XML documents (like HTML). Thanks to Victor Bucutea.
 Fixed bgastviewer not working. Thanks to Victor Bucutea.
 
+New Ecmascript rules:
+
+	Basic ruleset: AvoidTrailingComma
+
 May, 1, 2012 - 5.0.0:
 
 Fixed bug 3515487: Inconsistent reference to ruleset file in documentation

pmd/src/main/java/net/sourceforge/pmd/lang/ecmascript/ast/ASTArrayLiteral.java

@@ -5,7 +5,9 @@ package net.sourceforge.pmd.lang.ecmascript.ast;
 
 import org.mozilla.javascript.ast.ArrayLiteral;
 
-public class ASTArrayLiteral extends AbstractEcmascriptNode<ArrayLiteral> implements DestructuringNode {
+public class ASTArrayLiteral extends AbstractEcmascriptNode<ArrayLiteral> implements DestructuringNode, TrailingCommaNode {
+    private boolean trailingComma;
+
     public ASTArrayLiteral(ArrayLiteral arrayLiteral) {
 	super(arrayLiteral);
     }
@@ -20,4 +22,12 @@ public class ASTArrayLiteral extends AbstractEcmascriptNode<ArrayLiteral> implem
     public boolean isDestructuring() {
 	return node.isDestructuring();
     }
+
+    public boolean isTrailingComma() {
+	return trailingComma;
+    }
+
+    public void setTrailingComma(boolean trailingComma) {
+	this.trailingComma = trailingComma;
+    }
 }

pmd/src/main/java/net/sourceforge/pmd/lang/ecmascript/ast/ASTObjectLiteral.java

@@ -5,7 +5,9 @@ package net.sourceforge.pmd.lang.ecmascript.ast;
 
 import org.mozilla.javascript.ast.ObjectLiteral;
 
-public class ASTObjectLiteral extends AbstractEcmascriptNode<ObjectLiteral> implements DestructuringNode {
+public class ASTObjectLiteral extends AbstractEcmascriptNode<ObjectLiteral> implements DestructuringNode, TrailingCommaNode {
+    private boolean trailingComma;
+
     public ASTObjectLiteral(ObjectLiteral objectLiteral) {
 	super(objectLiteral);
     }
@@ -24,4 +26,12 @@ public class ASTObjectLiteral extends AbstractEcmascriptNode<ObjectLiteral> impl
     public boolean isDestructuring() {
 	return node.isDestructuring();
     }
+
+    public boolean isTrailingComma() {
+	return trailingComma;
+    }
+
+    public void setTrailingComma(boolean trailingComma) {
+	this.trailingComma = trailingComma;
+    }
 }

pmd/src/main/java/net/sourceforge/pmd/lang/ecmascript/ast/EcmascriptParser.java

@@ -5,52 +5,53 @@ package net.sourceforge.pmd.lang.ecmascript.ast;
 
 import java.io.IOException;
 import java.io.Reader;
-import java.util.HashMap;
-import java.util.Map;
+import java.util.ArrayList;
+import java.util.List;
 
 import net.sourceforge.pmd.lang.ast.ParseException;
 import net.sourceforge.pmd.lang.ecmascript.EcmascriptParserOptions;
 
 import org.mozilla.javascript.CompilerEnvirons;
-import org.mozilla.javascript.ErrorReporter;
 import org.mozilla.javascript.Parser;
-import org.mozilla.javascript.ast.AstNode;
 import org.mozilla.javascript.ast.AstRoot;
 import org.mozilla.javascript.ast.ErrorCollector;
+import org.mozilla.javascript.ast.ParseProblem;
 
 public class EcmascriptParser {
     protected final EcmascriptParserOptions parserOptions;
-    protected Map<AstNode, EcmascriptNode> nodeCache = new HashMap<AstNode, EcmascriptNode>();
 
     public EcmascriptParser(EcmascriptParserOptions parserOptions) {
 	this.parserOptions = parserOptions;
     }
 
-    protected AstRoot parseEcmascript(final Reader reader) throws ParseException {
+    protected AstRoot parseEcmascript(final Reader reader, final List<ParseProblem> parseProblems) throws ParseException {
 	final CompilerEnvirons compilerEnvirons = new CompilerEnvirons();
 	compilerEnvirons.setRecordingComments(parserOptions.isRecordingComments());
 	compilerEnvirons.setRecordingLocalJsDocComments(parserOptions.isRecordingLocalJsDocComments());
 	compilerEnvirons.setLanguageVersion(parserOptions.getRhinoLanguageVersion().getVersion());
 	compilerEnvirons.setIdeMode(true); // Scope's don't appear to get set right without this
+	compilerEnvirons.setWarnTrailingComma(true);
 
-	// TODO Fix hardcode
-	final ErrorReporter errorReporter = new ErrorCollector();
-	final Parser parser = new Parser(compilerEnvirons, errorReporter);
-	nodeCache.clear();
+	// TODO We should do something with Rhino errors...
+	final ErrorCollector errorCollector = new ErrorCollector();
+	final Parser parser = new Parser(compilerEnvirons, errorCollector);
 	try {
 	    // TODO Fix hardcode
 	    final String sourceURI = "unknown";
 	    // TODO Fix hardcode
 	    final int lineno = 0;
-	    return parser.parse(reader, sourceURI, lineno);
+	    AstRoot astRoot = parser.parse(reader, sourceURI, lineno);
+	    parseProblems.addAll(errorCollector.getErrors());
+	    return astRoot;
 	} catch (final IOException e) {
 	    throw new ParseException(e);
 	}
     }
 
     public EcmascriptNode parse(final Reader reader) {
-	final AstRoot astRoot = parseEcmascript(reader);
-	final EcmascriptTreeBuilder treeBuilder = new EcmascriptTreeBuilder();
+	final List<ParseProblem> parseProblems = new ArrayList<ParseProblem>();
+	final AstRoot astRoot = parseEcmascript(reader, parseProblems);
+	final EcmascriptTreeBuilder treeBuilder = new EcmascriptTreeBuilder(parseProblems);
 	return treeBuilder.build(astRoot);
     }
 }

pmd/src/main/java/net/sourceforge/pmd/lang/ecmascript/ast/EcmascriptTreeBuilder.java

@@ -6,6 +6,7 @@ package net.sourceforge.pmd.lang.ecmascript.ast;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.InvocationTargetException;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Stack;
 
@@ -44,6 +45,7 @@ import org.mozilla.javascript.ast.NumberLiteral;
 import org.mozilla.javascript.ast.ObjectLiteral;
 import org.mozilla.javascript.ast.ObjectProperty;
 import org.mozilla.javascript.ast.ParenthesizedExpression;
+import org.mozilla.javascript.ast.ParseProblem;
 import org.mozilla.javascript.ast.PropertyGet;
 import org.mozilla.javascript.ast.RegExpLiteral;
 import org.mozilla.javascript.ast.ReturnStatement;
@@ -128,12 +130,19 @@ public class EcmascriptTreeBuilder implements NodeVisitor {
 	}
     }
 
+    protected List<ParseProblem> parseProblems;
+    protected Map<ParseProblem, TrailingCommaNode> parseProblemToNode = new HashMap<ParseProblem, TrailingCommaNode>();
+
     // The nodes having children built.
     protected Stack<Node> nodes = new Stack<Node>();
 
     // The Rhino nodes with children to build.
     protected Stack<AstNode> parents = new Stack<AstNode>();
 
+    public EcmascriptTreeBuilder(List<ParseProblem> parseProblems) {
+	this.parseProblems = parseProblems;
+    }
+
     protected EcmascriptNode createNodeAdapter(AstNode node) {
 	try {
 	    Constructor<? extends EcmascriptNode> constructor = NODE_TYPE_TO_NODE_ADAPTER_TYPE.get(node.getClass());
@@ -152,6 +161,17 @@ public class EcmascriptTreeBuilder implements NodeVisitor {
     }
 
     public EcmascriptNode build(AstNode astNode) {
+	EcmascriptNode node = buildInternal(astNode);
+
+	// Set all the trailing comma nodes
+	for (TrailingCommaNode trailingCommaNode : parseProblemToNode.values()) {
+	    trailingCommaNode.setTrailingComma(true);
+	}
+
+	return node;
+    }
+
+    protected EcmascriptNode buildInternal(AstNode astNode) {
 	// Create a Node
 	EcmascriptNode node = createNodeAdapter(astNode);
 
@@ -161,6 +181,8 @@ public class EcmascriptTreeBuilder implements NodeVisitor {
 	    parent.jjtAddChild(node, parent.jjtGetNumChildren());
 	    node.jjtSetParent(parent);
 	}
+	
+	handleParseProblems(node);
 
 	// Build the children...
 	nodes.push(node);
@@ -176,8 +198,31 @@ public class EcmascriptTreeBuilder implements NodeVisitor {
 	if (parents.peek() == node) {
 	    return true;
 	} else {
-	    build(node);
+	    buildInternal(node);
 	    return false;
 	}
     }
+
+    private void handleParseProblems(EcmascriptNode node) {
+	if (node instanceof TrailingCommaNode) {
+	    TrailingCommaNode trailingCommaNode = (TrailingCommaNode) node;
+	    int nodeStart = node.getNode().getAbsolutePosition();
+	    int nodeEnd = nodeStart + node.getNode().getLength() - 1;
+	    for (ParseProblem parseProblem : parseProblems) {
+		// The node overlaps the comma (i.e. end of the problem)?
+		int problemStart = parseProblem.getFileOffset();
+		int commaPosition = parseProblem.getFileOffset() + parseProblem.getLength() - 1;
+		if (nodeStart <= commaPosition && commaPosition <= nodeEnd) {
+		    if ("Trailing comma is not legal in an ECMA-262 object initializer".equals(parseProblem.getMessage())) {
+			// Report on the shortest code block containing the
+			// problem (i.e. inner most code in nested structures).
+			EcmascriptNode currentNode = (EcmascriptNode) parseProblemToNode.get(parseProblem);
+			if (currentNode == null || node.getNode().getLength() < currentNode.getNode().getLength()) {
+			    parseProblemToNode.put(parseProblem, trailingCommaNode);
+			}
+		    }
+		}
+	    }
+	}
+    }
 }

pmd/src/main/java/net/sourceforge/pmd/lang/ecmascript/ast/TrailingCommaNode.java

@@ -0,0 +1,10 @@
+/**
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+package net.sourceforge.pmd.lang.ecmascript.ast;
+
+public interface TrailingCommaNode {
+    boolean isTrailingComma();
+
+    void setTrailingComma(boolean tailingComma);
+}

pmd/src/main/resources/rulesets/ecmascript/basic.xml

@@ -306,5 +306,40 @@ function(arg) {
         </example>
     </rule>
 
+    <rule name="AvoidTrailingComma"
+          message="Avoid trailing commas in object or array literals"
+          language="ecmascript"
+          since="5.1"
+          class="net.sourceforge.pmd.lang.rule.XPathRule"
+          externalInfoUrl="${pmd.website.baseurl}/rules/ecmascript/basic.html#AvoidTrailingComma">
+    <description>
+This rule helps improve code portability due to differences in browser treatment of trailing commas in object or array literals. 
+    </description>
+  <properties>
+    <property name="xpath">
+    <value>
+<![CDATA[
+	//ObjectLiteral[$allowObjectLiteral = "false" and @TrailingComma = 'true']
+|
+	//ArrayLiteral[$allowArrayLiteral = "false" and @TrailingComma = 'true']
+]]>
+    </value>
+    </property>
+    <property name="allowObjectLiteral" type="Boolean" value="false" description="Allow a trailing comma within an object literal" />
+    <property name="allowArrayLiteral" type="Boolean" value="false" description="Allow a trailing comma within an array literal" />
+    </properties>
+  <priority>1</priority>
+  <example>
+<![CDATA[
+function(arg) {
+    var obj1 = { a : 1 }; // Ok
+    var arr1 = [ 1, 2 ]; // Ok
+
+    var obj2 = { a : 1, }; // Syntax error in some browsers!
+    var arr2 = [ 1, 2, ]; // Length 2 or 3 depending on the browser!
+}
+]]>
+        </example>
+    </rule>
 
 </ruleset>

pmd/src/test/java/net/sourceforge/pmd/lang/ecmascript/rule/basic/BasicRulesTest.java

@@ -15,6 +15,7 @@ public class BasicRulesTest extends SimpleAggregatorTst {
     @Before
     public void setUp() {
 	addRule(RULESET, "AssignmentInOperand");
+	addRule(RULESET, "AvoidTrailingComma");
 	addRule(RULESET, "ConsistentReturn");
 	addRule(RULESET, "InnaccurateNumericLiteral");
 	addRule(RULESET, "ScopeForInVariable");

pmd/src/test/resources/net/sourceforge/pmd/lang/ecmascript/rule/basic/xml/AvoidTrailingComma.xml

@@ -0,0 +1,129 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<test-data>
+    <test-code>
+        <description><![CDATA[
+Ok, object literals
+     ]]></description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+x = {a : 1};
+x = {a : 1, b : 2}; 
+     ]]></code>
+        <source-type>ecmascript 3</source-type>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+Bad, object literals
+     ]]></description>
+        <expected-problems>2</expected-problems>
+        <code><![CDATA[
+x = {a : 1,};
+x = {a : 1, b : 2,}; 
+     ]]></code>
+        <source-type>ecmascript 3</source-type>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+Bad, object literals, multi-line nested
+     ]]></description>
+        <expected-problems>3</expected-problems>
+        <code><![CDATA[
+x = {
+	a : 1,
+	b : {
+		c : {
+			d: {
+			},
+		},
+	},
+};
+     ]]></code>
+        <source-type>ecmascript 3</source-type>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+Bad, object literals, compressed nested
+     ]]></description>
+        <expected-problems>3</expected-problems>
+        <code><![CDATA[
+x={a:1,b:{c:{d:{},},},};
+     ]]></code>
+        <source-type>ecmascript 3</source-type>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+Ok, allow object literals
+     ]]></description>
+        <expected-problems>0</expected-problems>
+        <rule-property name="allowObjectLiteral">true</rule-property>
+        <code><![CDATA[
+x = {,};
+x = {a : 1,};
+x = {a : 1, b : 2,}; 
+     ]]></code>
+        <source-type>ecmascript 3</source-type>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+Ok, array literals
+     ]]></description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+x = [];
+x = [1];
+x = [1, 2]; 
+     ]]></code>
+        <source-type>ecmascript 3</source-type>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+Bad, array literals
+     ]]></description>
+        <expected-problems>3</expected-problems>
+        <code><![CDATA[
+x = [,];
+x = [1,];
+x = [1,2,]; 
+     ]]></code>
+        <source-type>ecmascript 3</source-type>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+Bad, array literals, multi-line nested
+     ]]></description>
+        <expected-problems>4</expected-problems>
+        <code><![CDATA[
+x = [1,
+    [2,
+        [3,
+            [4,],
+        ],
+	],
+];
+     ]]></code>
+        <source-type>ecmascript 3</source-type>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+Bad, array literals, compressed nested
+     ]]></description>
+        <expected-problems>4</expected-problems>
+        <code><![CDATA[
+x=[1,[2,[3,[4,],],],];
+     ]]></code>
+        <source-type>ecmascript 3</source-type>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+Ok, allow array literals
+     ]]></description>
+        <expected-problems>0</expected-problems>
+        <rule-property name="allowArrayLiteral">true</rule-property>
+        <code><![CDATA[
+x = [,];
+x = [1,];
+x = [1,2,]; 
+     ]]></code>
+        <source-type>ecmascript 3</source-type>
+    </test-code>
+</test-data>