Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(532)

Issue 9674: A lint tool for javascript. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
17 years, 3 months ago by MikeSamuel
Modified:
16 years, 4 months ago
Reviewers:
metaweta
CC:
google-caja-discuss_googlegroups.com
Base URL:
http://google-caja.googlecode.com/svn/trunk/
Visibility:
Public.

Description

Adds Linter.java and a build rule that runs sanity checks on our client-side JS. The linter looks for @requires, @provides, and @overrides comment annotations in each JS file and comes up with a set of globals that the file is allowed to read/write. It then applies a ScopeAnalyzer to map nodes to the scopes that contain them. The ScopeAnalyzer has a few overrideable methods, and so can support a wide variety of scoping models including (1) ES3.1 scoping rules plus let and const (2) ES3 scoping (3) ES3 scoping with IE quirks (4) Block scoping as understood by the naive JS programmer. The scoping rules used by Linter are javascript standard, with one added constraint. Variables may not be re-declared, unless all re-declarations occur in mutually-disjoint loops. This catches errors like var i = 1; if (...) { ... for (var i = 0; ... ) // Deeply nested loop ... } return i; but allows the common idiom whereby loop iterators are reused within a loop for (var i = 0; ...) { ... } for (var i = 0; ...) { ... } This works because the below is banned for (var i = 0; ...) { .. .} return i; since a variable declared in a loop can only be used in the loop. Once scopes are assigned, a VariableLiveness instance walks the tree figuring out the conservative set of variables which might be live at any point in the program. The scoping and liveness data is used to check for common scoping and namespace violation bugs. The Linter also identifies expressions that are typically used as expressions but which appear in a statement context. So { a; +b; } is flagged because the operator + is normally evaluated for its value, but the value is ignored in this case. I included &&, ||, and ?: in the set of expressions typically evaluated for their value. So condition && side_effecting_expression(); should instead be written as if (condition) { side_effecting_expression(); } This change also includes cleanup and fixes of our supporting JS to make it lint clean, and adds an ANT build rule that runs the linter over our supporting JS when tests are built. Submitted @3808

Patch Set 1 #

Patch Set 2 : A lint tool for javascript. #

Patch Set 3 : A lint tool for javascript. #

Patch Set 4 : A lint tool for javascript. #

Patch Set 5 : A lint tool for javascript. #

Patch Set 6 : A lint tool for javascript. #

Patch Set 7 : A lint tool for javascript. #

Patch Set 8 : A lint tool for javascript. #

Patch Set 9 : A lint tool for javascript. #

Patch Set 10 : A lint tool for javascript. #

Patch Set 11 : A lint tool for javascript. #

Patch Set 12 : A lint tool for javascript. #

Patch Set 13 : A lint tool for javascript. #

Patch Set 14 : A lint tool for javascript. #

Patch Set 15 : A lint tool for javascript. #

Patch Set 16 : A lint tool for javascript. #

Patch Set 17 : A lint tool for javascript. #

Patch Set 18 : A lint tool for javascript. #

Patch Set 19 : A lint tool for javascript. #

Patch Set 20 : A lint tool for javascript. #

Patch Set 21 : A lint tool for javascript. #

Patch Set 22 : A lint tool for javascript. #

Patch Set 23 : A lint tool for javascript. #

Patch Set 24 : A lint tool for javascript. #

Patch Set 25 : A lint tool for javascript. #

Patch Set 26 : A lint tool for javascript. #

Patch Set 27 : A lint tool for javascript. #

Patch Set 28 : A lint tool for javascript. #

Patch Set 29 : A lint tool for javascript. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+4998 lines, -9 lines) Patch
M build.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 5 chunks +26 lines, -3 lines 0 comments Download
A src/com/google/caja/ancillary/linter/ErrorReporter.java View 1 chunk +141 lines, -0 lines 0 comments Download
A src/com/google/caja/ancillary/linter/ExitModes.java View 1 chunk +401 lines, -0 lines 0 comments Download
A src/com/google/caja/ancillary/linter/FileContent.java View 1 chunk +51 lines, -0 lines 0 comments Download
A src/com/google/caja/ancillary/linter/LexicalScope.java View 1 chunk +97 lines, -0 lines 0 comments Download
A src/com/google/caja/ancillary/linter/Linter.java View 1 chunk +609 lines, -0 lines 0 comments Download
A src/com/google/caja/ancillary/linter/LinterMessageType.java View 1 chunk +68 lines, -0 lines 0 comments Download
A src/com/google/caja/ancillary/linter/LiveSet.java View 1 chunk +187 lines, -0 lines 0 comments Download
A src/com/google/caja/ancillary/linter/NodeBuckets.java View 1 chunk +80 lines, -0 lines 0 comments Download
A src/com/google/caja/ancillary/linter/ScopeAnalyzer.java View 1 chunk +257 lines, -0 lines 0 comments Download
A src/com/google/caja/ancillary/linter/SymbolTable.java View 1 chunk +62 lines, -0 lines 0 comments Download
A src/com/google/caja/ancillary/linter/VariableLiveness.java View 1 chunk +769 lines, -0 lines 0 comments Download
A src/com/google/caja/ancillary/linter/package.html View 28 1 chunk +10 lines, -0 lines 0 comments Download
M src/com/google/caja/tools/GenRuleAntTask.java View 3 chunks +6 lines, -1 line 2 comments Download
M src/com/google/caja/tools/TestSummary.java View 2 chunks +7 lines, -5 lines 0 comments Download
A tests/com/google/caja/ancillary/linter/ErrorReporterTest.java View 1 chunk +84 lines, -0 lines 0 comments Download
A tests/com/google/caja/ancillary/linter/ExitModesTest.java View 1 chunk +112 lines, -0 lines 0 comments Download
A tests/com/google/caja/ancillary/linter/LinterTest.java View 1 chunk +528 lines, -0 lines 0 comments Download
A tests/com/google/caja/ancillary/linter/VariableLivenessTest.java View 1 chunk +1503 lines, -0 lines 0 comments Download

Messages

Total messages: 5
MikeSamuel
I discuss the reasons for the various warnings at http://groups.google.com/group/google-caja-discuss/web/lint-logs.html with a breakdown of example ...
17 years, 2 months ago (2008-12-30 09:42:05 UTC) #1
MikeSamuel
Please look at the build.xml and other non ancillary file changes.
16 years, 4 months ago (2009-10-19 23:49:39 UTC) #2
metaweta
LGTM
16 years, 4 months ago (2009-10-20 00:10:01 UTC) #3
metaweta
http://codereview.appspot.com/9674/diff/59239/60205 File src/com/google/caja/tools/GenRuleAntTask.java (right): http://codereview.appspot.com/9674/diff/59239/60205#newcode33 Line 33: private boolean unless = false; Unless what? This ...
16 years, 4 months ago (2009-10-20 00:10:24 UTC) #4
MikeSamuel
16 years, 4 months ago (2009-10-20 00:13:26 UTC) #5
http://codereview.appspot.com/9674/diff/59239/60205
File src/com/google/caja/tools/GenRuleAntTask.java (right):

http://codereview.appspot.com/9674/diff/59239/60205#newcode33
Line 33: private boolean unless = false;
On 2009/10/20 00:10:24, metaweta wrote:
> Unless what?  This seems strange to me.

See the unless="" attribute in build.xml at line 526
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b