Revision: 3876
Author: [email protected]
Date: Mon Nov 30 20:14:27 2009
Log: updates to the ancillary linter
http://code.google.com/p/google-caja/source/detail?r=3876
Modified:
/trunk/src/com/google/caja/ancillary/linter/LexicalScope.java
/trunk/src/com/google/caja/ancillary/linter/Linter.java
/trunk/src/com/google/caja/ancillary/linter/VariableLiveness.java
/trunk/tests/com/google/caja/ancillary/linter/LinterTest.java
/trunk/tests/com/google/caja/ancillary/linter/VariableLivenessTest.java
=======================================
--- /trunk/src/com/google/caja/ancillary/linter/LexicalScope.java Mon Oct
19 18:04:03 2009
+++ /trunk/src/com/google/caja/ancillary/linter/LexicalScope.java Mon Nov
30 20:14:27 2009
@@ -18,6 +18,9 @@
import com.google.caja.parser.js.CatchStmt;
import com.google.caja.parser.js.FunctionConstructor;
import com.google.caja.parser.js.WithStmt;
+import com.google.caja.util.Lists;
+
+import java.util.List;
/**
* A set of adjacent AST nodes that share a common ancestor which is in
the set.
@@ -54,11 +57,13 @@
final AncestorChain<?> root;
final LexicalScope parent;
final SymbolTable symbols;
+ final List<LexicalScope> innerScopes = Lists.newArrayList();
LexicalScope(AncestorChain<?> root, LexicalScope parent) {
this.root = root;
this.parent = parent;
this.symbols = new SymbolTable();
+ if (parent != null) { parent.innerScopes.add(this); }
}
LexicalScope declaringScope(String symbolName) {
=======================================
--- /trunk/src/com/google/caja/ancillary/linter/Linter.java Wed Nov 18
16:37:58 2009
+++ /trunk/src/com/google/caja/ancillary/linter/Linter.java Mon Nov 30
20:14:27 2009
@@ -18,7 +18,6 @@
import com.google.caja.lexer.InputSource;
import com.google.caja.lexer.JsLexer;
import com.google.caja.lexer.JsTokenQueue;
-import com.google.caja.lexer.JsTokenType;
import com.google.caja.lexer.ParseException;
import com.google.caja.lexer.Token;
import com.google.caja.lexer.TokenConsumer;
@@ -27,6 +26,7 @@
import com.google.caja.parser.ParserBase;
import com.google.caja.parser.js.Block;
import com.google.caja.parser.js.BreakStmt;
+import com.google.caja.parser.js.CatchStmt;
import com.google.caja.parser.js.ContinueStmt;
import com.google.caja.parser.js.Expression;
import com.google.caja.parser.js.ExpressionStmt;
@@ -39,6 +39,7 @@
import com.google.caja.parser.js.Loop;
import com.google.caja.parser.js.Operation;
import com.google.caja.parser.js.Operator;
+import com.google.caja.parser.js.OperatorCategory;
import com.google.caja.parser.js.Parser;
import com.google.caja.parser.js.Reference;
import com.google.caja.parser.js.ReturnStmt;
@@ -85,7 +86,7 @@
Map<InputSource, CharSequence> contentMap = Maps.newLinkedHashMap();
MessageQueue mq = new SimpleMessageQueue();
List<LintJob> lintJobs = parseInputs(inputs, contentMap, mc, mq);
- lint(lintJobs, mq);
+ lint(lintJobs, new Environment(Sets.<String>newHashSet()), mq);
if (ErrorReporter.reportErrors(contentMap, mc, mq, System.out)
.compareTo(MessageLevel.WARNING) < 0) {
// Touch the time-stamp file to make it clear that the inputs were
@@ -114,15 +115,8 @@
JsTokenQueue tq = new JsTokenQueue(new JsLexer(cp), src);
try {
if (tq.isEmpty()) { continue; }
- List<Token<JsTokenType>> tokens = tq.filteredTokens();
Parser p = new Parser(tq, mq);
- compUnits.add(
- new LintJob(
- src,
- parseIdentifierListFromComment("requires", tokens, mq),
- parseIdentifierListFromComment("provides", tokens, mq),
- parseIdentifierListFromComment("overrides", tokens, mq),
- p.parse()));
+ compUnits.add(makeLintJob(p.parse(), mq));
} catch (ParseException ex) {
ex.toMessageQueue(mq);
}
@@ -130,10 +124,21 @@
return compUnits;
}
- // Visible for testing
- static void lint(List<LintJob> jobs, MessageQueue mq) {
+ public static LintJob makeLintJob(Block program, MessageQueue mq) {
+ InputSource src = program.getFilePosition().source();
+ List<Token<?>> tokens = program.getComments();
+ return new LintJob(
+ src,
+ parseIdentifierListFromComment("requires", tokens, mq),
+ parseIdentifierListFromComment("provides", tokens, mq),
+ parseIdentifierListFromComment("overrides", tokens, mq),
+ program);
+ }
+
+ public static void lint(
+ List<LintJob> jobs, Environment env, MessageQueue mq) {
for (LintJob job : jobs) {
- lint(AncestorChain.instance(job.program),
+ lint(AncestorChain.instance(job.program), env,
// Anything defined by this file can be read by this file.
job.provides, job.requires, job.overrides, mq);
}
@@ -157,7 +162,7 @@
* @param mq receives messages about violations of canRead and canSet.
*/
private static void lint(
- AncestorChain<?> ac,
+ AncestorChain<?> ac, final Environment env,
Set<String> provides, final Set<String> requires,
final Set<String> overrides,
MessageQueue mq) {
@@ -186,7 +191,8 @@
containing.symbols.declare(fc.getIdentifierName(), scope.root);
}
} else if (scope.isGlobal()) {
- for (String symbolName : Sets.union(requires, overrides)) {
+ for (String symbolName : Sets.union(
+ env.outers, Sets.union(requires, overrides))) {
if (scope.symbols.getSymbol(symbolName) == null) {
scope.symbols.declare(symbolName, scope.root);
}
@@ -241,6 +247,8 @@
for (LexicalScope p = scope; (p = p.parent) != null;) {
SymbolTable.Symbol masked = p.symbols.getSymbol(symbolName);
if (masked != null) {
+ AncestorChain<?> firstMasked =
masked.getDeclarations().iterator()
+ .next();
mq.addMessage(
MessageType.MASKING_SYMBOL,
(scope.isCatchScope()
@@ -248,8 +256,7 @@
: MessageLevel.ERROR),
declarations.iterator().next().node.getFilePosition(),
MessagePart.Factory.valueOf(symbolName),
- masked.getDeclarations().iterator()
- .next().node.getFilePosition());
+ firstMasked.node.getFilePosition());
}
if (p.isFunctionScope()) { break; }
}
@@ -331,12 +338,13 @@
LexicalScope cscope =
ScopeAnalyzer.containingScopeForNode(use.ref.node);
LexicalScope subScopeOrigin = banned.get(Pair.pair(cscope,
symbolName));
if (subScopeOrigin != null) {
+ AncestorChain<?> firstDecl = subScopeOrigin.symbols
+ .getSymbol(symbolName).getDeclarations().iterator().next();
mq.addMessage(
LinterMessageType.OUT_OF_BLOCK_SCOPE,
use.ref.node.getFilePosition(),
MessagePart.Factory.valueOf(symbolName),
- (subScopeOrigin.symbols.getSymbol(symbolName).getDeclarations()
- .iterator().next().node.getFilePosition()));
+ firstDecl.node.getFilePosition());
continue;
}
LexicalScope dscope = cscope.declaringScope(symbolName);
@@ -380,17 +388,8 @@
}
// Check @provides and @overrides against the program's free variables.
- for (String symbolName : Sets.difference(
- globalScope.symbols.symbolNames(),
- Sets.union(provides, overrides))) {
- for (AncestorChain<?> decl
- : globalScope.symbols.getSymbol(symbolName).getDeclarations()) {
- if (decl == globalScope.root) { continue; } // a built-in
- mq.addMessage(
- MessageType.UNDOCUMENTED_GLOBAL, decl.node.getFilePosition(),
- MessagePart.Factory.valueOf(symbolName));
- }
- }
+ checkGlobalsDefined(
+ globalScope, globalScope, Sets.union(provides, overrides), mq);
for (String symbolName : Sets.difference(
globalsSet.keySet(), Sets.union(provides, overrides))) {
mq.addMessage(
@@ -408,9 +407,10 @@
// Check @requires are used
for (String symbolName : Sets.difference(requires,
globalsRead.keySet())) {
+ AncestorChain<?> root = globalScope.root;
mq.addMessage(
LinterMessageType.UNUSED_REQUIRE,
- globalScope.root.node.getFilePosition().source(),
+ root.node.getFilePosition().source(),
MessagePart.Factory.valueOf(symbolName));
}
// TODO(mikesamuel): check locals and formals used
@@ -418,9 +418,10 @@
// Check that @provides are provided
for (String symbolName : provides) {
if (!liveAtEnd.symbols.contains(Pair.pair(symbolName, globalScope)))
{
+ AncestorChain<?> root = globalScope.root;
mq.addMessage(
LinterMessageType.UNUSED_PROVIDE,
- globalScope.root.node.getFilePosition().source(),
+ root.node.getFilePosition().source(),
MessagePart.Factory.valueOf(symbolName));
}
}
@@ -466,7 +467,7 @@
}
/** Encapsulates information about a single input to the linter. */
- static final class LintJob { // Visible for testing
+ public static final class LintJob {
final InputSource src;
final Set<String> requires, provides, overrides;
final Block program;
@@ -481,6 +482,20 @@
}
}
+ public static final class Environment {
+ final Set<String> outers;
+
+ public Environment(Set<String> outers) {
+ this.outers = Sets.immutableSet(outers);
+ }
+ }
+
+ public static final Environment BROWSER_ENVIRONMENT = new Environment(
+ Sets.newLinkedHashSet(
+ "window", "document", "setTimeout", "setInterval", "location",
+ "XMLHttpRequest", "clearInterval", "clearTimeout", "navigator",
+ "event", "alert", "confirm", "prompt", "this"));
+
/**
* Find identifier lists in documentation comments.
* Annotations in documentation comments start with a '@' symbol
followed
@@ -489,11 +504,11 @@
* identifier list separated by spaces and/or commas.
*/
private static final Set<String> parseIdentifierListFromComment(
- String annotationName, List<Token<JsTokenType>> comments,
+ String annotationName, List<Token<?>> comments,
MessageQueue mq) {
// TODO(mikesamuel): replace with jsdoc comment parser
Set<String> idents = Sets.newLinkedHashSet();
- for (Token<JsTokenType> comment : comments) {
+ for (Token<?> comment : comments) {
// Remove line prefixes so they're not interpreted as significant in
the
// middle of an identifier list.
// And remove trailing content that is not whitespace or commas
@@ -567,7 +582,20 @@
Expression left = op.children().get(0);
return left instanceof Operation
&& Operator.CONSTRUCTOR == ((Operation) left).getOperator();
- default: return op.getOperator().getAssignmentDelegate() == null;
+ case LOGICAL_AND: case LOGICAL_OR:
+ return shouldBeEvaluatedForValue(op.children().get(1));
+ case TERNARY:
+ return shouldBeEvaluatedForValue(op.children().get(1))
+ && shouldBeEvaluatedForValue(op.children().get(2));
+ case COMMA:
+ // We do not allow comma, since bad things happen when commas are
used.
+ // Consider
+ // if (foo)
+ // return bar,
+ // baz();
+ // $FALL-THROUGH
+ default:
+ return op.getOperator().getCategory() !=
OperatorCategory.ASSIGNMENT;
}
}
@@ -605,4 +633,26 @@
File out = File.createTempFile(Linter.class.getSimpleName(), ".stamp");
(new Linter()).build(inputs, deps, out);
}
-}
+
+ private static void checkGlobalsDefined(
+ LexicalScope globalScope, LexicalScope scope,
+ Set<String> documentedGlobals, MessageQueue mq) {
+ for (String symbolName : Sets.difference(
+ scope.symbols.symbolNames(), documentedGlobals)) {
+ for (AncestorChain<?> decl
+ : scope.symbols.getSymbol(symbolName).getDeclarations()) {
+ if (decl == globalScope.root) { continue; } // a built-in
+ if (decl.parent.node instanceof CatchStmt) { continue; }
+ mq.addMessage(
+ MessageType.UNDOCUMENTED_GLOBAL,
+ decl.node.getFilePosition(),
+ MessagePart.Factory.valueOf(symbolName));
+ }
+ }
+ for (LexicalScope inner : scope.innerScopes) {
+ if (!inner.isFunctionScope()) {
+ checkGlobalsDefined(globalScope, inner, documentedGlobals, mq);
+ }
+ }
+ }
+}
=======================================
--- /trunk/src/com/google/caja/ancillary/linter/VariableLiveness.java Mon
Nov 16 17:29:55 2009
+++ /trunk/src/com/google/caja/ancillary/linter/VariableLiveness.java Mon
Nov 30 20:14:27 2009
@@ -24,6 +24,7 @@
import com.google.caja.parser.js.ContinueStmt;
import com.google.caja.parser.js.Declaration;
import com.google.caja.parser.js.DefaultCaseStmt;
+import com.google.caja.parser.js.DirectivePrologue;
import com.google.caja.parser.js.DoWhileLoop;
import com.google.caja.parser.js.Expression;
import com.google.caja.parser.js.ExpressionStmt;
@@ -47,7 +48,6 @@
import com.google.caja.parser.js.TryStmt;
import com.google.caja.parser.js.WhileLoop;
import com.google.caja.parser.js.WithStmt;
-import com.google.caja.parser.js.DirectivePrologue;
import com.google.caja.util.SyntheticAttributeKey;
import java.util.List;
@@ -209,8 +209,7 @@
} else if (s instanceof LabeledStmtWrapper) {
return processLabeledStmtWrapper((LabeledStmtWrapper) s, onEntry);
} else if (s instanceof DirectivePrologue) {
- // TODO: New node type, not yet supported by linter
- return processDirectivePrologue((DirectivePrologue) s, onEntry);
+ return processDirectivePrologue(onEntry);
} else {
throw new RuntimeException(s.getClass().getName());
}
@@ -675,10 +674,8 @@
return last;
}
- private static LiveCalc processDirectivePrologue(
- DirectivePrologue d, LiveSet onEntry) {
- // TODO: New node type, not yet supported by linter
- return processNoop(onEntry);
+ private static LiveCalc processDirectivePrologue(LiveSet onEntry) {
+ return new LiveCalc(onEntry, ExitModes.COMPLETES);
}
/**
=======================================
--- /trunk/tests/com/google/caja/ancillary/linter/LinterTest.java Wed Nov
18 16:37:58 2009
+++ /trunk/tests/com/google/caja/ancillary/linter/LinterTest.java Mon Nov
30 20:14:27 2009
@@ -58,6 +58,9 @@
.make()),
"ERROR: testProvides: @provides y not provided"
);
+ runLinterTest(
+ jobs(new LintJobMaker(js(fromString("for (var i;;) {}"))).make()),
+ "LINT: testProvides:1+6 - 11: Undocumented global i");
}
public final void testMultiplyProvidedSymbols() throws Exception {
@@ -288,7 +291,8 @@
);
runLinterTest(
jobs(new LintJobMaker(js(fromString("for (var k in o);"))).make()),
- "ERROR: testLoops:1+15 - 16: Symbol o has not been defined");
+ "ERROR: testLoops:1+15 - 16: Symbol o has not been defined",
+ "LINT: testLoops:1+6 - 11: Undocumented global k");
}
public final void testIgnoredValue() throws Exception {
@@ -298,7 +302,7 @@
+ "var c; f; \n" // Line 1
+ "+n; \n" // line 2
+ "a.b; \n" // line 3
- + "f && g(); \n" // line 4
+ + "f && g(); f && g; f() && g; \n" // line 4: 1st OK,
others not
+ "a = b, c = d; \n" // line 5
+ "a = b; \n" // OK
+ "m += n; \n" // OK
@@ -306,7 +310,8 @@
+ "new Array; \n" // line 9
+ "new Array(); \n" // line 10
+ "for (a = b, c = d; !a; ++a, --m, ++c) f; \n" // line 11
- + "while (1) { 1; }" // line 12. First allowed, second not
+ + "++c; \n" // OK
+ + "while (1) { 1; }" // line 13. First allowed, second not
// 1 2 3 4
// 1234567890123456789012345678901234567890
)))
@@ -317,12 +322,13 @@
"WARNING: testIgnoredValue:1+8 - 9: Operation has no effect",
"WARNING: testIgnoredValue:2+1 - 3: Operation has no effect",
"WARNING: testIgnoredValue:3+1 - 4: Operation has no effect",
- "WARNING: testIgnoredValue:4+1 - 9: Operation has no effect",
+ "WARNING: testIgnoredValue:4+12 - 18: Operation has no effect",
+ "WARNING: testIgnoredValue:4+21 - 29: Operation has no effect",
"WARNING: testIgnoredValue:5+1 - 13: Operation has no effect",
"WARNING: testIgnoredValue:9+1 - 10: Operation has no effect",
"WARNING: testIgnoredValue:10+1 - 12: Operation has no effect",
"WARNING: testIgnoredValue:11+39 - 40: Operation has no effect",
- "WARNING: testIgnoredValue:12+13 - 14: Operation has no effect");
+ "WARNING: testIgnoredValue:13+13 - 14: Operation has no effect");
}
public final void testDeadCode() throws Exception {
@@ -509,7 +515,7 @@
private void runLinterTest(List<Linter.LintJob> inputs, String...
messages) {
MessageQueue mq = new SimpleMessageQueue();
- Linter.lint(inputs, mq);
+ Linter.lint(inputs, new Linter.Environment(Sets.<String>newHashSet()),
mq);
List<String> actualMessageStrs = Lists.newArrayList();
for (Message msg : mq.getMessages()) {
actualMessageStrs.add(
=======================================
--- /trunk/tests/com/google/caja/ancillary/linter/VariableLivenessTest.java
Mon Oct 19 18:04:03 2009
+++ /trunk/tests/com/google/caja/ancillary/linter/VariableLivenessTest.java
Mon Nov 30 20:14:27 2009
@@ -1468,6 +1468,14 @@
"testLiveBreaks:27", formatShort(stmts.next().getFilePosition()));
assertFalse(stmts.hasNext());
}
+
+ public final void testDirectives() throws Exception {
+ assertLiveness(
+ js(fromString("\"use strict\";")),
+ "Block",
+ " DirectivePrologue",
+ " Directive : use strict");
+ }
private static ExitModes setup(ParseTreeNode js) {
ScopeAnalyzer sa = new ScopeAnalyzer();
