Rename ClosureVariable, use separate IR forms for declaration and assignment.

Rename ClosureVariable, use separate IR forms for declaration and assignment.

I plan to use ClosureVariable for the variables assigned in try, so
the name will become incorrect.  They are renamed to MutableVariable.

Use separate IR forms for declaration of MutableVariable (LetMutable)
and for assignment to mutable variables (SetMutableVariable).  This
distinction doesn't do much in this change other than make the tree a
bit more well-formed, but I plan to make LetMutable take a list of
variables and values (e.g. all the variables mutated in a try).

R=asgerf@google.com, karlklose@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=43396

[karlklose, 2015-02-03 09:56:16]

LGTM with comments.

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:163: /// to prevent unrestricted
use of references to them.  During one-pass
Would it make sense to share the context (maybe on a commen interface that also
contains 'body')?

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:164: /// construction, a
[LetMutable} with an empty body is use to represent the
'[LetMutable}' -> '[LetMutable]'.

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart (left):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:356: String
nameClosureVariableIfAbsent(ClosureVariable variable) {
is this dead code now?

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:419: " add missing visit
overrides (${node.runtimeType})");
Please do not use '.runtimeType. in production code, it requires runtime type
support.  Normally 'toString' should include sufficient information.

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:53: final
Map<cps_ir.MutableVariable, Variable> local2mutable =
I would really like to get rid of this naming convention.

[asgerf, 2015-02-03 10:27:31]

LGTM with comments

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1533: <ExecutableElement,
List<ir.MutableVariable>>{};
I think this can be deleted actually. I believe it was used to create the list
of closure/mutable variables that you got rid of in one of the previous CLs.

The same goes for getMutablesList.

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1535: final
DartCapturedVariableInfo mutableVariables;
I think either the field should be `capturedVariables` or the class should be
renamed also.

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart (right):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:205: return
'(${node.runtimeType} ${access(node.variable)})';
What karl said about runtimeType, unless we're certain that this can be tree
shaked in production(?).

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:721: setValue(node,
getValue(node.variable.definition));
When targeting Dart I don't think we can track mutable variables like this,
because they can be reassigned inside a nested function.

We could either default to nonConst() while targeting Dart, or detect mutable
variables that are assigned inside nested functions.

For JS it should be fine, of course, since mutable variables are local to one
function activation.

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:748: : typeSystem.dynamicType;
Off-topic: Is type propagation relying on type annotations?

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart (right):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart:496: /// If true, this declares
a new copy of the closure variable.
"closure variable" -> "variable"

[asgerf, 2015-02-03 10:30:26]

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:721: setValue(node,
getValue(node.variable.definition));
On 2015/02/03 10:27:31, asgerf wrote:
> When targeting Dart I don't think we can track mutable variables like this,
> because they can be reassigned inside a nested function.
> 
> We could either default to nonConst() while targeting Dart, or detect mutable
> variables that are assigned inside nested functions.
> 
> For JS it should be fine, of course, since mutable variables are local to one
> function activation.

Actually on second thought, I didn't realize this was a flow-insensitive
analysis. It should be fine.

[Kevin Millikin (Google), 2015-02-03 14:11:49]

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1533: <ExecutableElement,
List<ir.MutableVariable>>{};
On 2015/02/03 10:27:31, asgerf wrote:
> I think this can be deleted actually. I believe it was used to create the list
> of closure/mutable variables that you got rid of in one of the previous CLs.
> 
> The same goes for getMutablesList.

I had there feeling there was more I could get rid of.

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1535: final
DartCapturedVariableInfo mutableVariables;
On 2015/02/03 10:27:31, asgerf wrote:
> I think either the field should be `capturedVariables` or the class should be
> renamed also.

Yeah, it's the captured variables.  capturedVariables is a better name than both
mutableVariables and closureVariables.

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:163: /// to prevent unrestricted
use of references to them.  During one-pass
On 2015/02/03 09:56:16, karlklose wrote:
> Would it make sense to share the context (maybe on a commen interface that
also
> contains 'body')?

It's not always a field named body that gets plugged.  The thing that is common
to all the one-hole contexts is that they have a .plug(Expression) method.

I suppose we could introduce an interface for Expressions that have .plug.  Do
you think it buys us much?

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:164: /// construction, a
[LetMutable} with an empty body is use to represent the
On 2015/02/03 09:56:16, karlklose wrote:
> '[LetMutable}' -> '[LetMutable]'.

Done.

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart (left):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:356: String
nameClosureVariableIfAbsent(ClosureVariable variable) {
On 2015/02/03 09:56:16, karlklose wrote:
> is this dead code now?

Yes.

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart (right):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:205: return
'(${node.runtimeType} ${access(node.variable)})';
On 2015/02/03 10:27:31, asgerf wrote:
> What karl said about runtimeType, unless we're certain that this can be tree
> shaked in production(?).

I had thought that the entire file was unused, but I guess that turns out not to
be true.  I'll go back to explicit strings.

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:419: " add missing visit
overrides (${node.runtimeType})");
On 2015/02/03 09:56:16, karlklose wrote:
> Please do not use '.runtimeType. in production code, it requires runtime type
> support.  Normally 'toString' should include sufficient information.

Done.

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:721: setValue(node,
getValue(node.variable.definition));
On 2015/02/03 10:30:26, asgerf wrote:
> On 2015/02/03 10:27:31, asgerf wrote:
> > When targeting Dart I don't think we can track mutable variables like this,
> > because they can be reassigned inside a nested function.
> > 
> > We could either default to nonConst() while targeting Dart, or detect
mutable
> > variables that are assigned inside nested functions.
> > 
> > For JS it should be fine, of course, since mutable variables are local to
one
> > function activation.
> 
> Actually on second thought, I didn't realize this was a flow-insensitive
> analysis. It should be fine.

Yes, but I'll add a comment explaining why this is OK.

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:748: : typeSystem.dynamicType;
On 2015/02/03 10:27:31, asgerf wrote:
> Off-topic: Is type propagation relying on type annotations?

At the moment it doesn't.

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:53: final
Map<cps_ir.MutableVariable, Variable> local2mutable =
On 2015/02/03 09:56:16, karlklose wrote:
> I would really like to get rid of this naming convention.

Concrete suggestions for naming maps?  Name them after the type of values?

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart (right):

https://codereview.chromium.org/898463002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart:496: /// If true, this declares
a new copy of the closure variable.
On 2015/02/03 10:27:31, asgerf wrote:
> "closure variable" -> "variable"

Done.  Actually, the second part of the comment doesn't make much sense any more
so I'll rewrite it too.

[Kevin Millikin (Google), 2015-02-03 14:18:32]

Committed patchset #2 (id:20001) manually as 43396 (presubmit successful).