pmd: fix #1043 node.getEndLine() always returns 0 (ECMAscript)

* added commons-io dependency
 * added a SourceCodePositioner to calculate line/column from position

pmd/etc/changelog.txt

@@ -1,5 +1,6 @@
 ???? ??, 2012 - 5.0.2:
 
+Fixed bug 1043: node.getEndLine() always returns 0 (ECMAscript)
 Fixed bug 1044: Unknown option: -excludemarker
 Fixed bug 1047: False Positive in 'for' loops for LocalVariableCouldBeFinal in 5.0.1
 Fixed bug 1048: CommentContent Rule, String Index out of range Exception

pmd/pom.xml

@@ -605,6 +605,11 @@
             <artifactId>javacc</artifactId>
             <version>${javacc.version}</version>
         </dependency>
+        <dependency>
+            <groupId>commons-io</groupId>
+            <artifactId>commons-io</artifactId>
+            <version>2.2</version>
+        </dependency>
     </dependencies>
 
     <reporting>

pmd/src/main/java/net/sourceforge/pmd/lang/ast/AbstractNode.java

@@ -119,7 +119,7 @@ public abstract class AbstractNode implements Node {
 	    if (children != null && children.length > 0) {
 		return children[0].getBeginColumn();
 	    } else {
-		throw new RuntimeException("Unable to determine begining line of Node.");
+		throw new RuntimeException("Unable to determine beginning line of Node.");
 	    }
 	}
     }

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

@@ -7,18 +7,27 @@ import net.sourceforge.pmd.lang.ast.AbstractNode;
 
 import org.mozilla.javascript.ast.AstNode;
 
-public abstract class AbstractEcmascriptNode<T extends AstNode> extends AbstractNode implements EcmascriptNode {
+public abstract class AbstractEcmascriptNode<T extends AstNode> extends AbstractNode implements EcmascriptNode<T> {
 
     protected final T node;
 
     public AbstractEcmascriptNode(T node) {
 	super(node.getType());
 	this.node = node;
-	this.beginLine = node.getLineno() + 1;
+    }
-	this.beginLine = node.getLineno() + 1;
+
-	// TODO Implement positions, or figure out how to do begin/end lines/column
+    /* package private */
-	//this.beginPosition = node.getAbsolutePosition();
+    void calculateLineNumbers(SourceCodePositioner positioner) {
-	//this.endPosition = this.beginPosition + node.getLength();
+	int startOffset = node.getAbsolutePosition();
+	int endOffset = startOffset + node.getLength();
+
+	this.beginLine = positioner.lineNumberFromOffset(startOffset);
+	this.beginColumn = positioner.columnFromOffset(startOffset);
+	this.endLine = positioner.lineNumberFromOffset(endOffset);
+	this.endColumn = positioner.columnFromOffset(endOffset) - 1; // end column is inclusive
+	if (this.endColumn < 0) {
+	    this.endColumn = 0;
+	}
     }
 
     /**
@@ -34,7 +43,9 @@ public abstract class AbstractEcmascriptNode<T extends AstNode> extends Abstract
     public Object childrenAccept(EcmascriptParserVisitor visitor, Object data) {
 	if (children != null) {
 	    for (int i = 0; i < children.length; ++i) {
-		((EcmascriptNode) children[i]).jjtAccept(visitor, data);
+		@SuppressWarnings("unchecked") // we know that the children here are all EcmascriptNodes
+		EcmascriptNode<T> ecmascriptNode = (EcmascriptNode<T>) children[i];
+		ecmascriptNode.jjtAccept(visitor, data);
 	    }
 	}
 	return data;
@@ -52,11 +63,6 @@ public abstract class AbstractEcmascriptNode<T extends AstNode> extends Abstract
 	return node.hasSideEffects();
     }
 
-    @Override
-    public int getBeginColumn() {
-	return -1;
-    }
-
     @Override
     public String toString() {
 	return node.shortName();

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

@@ -11,6 +11,7 @@ import java.util.List;
 import net.sourceforge.pmd.lang.ast.ParseException;
 import net.sourceforge.pmd.lang.ecmascript.EcmascriptParserOptions;
 
+import org.apache.commons.io.IOUtils;
 import org.mozilla.javascript.CompilerEnvirons;
 import org.mozilla.javascript.Parser;
 import org.mozilla.javascript.ast.AstRoot;
@@ -24,7 +25,7 @@ public class EcmascriptParser {
 	this.parserOptions = parserOptions;
     }
 
-    protected AstRoot parseEcmascript(final Reader reader, final List<ParseProblem> parseProblems) throws ParseException {
+    protected AstRoot parseEcmascript(final String sourceCode, final List<ParseProblem> parseProblems) throws ParseException {
 	final CompilerEnvirons compilerEnvirons = new CompilerEnvirons();
 	compilerEnvirons.setRecordingComments(parserOptions.isRecordingComments());
 	compilerEnvirons.setRecordingLocalJsDocComments(parserOptions.isRecordingLocalJsDocComments());
@@ -35,23 +36,23 @@ public class EcmascriptParser {
 	// TODO We should do something with Rhino errors...
 	final ErrorCollector errorCollector = new ErrorCollector();
 	final Parser parser = new Parser(compilerEnvirons, errorCollector);
+	// TODO Fix hardcode
+	final String sourceURI = "unknown";
+	final int beginLineno = 1;
+	AstRoot astRoot = parser.parse(sourceCode, sourceURI, beginLineno);
+	parseProblems.addAll(errorCollector.getErrors());
+	return astRoot;
+    }
+
+    public EcmascriptNode<AstRoot> parse(final Reader reader) {
 	try {
-	    // TODO Fix hardcode
+	    final List<ParseProblem> parseProblems = new ArrayList<ParseProblem>();
-	    final String sourceURI = "unknown";
+	    final String sourceCode = IOUtils.toString(reader);
-	    // TODO Fix hardcode
+	    final AstRoot astRoot = parseEcmascript(sourceCode, parseProblems);
-	    final int lineno = 0;
+	    final EcmascriptTreeBuilder treeBuilder = new EcmascriptTreeBuilder(sourceCode, parseProblems);
-	    AstRoot astRoot = parser.parse(reader, sourceURI, lineno);
+	    return treeBuilder.build(astRoot);
-	    parseProblems.addAll(errorCollector.getErrors());
+	} catch (IOException e) {
-	    return astRoot;
-	} catch (final IOException e) {
 	    throw new ParseException(e);
 	}
     }
-
-    public EcmascriptNode parse(final Reader reader) {
-	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

@@ -65,9 +65,9 @@ import org.mozilla.javascript.ast.XmlExpression;
 import org.mozilla.javascript.ast.XmlMemberGet;
 import org.mozilla.javascript.ast.XmlString;
 
-public class EcmascriptTreeBuilder implements NodeVisitor {
+public final class EcmascriptTreeBuilder implements NodeVisitor {
 
-    protected static final Map<Class<? extends AstNode>, Constructor<? extends EcmascriptNode>> NODE_TYPE_TO_NODE_ADAPTER_TYPE = new HashMap<Class<? extends AstNode>, Constructor<? extends EcmascriptNode>>();
+    private static final Map<Class<? extends AstNode>, Constructor<? extends EcmascriptNode<?>>> NODE_TYPE_TO_NODE_ADAPTER_TYPE = new HashMap<Class<? extends AstNode>, Constructor<? extends EcmascriptNode<?>>>();
     static {
 	register(ArrayComprehension.class, ASTArrayComprehension.class);
 	register(ArrayComprehensionLoop.class, ASTArrayComprehensionLoop.class);
@@ -120,7 +120,7 @@ public class EcmascriptTreeBuilder implements NodeVisitor {
 	register(XmlString.class, ASTXmlString.class);
     }
 
-    protected static void register(Class<? extends AstNode> nodeType, Class<? extends EcmascriptNode> nodeAdapterType) {
+    private static <T extends AstNode> void register(Class<T> nodeType, Class<? extends EcmascriptNode<T>> nodeAdapterType) {
 	try {
 	    NODE_TYPE_TO_NODE_ADAPTER_TYPE.put(nodeType, nodeAdapterType.getConstructor(nodeType));
 	} catch (SecurityException e) {
@@ -139,13 +139,18 @@ public class EcmascriptTreeBuilder implements NodeVisitor {
     // The Rhino nodes with children to build.
     protected Stack<AstNode> parents = new Stack<AstNode>();
 
-    public EcmascriptTreeBuilder(List<ParseProblem> parseProblems) {
+    private final SourceCodePositioner sourceCodePositioner;
+
+    public EcmascriptTreeBuilder(String sourceCode, List<ParseProblem> parseProblems) {
+	this.sourceCodePositioner = new SourceCodePositioner(sourceCode);
 	this.parseProblems = parseProblems;
     }
 
-    protected EcmascriptNode createNodeAdapter(AstNode node) {
+    private <T extends AstNode> EcmascriptNode<T> createNodeAdapter(T node) {
 	try {
-	    Constructor<? extends EcmascriptNode> constructor = NODE_TYPE_TO_NODE_ADAPTER_TYPE.get(node.getClass());
+	    @SuppressWarnings("unchecked") // the register function makes sure only EcmascriptNode<T> can be added,
+	    // where T is "T extends AstNode".
+	    Constructor<? extends EcmascriptNode<T>> constructor = (Constructor<? extends EcmascriptNode<T>>) NODE_TYPE_TO_NODE_ADAPTER_TYPE.get(node.getClass());
 	    if (constructor == null) {
 		throw new IllegalArgumentException("There is no Node adapter class registered for the Node class: "
 			+ node.getClass());
@@ -160,8 +165,10 @@ public class EcmascriptTreeBuilder implements NodeVisitor {
 	}
     }
 
-    public EcmascriptNode build(AstNode astNode) {
+    public <T extends AstNode> EcmascriptNode<T> build(T astNode) {
-	EcmascriptNode node = buildInternal(astNode);
+	EcmascriptNode<T> node = buildInternal(astNode);
+
+	calculateLineNumbers(node);
 
 	// Set all the trailing comma nodes
 	for (TrailingCommaNode trailingCommaNode : parseProblemToNode.values()) {
@@ -171,9 +178,9 @@ public class EcmascriptTreeBuilder implements NodeVisitor {
 	return node;
     }
 
-    protected EcmascriptNode buildInternal(AstNode astNode) {
+    private <T extends AstNode> EcmascriptNode<T> buildInternal(T astNode) {
 	// Create a Node
-	EcmascriptNode node = createNodeAdapter(astNode);
+	EcmascriptNode<T> node = createNodeAdapter(astNode);
 
 	// Append to parent
 	Node parent = nodes.isEmpty() ? null : nodes.peek();
@@ -203,7 +210,7 @@ public class EcmascriptTreeBuilder implements NodeVisitor {
 	}
     }
 
-    private void handleParseProblems(EcmascriptNode node) {
+    private void handleParseProblems(EcmascriptNode<? extends AstNode> node) {
 	if (node instanceof TrailingCommaNode) {
 	    TrailingCommaNode trailingCommaNode = (TrailingCommaNode) node;
 	    int nodeStart = node.getNode().getAbsolutePosition();
@@ -216,7 +223,7 @@ public class EcmascriptTreeBuilder implements NodeVisitor {
 		    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);
+			EcmascriptNode<? extends AstNode> currentNode = (EcmascriptNode<? extends AstNode>) parseProblemToNode.get(parseProblem);
 			if (currentNode == null || node.getNode().getLength() < currentNode.getNode().getLength()) {
 			    parseProblemToNode.put(parseProblem, trailingCommaNode);
 			}
@@ -225,4 +232,15 @@ public class EcmascriptTreeBuilder implements NodeVisitor {
 	    }
 	}
     }
+
+    private void calculateLineNumbers(EcmascriptNode<?> node) {
+	EcmascriptParserVisitorAdapter visitor = new EcmascriptParserVisitorAdapter() {
+	    @Override
+	    public Object visit(EcmascriptNode node, Object data) {
+	        ((AbstractEcmascriptNode<?>)node).calculateLineNumbers(sourceCodePositioner);
+	        return super.visit(node, data); // also visit the children
+	    }
+	};
+	node.jjtAccept(visitor, null);
+    }
 }

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

@@ -0,0 +1,56 @@
+/**
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+package net.sourceforge.pmd.lang.ecmascript.ast;
+
+import java.util.Arrays;
+
+/**
+ * Calculates from an absolute offset in the source file the line/column coordinate.
+ * This is needed as Rhino only offers absolute positions for each node.
+ * 
+ * Idea from: http://code.google.com/p/closure-compiler/source/browse/trunk/src/com/google/javascript/jscomp/SourceFile.java
+ */
+public class SourceCodePositioner {
+
+    private int[] lineOffsets;
+    
+    public SourceCodePositioner(String sourceCode) {
+	analyzeLineOffsets(sourceCode);
+    }
+
+    private void analyzeLineOffsets(String sourceCode) {
+	String[] lines = sourceCode.split("\n");
+	
+	int startOffset = 0;
+	int lineNumber = 0;
+	
+	lineOffsets = new int[lines.length];
+	
+	for (String line : lines) {
+	    lineOffsets[lineNumber] = startOffset;
+	    lineNumber++;
+	    startOffset += line.length() + 1; // +1 for the "\n" character
+	}
+    }
+
+    public int lineNumberFromOffset(int offset) {
+	int search = Arrays.binarySearch(lineOffsets, offset);
+	int lineNumber;
+	if (search >= 0) {
+	    lineNumber = search;
+	} else {
+	    int insertionPoint = search;
+	    insertionPoint += 1;
+	    insertionPoint *= -1;
+	    lineNumber = insertionPoint - 1; // take the insertion point one before
+	}
+	return lineNumber + 1; // 1-based line numbers
+    }
+    
+    public int columnFromOffset(int offset) {
+	int lineNumber = lineNumberFromOffset(offset);
+	int columnOffset = offset - lineOffsets[lineNumber - 1];
+	return columnOffset + 1; // 1-based column offsets
+    }
+}

pmd/src/test/java/net/sourceforge/pmd/lang/ecmascript/ast/EcmascriptParserTest.java

@@ -0,0 +1,49 @@
+/**
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+package net.sourceforge.pmd.lang.ecmascript.ast;
+
+import static org.junit.Assert.assertEquals;
+
+import java.io.Reader;
+import java.io.StringReader;
+
+import net.sourceforge.pmd.PMD;
+import net.sourceforge.pmd.lang.ast.Node;
+import net.sourceforge.pmd.lang.ecmascript.EcmascriptParserOptions;
+
+import org.junit.Test;
+import org.mozilla.javascript.ast.AstRoot;
+
+public class EcmascriptParserTest {
+
+    @Test
+    public void testLineNumbers() {
+	
+	EcmascriptParser parser = new EcmascriptParser(new EcmascriptParserOptions());
+	Reader sourceCode = new StringReader(SOURCE_CODE);
+	EcmascriptNode<AstRoot> node = parser.parse(sourceCode);
+
+	assertEquals(1, node.getBeginLine());
+	assertEquals(1, node.getBeginColumn());
+	assertEquals(3, node.getEndLine());
+	assertEquals(1, node.getEndColumn());
+
+	Node child = node.getFirstChildOfType(ASTFunctionNode.class);
+	assertEquals(1, child.getBeginLine());
+	assertEquals(1, child.getBeginColumn());
+	assertEquals(3, child.getEndLine());
+	assertEquals(1, child.getEndColumn());
+
+	child = node.getFirstDescendantOfType(ASTFunctionCall.class);
+	assertEquals(2, child.getBeginLine());
+	assertEquals(3, child.getBeginColumn());
+	assertEquals(2, child.getEndLine());
+	assertEquals(16, child.getEndColumn());
+    }
+
+    private static final String SOURCE_CODE =
+	    "function a() {" + PMD.EOL
+	  + "  alert('hello');" + PMD.EOL
+	  + "}" + PMD.EOL;
+}

pmd/src/test/java/net/sourceforge/pmd/lang/ecmascript/ast/SourceCodePositionerTest.java

@@ -0,0 +1,36 @@
+/**
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+package net.sourceforge.pmd.lang.ecmascript.ast;
+
+import static org.junit.Assert.*;
+
+import org.junit.Test;
+
+public class SourceCodePositionerTest {
+
+    private final String SOURCE_CODE = "abcd\ndefghi\n\njklmn\nopq";
+
+    @Test
+    public void testLineNumberFromOffset() {
+	SourceCodePositioner positioner = new SourceCodePositioner(SOURCE_CODE);
+	
+	int offset;
+
+	offset = SOURCE_CODE.indexOf('a');
+	assertEquals(1, positioner.lineNumberFromOffset(offset));
+	assertEquals(1, positioner.columnFromOffset(offset));
+
+	offset = SOURCE_CODE.indexOf('b');
+	assertEquals(1, positioner.lineNumberFromOffset(offset));
+	assertEquals(2, positioner.columnFromOffset(offset));
+
+	offset = SOURCE_CODE.indexOf('e');
+	assertEquals(2, positioner.lineNumberFromOffset(offset));
+	assertEquals(2, positioner.columnFromOffset(offset));
+
+	offset = SOURCE_CODE.indexOf('q');
+	assertEquals(5, positioner.lineNumberFromOffset(offset));
+	assertEquals(3, positioner.columnFromOffset(offset));
+    }
+}