First stage of replacing Scope with a more flexible API

This is part 5 of https://groups.google.com/group/google-caja-discuss/browse_thread/thread/50db6111ae71c2c9

We currently have a Scope class which was defined for the Rewriter
scheme.  For each optimization pass I've done, I've ended up writing
my own scope scheme, often on top of Scope.  E.g. the AlphaRenaming
uses an identity map to associate extra info with scopes, and the
array index optimization builds it's own scope tree, so that it can
easily walk from a scope to its children.  The linter uses a scoping
scheme that abstracts away the decision of where declarations are
introduced into scopes and which scopes they are hoisted to, so that
it can make sure that scoping is consistent between JScript and other
interpreters.

Further changelists will unify these scoping schemes and support the
following use cases:
   # In a rewriter to scope lazily
   # On an entire tree to attach a scope to each node
   # To generate masking errors about violations of Cajita specific
     invariants (such as forbidding declaration of the name "cajita")
   # To identify the kind of symbol -- function declaration, local
     function name, exception, data, globals
   # To collect information about declarations
   # To resolve references to declaration points or to scopes
   # To detect redelcaration
   # To emulate ES5 scoping rules
   # To emulate broken IE scoping
   # To collect uses of a variable
   # To determine whether/where a symbol is used as a LeftHandSideExpression
   # To collect uses in narrower scopes.
   # To collect the set of symbols from wider scopes.

There are a few separable concerns here:
   # Applying scoping rules of a given variant of JS -- exception
     hoisting, whether function constructors' names intrude on the
     containing scope to introduce scopes, and collect declarations
   # Associating scopes with parse tree nodes Collecting usage
     information

Please see ScopeListener for a general interface to collecting and
collating Scope information.  See ScopeAnalyzer for a general
implementation that implements a scoping scheme, and its concrete
implementations: ES5ScopeAnalyzer, JScriptScopeAnalyzer,
WorstCaseScopeAnalyzer.

The ScopeAnalyzerTests give an idea of how ScopeAnalyzer works.

In later CLS,  Scope has been rewritten to use ES5ScopeAnalyzer under
the hood, and Rewriter will change to create the scopes so that it is
no longer the responsibility of Rules to create and pass around scopes.


Submitted @3849

[MikeSamuel, 2009-11-06 22:54:05]

On 2009/11/04 20:04:42, MikeSamuel wrote:
> 

ping

[ihab.awad, 2009-11-10 00:39:07]

Some detailed comments inline, but general comments below. I have not finished
going through ScopeAnalyzer.java yet. The broad questions are:

1. ScopeAnalyzer is parameterized by its scope implementation type, <S extends
AbstractScope>. Why should it constrain S to extend AbstractScope given that it
otherwise makes no demands on S? By firing enterScope and exitScope on S, it
requires the listener's type S to be an AbstractScope, which the listener may
not want.

2. That said, it's probably perfectly reasonable that most listeners will
construct *some* S that is an AbstractScope. So then there's the opposite
question. The ScopeAnalyzer is creating all this nifty richly connected graph
with the Symbol and ScopeTree objects. But, alack! -- the listener is forbidden
from seeing any of this. It gets created and thrown away. Is there benefit to
making this an output of the ScopeAnalyzer?

I guess it just seems to me that there are 2 easily understandable choices --
complete separation or complete connection between Analyzer and its listeners. 
The current code seems to be a bit of a mixture.

http://codereview.appspot.com/148045/diff/1/11
File src/com/google/caja/parser/js/scope/AbstractScope.java (right):

http://codereview.appspot.com/148045/diff/1/11#newcode22
src/com/google/caja/parser/js/scope/AbstractScope.java:22: public interface
AbstractScope {
If we *are* going to define such a beast, should it not also include a slot for
a pointer to the ParseTreeNode from which it is generated?

http://codereview.appspot.com/148045/diff/1/11#newcode23
src/com/google/caja/parser/js/scope/AbstractScope.java:23: /**
Blank line before "/*" - all in this file.

http://codereview.appspot.com/148045/diff/1/6
File src/com/google/caja/parser/js/scope/ScopeAnalyzer.java (right):

http://codereview.appspot.com/148045/diff/1/6#newcode93
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:93: protected abstract
boolean fnCtorsDeclareInBody();
Should this simply be a boolean parameter to the ScopeAnalyzer ctor?

http://codereview.appspot.com/148045/diff/1/6#newcode98
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:98: protected abstract
boolean fnCtorsDeclareInContaining();
Same.

http://codereview.appspot.com/148045/diff/1/7
File src/com/google/caja/parser/js/scope/ScopeListener.java (right):

http://codereview.appspot.com/148045/diff/1/7#newcode20
src/com/google/caja/parser/js/scope/ScopeListener.java:20: public interface
ScopeListener<S extends AbstractScope> {
This interface reads a little confusingly to me. Is type S the type of scopes
being constructed by the listener? If not, then it must be the scopes
constructed by the scope analyzer, right? If so, then the type of *these* scopes
is fixed and it need not be a type parameter.

To clear this up and separate concerns more crisply, I think you could get away
with not introducing a Scope in the signature of ScopeListener at all if your
callbacks refer, not to a Scope, but to the ParseTreeNode corresponding to the
Scope.

This change would not cause more objects to be created or more code to be run.
The objects of type S being given to this listener are *still* being created by
the scope analyzer during its tree walk, and the listener is *still* creating
its own custom scopes if it wants to.

http://codereview.appspot.com/148045/diff/1/7#newcode29
src/com/google/caja/parser/js/scope/ScopeListener.java:29: void
declaration(AncestorChain<Identifier> id, S scope);
For example:

  declaration(..., ParseTreeNode node)

where the node would be (say) a FunctionConstructor, a CatchStmt, an
UncajoledModule, .... In other words, some node that *introduces* a new scope,
but not the scope itself (building that is the job of the listener, if it wants
to).

http://codereview.appspot.com/148045/diff/1/9
File src/com/google/caja/parser/js/scope/ScopeType.java (right):

http://codereview.appspot.com/148045/diff/1/9#newcode34
src/com/google/caja/parser/js/scope/ScopeType.java:34: /**
More spaces before "/*"

[ihab.awad, 2009-11-17 06:14:57]

LGTM++

The one comment I am really wondering about is why ScopeAnalyzer is
parameterized on <S extends AbstractScope> instead of just unbounded <S>.

http://codereview.appspot.com/148045/diff/1/6
File src/com/google/caja/parser/js/scope/ScopeAnalyzer.java (right):

http://codereview.appspot.com/148045/diff/1/6#newcode53
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:53: *       that is a
subtree of the tree to which the former scope corresponds.
glorpf! 

Narrower Scope: Scope S2 is narrower than scope S1 if the AST of S2 is a subtree
of the AST of S1.

Wider Scope: The inverse of Narrower Scope.

http://codereview.appspot.com/148045/diff/1/6#newcode70
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:70: *       first appears
in a scope that is wholly contained in the other's scope
s/contained in/narrower than/

http://codereview.appspot.com/148045/diff/1/6#newcode72
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:72: *       wider scope. 
E.g. In <code>var x; function (x) { ... x ... }}</code>
{@code ... } ?

http://codereview.appspot.com/148045/diff/1/6#newcode81
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:81: public abstract class
ScopeAnalyzer<S extends AbstractScope> {
So I guess my remaining concern about coupling of ScopeAnalyzer to other classes
is that I think type parameter S should be unbounded. There's good reason to
provide AbstractScope, but S here need not be limited to it.

http://codereview.appspot.com/148045/diff/1/6#newcode172
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:172: s.declared =
Sets.newHashSet();
See comment below about why ScopeTree.declared not initialized statically along
with the other instance variables.

http://codereview.appspot.com/148045/diff/1/6#newcode176
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:176: for (Symbol<S> use :
s.uses) { handleUse(use.id, use.useScope); }
Can this line be moved up one, thus making the traversal a pure preorder (do
this scope stuff, then recurse)?

I believe the answer is "no" because the recursively invoked declare(...) may
hoist() up to this scope, and thus modify it, which would affect the behavior of
handleUse(...). 

If, as I suspect, the order is important in this way, I think a brief comment
about why publishEvents(inner) needs to be done *before* handleUse(...) would
help.

http://codereview.appspot.com/148045/diff/1/6#newcode196
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:196: } else {  //
Declaration
This should be deliberate:

  } else if (n instanceof Declaration) {
  } else {
    // Note about why this cannot happen
    // Perhaps throw internal error or something?
  }

Also, it turns out that the Declaration case only happens when you are
traversing through a foreach loop; it's not clear in this code that that's
indeed what's going on. I had a few "huh?" moments before I realized there was
trickery afoot.

http://codereview.appspot.com/148045/diff/1/6#newcode207
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:207: private static
Operator lhsOperator(AncestorChain<Identifier> ac) {
Maybe rename this function to say something about "assignment" because
"lhsOperator" by itself caused me heartburn before I figured out what was really
going on.

http://codereview.appspot.com/148045/diff/1/6#newcode235
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:235: if
("this".equals(symbolName)) {
Use pre-defined constant for "this".

http://codereview.appspot.com/148045/diff/1/6#newcode243
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:243: } else if
("arguments".equals(symbolName)) {
Use pre-defined constant for "arguments".

http://codereview.appspot.com/148045/diff/1/6#newcode301
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:301: * @param the scope
in which the identifier appears.
@param >>scope<< the scope ...

http://codereview.appspot.com/148045/diff/1/6#newcode353
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:353: private static final
class ScopeTree<S> {
Please add some quick description of the instance variables of this class. Some
are confusing on first reading (what is the difference between "declarations"
and "declared"? what is in "uses"? what exactly lives in "inScope").

http://codereview.appspot.com/148045/diff/1/6#newcode361
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:361: Set<String>
declared;
Why is this not initialized statically like the other instance variables?

http://codereview.appspot.com/148045/diff/1/2
File tests/com/google/caja/parser/js/scope/ScopeAnalyzerTest.java (right):

http://codereview.appspot.com/148045/diff/1/2#newcode134
tests/com/google/caja/parser/js/scope/ScopeAnalyzerTest.java:134: " 
alert('stopped at ' + i);",
Do you also want to read 'e' here just to make sure it hits the correct scope,
or would that be redundant and repetitive and repetitious?

http://codereview.appspot.com/148045/diff/1/2#newcode245
tests/com/google/caja/parser/js/scope/ScopeAnalyzerTest.java:245: "  var
arguments = (function (arguments) { return arguments[z]; })();",
Could this be broken up into 3 test cases, namely, this:

  var arguments = (function(x) { ... })();

and this:

  var x = (function(arguments) { ... })();

and finally this:

  var arguments = (function(arguments) { ... })();

?

[MikeSamuel, 2009-11-17 18:14:23]

On 2009/11/17 06:14:57, ihab.awad wrote:
> LGTM++
> 
> The one comment I am really wondering about is why ScopeAnalyzer is
> parameterized on <S extends AbstractScope> instead of just unbounded <S>.

In
https://groups.google.com/group/google-caja-discuss/browse_thread/thread/9317...
I argued that I think at least a placeholder type is at least useful, though we
can get as simple or rich as we like.  Are you imagining some use case where a
placeholder type gets in the way?

[MikeSamuel, 2009-11-17 18:14:27]

On 2009/11/17 06:14:57, ihab.awad wrote:
> LGTM++
> 
> The one comment I am really wondering about is why ScopeAnalyzer is
> parameterized on <S extends AbstractScope> instead of just unbounded <S>.

In
https://groups.google.com/group/google-caja-discuss/browse_thread/thread/9317...
I argued that I think at least a placeholder type is at least useful, though we
can get as simple or rich as we like.  Are you imagining some use case where a
placeholder type gets in the way?

[MikeSamuel, 2009-11-17 19:26:17]

http://codereview.appspot.com/148045/diff/1/11
File src/com/google/caja/parser/js/scope/AbstractScope.java (right):

http://codereview.appspot.com/148045/diff/1/11#newcode22
src/com/google/caja/parser/js/scope/AbstractScope.java:22: public interface
AbstractScope {
On 2009/11/10 00:39:07, ihab.awad wrote:
> If we *are* going to define such a beast, should it not also include a slot
for
> a pointer to the ParseTreeNode from which it is generated?

We can get as complex or as simple in this class as we like.

http://codereview.appspot.com/148045/diff/1/11#newcode23
src/com/google/caja/parser/js/scope/AbstractScope.java:23: /**
On 2009/11/10 00:39:07, ihab.awad wrote:
> Blank line before "/*" - all in this file.

Done.

http://codereview.appspot.com/148045/diff/1/6
File src/com/google/caja/parser/js/scope/ScopeAnalyzer.java (right):

http://codereview.appspot.com/148045/diff/1/6#newcode53
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:53: *       that is a
subtree of the tree to which the former scope corresponds.
On 2009/11/17 06:14:57, ihab.awad wrote:
> glorpf! 
> 
> Narrower Scope: Scope S2 is narrower than scope S1 if the AST of S2 is a
subtree
> of the AST of S1.
> 
> Wider Scope: The inverse of Narrower Scope.

Tried to clean up the language and took David-Sarah's suggestion about
contained/container.

http://codereview.appspot.com/148045/diff/1/6#newcode72
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:72: *       wider scope. 
E.g. In <code>var x; function (x) { ... x ... }}</code>
On 2009/11/17 06:14:57, ihab.awad wrote:
> {@code ... } ?

It contains lots of curly brackets and no angle brackets.

http://codereview.appspot.com/148045/diff/1/6#newcode93
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:93: protected abstract
boolean fnCtorsDeclareInBody();
On 2009/11/10 00:39:07, ihab.awad wrote:
> Should this simply be a boolean parameter to the ScopeAnalyzer ctor?

It could be either.

http://codereview.appspot.com/148045/diff/1/6#newcode172
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:172: s.declared =
Sets.newHashSet();
On 2009/11/17 06:14:57, ihab.awad wrote:
> See comment below about why ScopeTree.declared not initialized statically
along
> with the other instance variables.

Fixed

http://codereview.appspot.com/148045/diff/1/6#newcode176
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:176: for (Symbol<S> use :
s.uses) { handleUse(use.id, use.useScope); }
On 2009/11/17 06:14:57, ihab.awad wrote:
> Can this line be moved up one, thus making the traversal a pure preorder (do
> this scope stuff, then recurse)?
> 
> I believe the answer is "no" because the recursively invoked declare(...) may
> hoist() up to this scope, and thus modify it, which would affect the behavior
of
> handleUse(...). 
> 
> If, as I suspect, the order is important in this way, I think a brief comment
> about why publishEvents(inner) needs to be done *before* handleUse(...) would
> help.

No.  We need to know the set of declarations visible in a scope so that we can
properly resolve uses to definitions.
Commented

http://codereview.appspot.com/148045/diff/1/6#newcode196
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:196: } else {  //
Declaration
On 2009/11/17 06:14:57, ihab.awad wrote:
> This should be deliberate:
> 
>   } else if (n instanceof Declaration) {
>   } else {
>     // Note about why this cannot happen
>     // Perhaps throw internal error or something?
>   }
> 
> Also, it turns out that the Declaration case only happens when you are
> traversing through a foreach loop; it's not clear in this code that that's
> indeed what's going on. I had a few "huh?" moments before I realized there was
> trickery afoot.

Done.

http://codereview.appspot.com/148045/diff/1/6#newcode207
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:207: private static
Operator lhsOperator(AncestorChain<Identifier> ac) {
On 2009/11/17 06:14:57, ihab.awad wrote:
> Maybe rename this function to say something about "assignment" because
> "lhsOperator" by itself caused me heartburn before I figured out what was
really
> going on.

Done.

http://codereview.appspot.com/148045/diff/1/6#newcode235
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:235: if
("this".equals(symbolName)) {
On 2009/11/17 06:14:57, ihab.awad wrote:
> Use pre-defined constant for "this".

Why?  Are they likely to change?

http://codereview.appspot.com/148045/diff/1/6#newcode301
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:301: * @param the scope
in which the identifier appears.
On 2009/11/17 06:14:57, ihab.awad wrote:
> @param >>scope<< the scope ...

Done.

http://codereview.appspot.com/148045/diff/1/6#newcode361
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:361: Set<String>
declared;
On 2009/11/17 06:14:57, ihab.awad wrote:
> Why is this not initialized statically like the other instance variables?

Done.

http://codereview.appspot.com/148045/diff/1/7
File src/com/google/caja/parser/js/scope/ScopeListener.java (right):

http://codereview.appspot.com/148045/diff/1/7#newcode20
src/com/google/caja/parser/js/scope/ScopeListener.java:20: public interface
ScopeListener<S extends AbstractScope> {
On 2009/11/10 00:39:07, ihab.awad wrote:
> This interface reads a little confusingly to me. Is type S the type of scopes
> being constructed by the listener? If not, then it must be the scopes
> constructed by the scope analyzer, right? If so, then the type of *these*
scopes
> is fixed and it need not be a type parameter.
> 
> To clear this up and separate concerns more crisply, I think you could get
away
> with not introducing a Scope in the signature of ScopeListener at all if your
> callbacks refer, not to a Scope, but to the ParseTreeNode corresponding to the
> Scope.
> 
> This change would not cause more objects to be created or more code to be run.
> The objects of type S being given to this listener are *still* being created
by
> the scope analyzer during its tree walk, and the listener is *still* creating
> its own custom scopes if it wants to.

ScopeAnalyzers don't create scopes.
ParseTreeNodes can't be conflated with scopes since they can appear more than
once in the same tree.

http://codereview.appspot.com/148045/diff/1/7#newcode29
src/com/google/caja/parser/js/scope/ScopeListener.java:29: void
declaration(AncestorChain<Identifier> id, S scope);
On 2009/11/10 00:39:07, ihab.awad wrote:
> For example:
> 
>   declaration(..., ParseTreeNode node)
> 
> where the node would be (say) a FunctionConstructor, a CatchStmt, an
> UncajoledModule, .... In other words, some node that *introduces* a new scope,
> but not the scope itself (building that is the job of the listener, if it
wants
> to).

Using nodes will not work since nodes can appear more than once in the same
tree.

http://codereview.appspot.com/148045/diff/1/9
File src/com/google/caja/parser/js/scope/ScopeType.java (right):

http://codereview.appspot.com/148045/diff/1/9#newcode34
src/com/google/caja/parser/js/scope/ScopeType.java:34: /**
On 2009/11/10 00:39:07, ihab.awad wrote:
> More spaces before "/*"

Done.

http://codereview.appspot.com/148045/diff/1/2
File tests/com/google/caja/parser/js/scope/ScopeAnalyzerTest.java (right):

http://codereview.appspot.com/148045/diff/1/2#newcode134
tests/com/google/caja/parser/js/scope/ScopeAnalyzerTest.java:134: " 
alert('stopped at ' + i);",
On 2009/11/17 06:14:57, ihab.awad wrote:
> Do you also want to read 'e' here just to make sure it hits the correct scope,
> or would that be redundant and repetitive and repetitious?

Added a separate test for that testExceptionReadAndAssign

http://codereview.appspot.com/148045/diff/1/2#newcode245
tests/com/google/caja/parser/js/scope/ScopeAnalyzerTest.java:245: "  var
arguments = (function (arguments) { return arguments[z]; })();",
On 2009/11/17 06:14:57, ihab.awad wrote:
> Could this be broken up into 3 test cases, namely, this:
> 
>   var arguments = (function(x) { ... })();
> 
> and this:
> 
>   var x = (function(arguments) { ... })();
> 
> and finally this:
> 
>   var arguments = (function(arguments) { ... })();
> 
> ?

Not without changing what it's testing.  In those cases, there is at most one
masking or redefinition.

What are these trying to test, and might it be better to include both sets of
tests?

[ihab.awad, 2009-11-17 19:31:08]

LGTM++

http://codereview.appspot.com/148045/diff/1/2
File tests/com/google/caja/parser/js/scope/ScopeAnalyzerTest.java (right):

http://codereview.appspot.com/148045/diff/1/2#newcode245
tests/com/google/caja/parser/js/scope/ScopeAnalyzerTest.java:245: "  var
arguments = (function (arguments) { return arguments[z]; })();",
On second thoughts, fine as-is.

[DavidSarah, 2009-11-17 21:14:45]

http://codereview.appspot.com/148045/diff/5017/6006
File src/com/google/caja/parser/js/scope/ScopeAnalyzer.java (right):

http://codereview.appspot.com/148045/diff/5017/6006#newcode72
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:72: *       containing
scope.
Add "A symbol is not considered to mask itself."

http://codereview.appspot.com/148045/diff/5017/6006#newcode237
src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:237: private ScopeTree<S>
defSite(String symbolName, ScopeTree<S> useSite) {
"defSite" sounds like a verb. How about "definingSite"?

http://codereview.appspot.com/148045/diff/5017/6007
File src/com/google/caja/parser/js/scope/ScopeListener.java (right):

http://codereview.appspot.com/148045/diff/5017/6007#newcode63
src/com/google/caja/parser/js/scope/ScopeListener.java:63: * {@code e = 3}
affects the exception variable.
Crumbs, hadn't thought of that. Now I remember why I didn't allow multiple
declaration in Jacaranda.

How does this interact with
http://code.google.com/p/google-caja/wiki/CatchBlocksScopeBleed -- presumably
the variables get merged in IE (so a 'duplicate' event should be fired)?

http://codereview.appspot.com/148045/diff/5017/6007#newcode92
src/com/google/caja/parser/js/scope/ScopeListener.java:92: * @param defSite the
scope in which id is defined or null in the case
Rename to "definingSite" here (and below) also if you decide to rename it in
ScopeAnalyzer.

http://codereview.appspot.com/148045/diff/5017/6009
File src/com/google/caja/parser/js/scope/ScopeType.java (right):

http://codereview.appspot.com/148045/diff/5017/6009#newcode77
src/com/google/caja/parser/js/scope/ScopeType.java:77: public final boolean
isDeclarationContainer;
document

http://codereview.appspot.com/148045/diff/5017/6009#newcode79
src/com/google/caja/parser/js/scope/ScopeType.java:79: ScopeType() {
this(false); }
document

http://codereview.appspot.com/148045/diff/5017/6009#newcode80
src/com/google/caja/parser/js/scope/ScopeType.java:80: ScopeType(boolean
isDeclarationContainer) {
document

http://codereview.appspot.com/148045/diff/5017/6009#newcode84
src/com/google/caja/parser/js/scope/ScopeType.java:84: public static ScopeType
forNode(ParseTreeNode n) {
document

http://codereview.appspot.com/148045/diff/5017/6002
File tests/com/google/caja/parser/js/scope/ScopeAnalyzerTest.java (right):

http://codereview.appspot.com/148045/diff/5017/6002#newcode34
tests/com/google/caja/parser/js/scope/ScopeAnalyzerTest.java:34: public class
ScopeAnalyzerTest extends CajaTestCase {
This doesn't test that JScriptScopeAnalyzer or WorstCaseScopeAnalyzer give the
same results as ES5ScopeAnalyzer in the cases where they are expected to be the
same. Perhaps define a helper method that will test all three on a given test
case.

[MikeSamuel, 2009-11-17 22:42:51]

On 2009/11/17 21:14:45, DavidSarah wrote:
> http://codereview.appspot.com/148045/diff/5017/6006
> File src/com/google/caja/parser/js/scope/ScopeAnalyzer.java (right):
> 
> http://codereview.appspot.com/148045/diff/5017/6006#newcode72
> src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:72: *       containing
> scope.
> Add "A symbol is not considered to mask itself."
> 
> http://codereview.appspot.com/148045/diff/5017/6006#newcode237
> src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:237: private
ScopeTree<S>
> defSite(String symbolName, ScopeTree<S> useSite) {
> "defSite" sounds like a verb. How about "definingSite"?
> 
> http://codereview.appspot.com/148045/diff/5017/6007
> File src/com/google/caja/parser/js/scope/ScopeListener.java (right):
> 
> http://codereview.appspot.com/148045/diff/5017/6007#newcode63
> src/com/google/caja/parser/js/scope/ScopeListener.java:63: * {@code e = 3}
> affects the exception variable.
> Crumbs, hadn't thought of that. Now I remember why I didn't allow multiple
> declaration in Jacaranda.
> 
> How does this interact with
> http://code.google.com/p/google-caja/wiki/CatchBlocksScopeBleed -- presumably
> the variables get merged in IE (so a 'duplicate' event should be fired)?
> 
> http://codereview.appspot.com/148045/diff/5017/6007#newcode92
> src/com/google/caja/parser/js/scope/ScopeListener.java:92: * @param defSite
the
> scope in which id is defined or null in the case
> Rename to "definingSite" here (and below) also if you decide to rename it in
> ScopeAnalyzer.
> 
> http://codereview.appspot.com/148045/diff/5017/6009
> File src/com/google/caja/parser/js/scope/ScopeType.java (right):
> 
> http://codereview.appspot.com/148045/diff/5017/6009#newcode77
> src/com/google/caja/parser/js/scope/ScopeType.java:77: public final boolean
> isDeclarationContainer;
> document
> 
> http://codereview.appspot.com/148045/diff/5017/6009#newcode79
> src/com/google/caja/parser/js/scope/ScopeType.java:79: ScopeType() {
> this(false); }
> document
> 
> http://codereview.appspot.com/148045/diff/5017/6009#newcode80
> src/com/google/caja/parser/js/scope/ScopeType.java:80: ScopeType(boolean
> isDeclarationContainer) {
> document
> 
> http://codereview.appspot.com/148045/diff/5017/6009#newcode84
> src/com/google/caja/parser/js/scope/ScopeType.java:84: public static ScopeType
> forNode(ParseTreeNode n) {
> document
> 
> http://codereview.appspot.com/148045/diff/5017/6002
> File tests/com/google/caja/parser/js/scope/ScopeAnalyzerTest.java (right):
> 
> http://codereview.appspot.com/148045/diff/5017/6002#newcode34
> tests/com/google/caja/parser/js/scope/ScopeAnalyzerTest.java:34: public class
> ScopeAnalyzerTest extends CajaTestCase {
> This doesn't test that JScriptScopeAnalyzer or WorstCaseScopeAnalyzer give the
> same results as ES5ScopeAnalyzer in the cases where they are expected to be
the
> same. Perhaps define a helper method that will test all three on a given test
> case.

D-S H, I believe I have addressed all these comments in issue 156050.