Implement correct scoping rules for variables.

Implement correct scoping rules for variables.

BUG=13205, 13016

[karlklose, 2013-09-26 07:05:42]

https://codereview.chromium.org/24488004/diff/3001/pkg/analyzer_experimental/...
File pkg/analyzer_experimental/lib/src/services/formatter_impl.dart (left):

https://codereview.chromium.org/24488004/diff/3001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:1316:
previousToken = token;
@pquitslund: This looks like a bug.

[ahe, 2013-09-26 07:48:06]

Let's talk about how to accomplish this without bloating AST nodes with
information that isn't part of the AST nodes.

https://codereview.chromium.org/24488004/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/scanner/listener.dart (right):

https://codereview.chromium.org/24488004/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:1076:
{List<VariableDefinitions> declarations}) {
Please don't use named arguments. They add call overhead, but in this case,
declarations don't belong here in the first place. A node list is a list of
nodes. That's it.

https://codereview.chromium.org/24488004/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:1469:
List<VariableDefinitions> declarations = <VariableDefinitions>[];
This is what we have a link builder for. Please use that to avoid allocating
growable lists in the parser.

https://codereview.chromium.org/24488004/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/24488004/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:558: final
Link<VariableDefinitions> declarations;
I'm not sure this is a good place for this information. It feels like this
belongs in the element model.

https://codereview.chromium.org/24488004/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:1570: final
Link<VariableDefinitions> declarations;
This doesn't feel like a part of a switch case.

[ngeoffray, 2013-09-26 07:48:38]

I share Peter's concerns.

https://codereview.chromium.org/24488004/diff/3001/pkg/analyzer_experimental/...
File pkg/analyzer_experimental/lib/src/services/formatter_impl.dart (left):

https://codereview.chromium.org/24488004/diff/3001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:1316:
previousToken = token;
On 2013/09/26 07:05:43, karlklose wrote:
> @pquitslund: This looks like a bug.

Why did you have to fix it? Isn't it OK to shadow a field?

https://codereview.chromium.org/24488004/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/24488004/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1923:
Elements.isLocal(result)) {
One line? Also, what is this check? That result is a local but not a parameter?
How about function  statements/expression? Aren't their runtime type
FunctionElement?

Can't you do Elements.isLocal(result) && !result.isParameter() &&
!result.isFunction()? It would look clearer to me.

https://codereview.chromium.org/24488004/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/24488004/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:558: final
Link<VariableDefinitions> declarations;
This seems off. Can't you handle during resolution?

https://codereview.chromium.org/24488004/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:1582: this.startToken,
this.declarations);
indentation.

[karlklose, 2013-09-27 07:07:43]

I'll rewrite it to not modify the AST.

https://codereview.chromium.org/24488004/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/24488004/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:558: final
Link<VariableDefinitions> declarations;
We do not really have a place to store it in the model, so I'll rewrite this to
compute the information in the resolver. These declarations are part of the
tree, but I see you point of keeping the AST nodes as clean as possible, and the
information can be recomputed by walking the tree again.

https://codereview.chromium.org/24488004/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:1570: final
Link<VariableDefinitions> declarations;
They are part of the statements, that's why I added it here and in Block.

[pquitslund, 2013-09-30 16:12:48]

https://codereview.chromium.org/24488004/diff/3001/pkg/analyzer_experimental/...
File pkg/analyzer_experimental/lib/src/services/formatter_impl.dart (left):

https://codereview.chromium.org/24488004/diff/3001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:1316:
previousToken = token;
On 2013/09/26 07:05:43, karlklose wrote:
> @pquitslund: This looks like a bug.

Hmmm, I agree this doesn't look right.  I have another proposed fix though. 
Let's just remove the variable declaration for 'previousToken'.  I don't think I
intended to be shadowing here but I do think I want the side-effect of updating
the cached previous.  (Although, that may benefit from a rethink too.)

[pquitslund, 2013-09-30 17:23:34]

https://codereview.chromium.org/24488004/diff/3001/pkg/analyzer_experimental/...
File pkg/analyzer_experimental/lib/src/services/formatter_impl.dart (left):

https://codereview.chromium.org/24488004/diff/3001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/formatter_impl.dart:1316:
previousToken = token;
On 2013/09/30 16:12:48, pquitslund wrote:
> On 2013/09/26 07:05:43, karlklose wrote:
> > @pquitslund: This looks like a bug.
> 
> Hmmm, I agree this doesn't look right.  I have another proposed fix though. 
> Let's just remove the variable declaration for 'previousToken'.  I don't think
I
> intended to be shadowing here but I do think I want the side-effect of
updating
> the cached previous.  (Although, that may benefit from a rethink too.)
> 

And here's that fix: https://codereview.chromium.org/25304002/