703355 - Static analysis on our JavaScript code

Status: RESOLVED DUPLICATE of bug 1308901

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[David Teller [:Yoric] - still alive but not very active]

At the moment, testing our JavaScript code is painful and error-prone. Simple misnamed variables can take hours to track down.

We can considerably improve the situation by introducing some optional JavaScript static analysis. A good first step would be to use either the Google Closure Compiler or pcwalton's offline type inference on the JS we ship with our products. This would certainly yield a number of false positives, but we could progressively refine the analysis either by careful scripting or by adapting the tool.

Comment 1

[Josh Matthews [:jdm]]

Dave might have input/experience here.

Comment 2

[Dave Herman [:dherman]]

IME, static analysis projects work best if you have concrete analyses in mind that you want to do. This could range from simple things, like "give me the set of globals created by this code", to complex things, like "infer types according to the following typed dialect." Especially for a student project, you want to think carefully about making the project concrete. I'd guess that my first example is too easy and my second example is very very hard -- but I have a PhD student intern starting in January who'll be working on a typed dialect of JS.

Dave

Comment 3

[Dave Herman [:dherman]]

It would also be interesting to think about analyses that look for UI jank. CC'ing Taras, who's interested in this problem. We could have a brainstorming session to see if there are common patterns (like accesses to potentially-expensive synchronous API's on the main thread) that would be easy to detect.

Dave

Comment 4

[Patrick Walton (:pcwalton)]

(In reply to Dave Herman [:dherman] from comment #3)
> It would also be interesting to think about analyses that look for UI jank.
> CC'ing Taras, who's interested in this problem. We could have a
> brainstorming session to see if there are common patterns (like accesses to
> potentially-expensive synchronous API's on the main thread) that would be
> easy to detect.

In the case of Places, I think we know where the I/O on the main thread is, we just have problems with not enough people to fix it and review bandwidth.

Comment 5

[(dormant account)]

indeed, static analysis is distinctly non-useful for finding existing perf problems. It's useful for preventing known problems from reappearing, but that's about it.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

(In reply to Dave Herman [:dherman] from comment #2)
> IME, static analysis projects work best if you have concrete analyses in
> mind that you want to do.

For the moment, I simply wish to aid with the first phases of debugging, i.e. catching what essentially amounts to unbound variables/fields/methods, functions/methods called with the wrong number of arguments. In other words, a form of type inference.

I would like to stress that I am not asking for the development of a new static analysis tool for JavaScript. For the moment, I would simply like to use existing tools. In a previous project, I have used the Google Closure Compiler as type-checker for JavaScript with great success, so, as a first experiment, I would simply go for the optional integration of the Closure Compiler in our build toolchain.

Once we have reached that stage, no matter how many false positives it yields, we can start thinking how to improve it and/or how to annotate our code to decrease the amount of false positives.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #5)
> indeed, static analysis is distinctly non-useful for finding existing perf
> problems. It's useful for preventing known problems from reappearing, but
> that's about it.

Well, we could certainly add an idl annotation "@expensive" and treat it as a computational effect in a static analysis, through both JS and C++. That might require a different bug, though.

Comment 8

[Dave Herman [:dherman]]

> For the moment, I simply wish to aid with the first phases of debugging,
> i.e. catching what essentially amounts to unbound variables/fields/methods,
> functions/methods called with the wrong number of arguments. In other words,
> a form of type inference.

That's a big goal. Type inference requires having an idea of the type system you're inferring. IMO the first step is to start designing the type system, and that's not easy. That's what our research intern Ravi will be working on starting in January.

> Once we have reached that stage, no matter how many false positives it
> yields, we can start thinking how to improve it and/or how to annotate our
> code to decrease the amount of false positives.

This doesn't sound like a good idea to me. Tying our codebase to an unspecified/unproven analysis is a recipe for slowing down our development process. By all means, experiment with bugfinding tools, but let's not start adding annotations to our codebase that will either get out of sync with the tool (generating lots of false positives) or require us to file bugs for fixing stale annotations as the analysis evolves.

Dave

Comment 9

[David Teller [:Yoric] - still alive but not very active]

(In reply to Dave Herman [:dherman] from comment #8)
> > For the moment, I simply wish to aid with the first phases of debugging,
> > i.e. catching what essentially amounts to unbound variables/fields/methods,
> > functions/methods called with the wrong number of arguments. In other words,
> > a form of type inference.
> 
> That's a big goal. Type inference requires having an idea of the type system
> you're inferring. IMO the first step is to start designing the type system,
> and that's not easy. That's what our research intern Ravi will be working on
> starting in January.

I know the difficulty pretty well. I did get my PhD with a few type systems of my own, after all :)

This is why I would like to use an existing tool, rather than spending 6 months to 10 years designing, implementing and testing a new one.

> > Once we have reached that stage, no matter how many false positives it
> > yields, we can start thinking how to improve it and/or how to annotate our
> > code to decrease the amount of false positives.
> 
> This doesn't sound like a good idea to me. Tying our codebase to an
> unspecified/unproven analysis is a recipe for slowing down our development
> process.

By "unproven", do you mean "no subject reduction" or "untested"? If the latter, I strongly doubt that we are going to find any testing tool better tested than the Google Closure Compiler before quite some time. If the former, well, although I know of one formally proved type system for a subset of JavaScript (JS0), I tend to doubt that we will see anything that works with our code before many years.

> By all means, experiment with bugfinding tools, but let's not start
> adding annotations to our codebase that will either get out of sync with the
> tool (generating lots of false positives) or require us to file bugs for
> fixing stale annotations as the analysis evolves.

I understand your point but I do not see how we can get away without annotations, regardless of the path we choose.

Comment 10

[(dormant account)]

I think all this can be achieved by much cheaper means of adding diagnostic information to spidermonkey. I do not see how teaching some 3rd party tool to analyze our js, integrating it, etc is a good use of time.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #10)
> I think all this can be achieved by much cheaper means of adding diagnostic
> information to spidermonkey.

Could you elaborate? I admit that I fail to see how.
(removing "mentored bug" and "student project" status for the moment)

Comment 12

[(dormant account)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #11)
> (In reply to Taras Glek (:taras) from comment #10)
> > I think all this can be achieved by much cheaper means of adding diagnostic
> > information to spidermonkey.
> 
> Could you elaborate? I admit that I fail to see how.
> (removing "mentored bug" and "student project" status for the moment)

some combination of existing type-inference features and javascript function-tracing apis.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

At this stage, we need input from the JS team.

Comment 14

[Benjamin Smedberg]

We have lots of past experience that annotations which aren't strictly enforced (via tinderbox failure) become useless very quickly. I'm all for experimentation too, but in order to impose annotation costs on everybody, you'd have to demonstrate some huge improvement in order for the cost/benefit to be worthwhile.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

In the case of the Closure Compiler, annotations are extremely lightweight – most of them are in fact trivial type annotations that merge in the JavaDoc style, which are useful even for human readers.

Can we at least agree that our JS Coding Guidelines allow developers to add these annotations to their code if they wish?

For reference, the format is documented here: http://code.google.com/closure/compiler/docs/js-for-compiler.html

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Well, by now, everybody but us is doing static analysis on JS code and there seems to be general agreement (at least from the people in charge of static analysis) that we want static analysis on our JS code.

Closing this bug to replace it by an actual battle plan.