Mock up all remaining core library implementation classes.

Mock up all remaining core library implementation classes.

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

[ahe, 2012-02-16 23:56:09]

Total: 3084 tests                              
 * 258 tests will be skipped
 * 1 tests are expected to be flaky but not crash
 * 1273 tests are expected to pass
 * 0 tests are expected to fail that we won't fix
 * 1533 tests are expected to fail that we should fix
 * 17 tests are expected to crash that we should fix
 * 0 tests are allowed to timeout

--- Total time: 12:21 ---

[kasperl, 2012-02-17 06:03:30]

LGTM.

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/frog_leg....
File dart/frog/leg/frog_leg.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/frog_leg....
dart/frog/leg/frog_leg.dart:111: if (element.position() === null) element =
element.enclosingElement;
Maybe add a comment on when this happens? What kind of elements do not have a
position? Would it make sense to have a helper on the element that runs out
through the enclosingElement chain until it finds an element with a position?

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/resolver....
File dart/frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/resolver....
dart/frog/leg/resolver.dart:79: visitor.inInstanceContext = true;
Do you want to reset the flag to what ever it was after the call to
visitor.visit(tree.body)? If so you might want to have a
visitor.visitInInstanceContext(tree.body) helper.

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/tests/co19/co19-le...
File dart/tests/co19/co19-leg.status (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/tests/co19/co19-le...
dart/tests/co19/co19-leg.status:8: # These classes have been removed, co19 issue
79.
w00t! This is very nice. It feels really nice to have explicit Fail markers for
failing tests.

[ahe, 2012-02-17 07:40:47]

Maybe I should add some TODOs. But I think I also need to prepare some kind of
document, possibly a slide show to explain some of the ideas behind the element
model.

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/frog_leg....
File dart/frog/leg/frog_leg.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/frog_leg....
dart/frog/leg/frog_leg.dart:111: if (element.position() === null) element =
element.enclosingElement;
On 2012/02/17 06:03:31, kasperl wrote:
> Maybe add a comment on when this happens? What kind of elements do not have a
> position? Would it make sense to have a helper on the element that runs out
> through the enclosingElement chain until it finds an element with a position?

I'm not sure this API is where I want it. For example, if we need a synthetic
"this" element, it's position should probably be the span of the entire method
to which it corresponds. In other words, a Node, not a Token.

I think something is broken in the SSA builder and closurizer. It is creating
ad-hoc elements (which is really something the resolver should provide), and it
creates the wrong kind of elements using "new Element" and doesn't record
positions. Later it gets confused by these ad-hoc elements and tries to report
an internal error.

I think I need to write up something about how to use elements and how to
maintain a sane semantic model. Right now, I can see that the SSA builder and
the type checker are repeating work that earlier phases should take of.

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/resolver....
File dart/frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/resolver....
dart/frog/leg/resolver.dart:79: visitor.inInstanceContext = true;
On 2012/02/17 06:03:31, kasperl wrote:
> Do you want to reset the flag to what ever it was after the call to
> visitor.visit(tree.body)? If so you might want to have a
> visitor.visitInInstanceContext(tree.body) helper.

I think this is more broken than it lets out. Later on, the SSA builder fudges
up a ConstructorBodyElement which is a much cleaner solution had it been applied
here.

This brings me back to needing to communicate a few high-level principles on how
to use elements and maintain a semantic model.

[ngeoffray, 2012-02-17 10:03:50]

DBC

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/frog_leg....
File dart/frog/leg/frog_leg.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/frog_leg....
dart/frog/leg/frog_leg.dart:111: if (element.position() === null) element =
element.enclosingElement;
On 2012/02/17 07:40:47, ahe wrote:
> On 2012/02/17 06:03:31, kasperl wrote:
> > Maybe add a comment on when this happens? What kind of elements do not have
a
> > position? Would it make sense to have a helper on the element that runs out
> > through the enclosingElement chain until it finds an element with a
position?
> 
> I'm not sure this API is where I want it. For example, if we need a synthetic
> "this" element, it's position should probably be the span of the entire method
> to which it corresponds. In other words, a Node, not a Token.
> 
> I think something is broken in the SSA builder and closurizer. It is creating
> ad-hoc elements (which is really something the resolver should provide), and
it
> creates the wrong kind of elements using "new Element" and doesn't record
> positions. Later it gets confused by these ad-hoc elements and tries to report
> an internal error.
> 
> I think I need to write up something about how to use elements and how to
> maintain a sane semantic model. Right now, I can see that the SSA builder and
> the type checker are repeating work that earlier phases should take of.

The thing is, the backend should not cancel on elements. It does not know
whether an element is part of the model or is a synthetized element created just
for its purposes. Another alternative that we talked about with Florian is to
have an interface BackendElement that all elements created by the backend should
implement.

Also, only the backend and the emitter see these synthetized elements. The
resolver and type checker never see them.

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/lib/mocki...
File dart/frog/leg/lib/mockimpl.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/lib/mocki...
dart/frog/leg/lib/mockimpl.dart:105: class TimeZoneImplementation implements
TimeZone {
Why do you need to copy this over?

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/resolver....
File dart/frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/resolver....
dart/frog/leg/resolver.dart:79: visitor.inInstanceContext = true;
On 2012/02/17 07:40:47, ahe wrote:
> On 2012/02/17 06:03:31, kasperl wrote:
> > Do you want to reset the flag to what ever it was after the call to
> > visitor.visit(tree.body)? If so you might want to have a
> > visitor.visitInInstanceContext(tree.body) helper.
> 
> I think this is more broken than it lets out. Later on, the SSA builder fudges
> up a ConstructorBodyElement which is a much cleaner solution had it been
applied
> here.

It wasn't intended to be a solution for the resolver. The only reason the
backend needs a ConstructorBodyElement is because it creates two SSA methods,
and it cannot do that with one element. So it creates another one and inserts it
into the pipeline.

We could have decided to just generate one SSA method for a constructor, and we
would have then not introduced the ConstructorBodyElement.

> 
> This brings me back to needing to communicate a few high-level principles on
how
> to use elements and maintain a semantic model.

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/resolver....
dart/frog/leg/resolver.dart:79: visitor.inInstanceContext = true;
I don't think you need to set the flag. It's already set line 458 by checking
isGenerativeConstructor.

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/ssa/build...
File dart/frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/ssa/build...
dart/frog/leg/ssa/builder.dart:323: "Could not find value", element: element);
This is exactly what we talked about with Florian: not cancel on elements in the
backend. I believe it's on his TODO list to clean up error reporting in the
backend.

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/ssa/closu...
File dart/frog/leg/ssa/closure.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/ssa/closu...
dart/frog/leg/ssa/closure.dart:315: // visitChildren.
Please explain the hack: a parameter with a function type.

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/ssa/closu...
dart/frog/leg/ssa/closure.dart:370: // parameters, etc. Other phases should only
visit statements.
Not sure I understand this comment, because the backend does not create
parameters.

[floitsch, 2012-02-17 10:40:39]

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/ssa/build...
File dart/frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/ssa/build...
dart/frog/leg/ssa/builder.dart:323: "Could not find value", element: element);
While you are it: with "Cannot find value" it fits on one line (I think).

[floitsch, 2012-02-17 10:40:39]

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/ssa/build...
File dart/frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/ssa/build...
dart/frog/leg/ssa/builder.dart:323: "Could not find value", element: element);
While you are it: with "Cannot find value" it fits on one line (I think).

[ahe, 2012-02-17 16:26:45]

Hi Kasper, Florian, and Nicolas.

Thank you for taking a look and for all your suggestions.

Cheers,
Peter

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/frog_leg....
File dart/frog/leg/frog_leg.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/frog_leg....
dart/frog/leg/frog_leg.dart:111: if (element.position() === null) element =
element.enclosingElement;
On 2012/02/17 10:03:50, ngeoffray wrote:
> On 2012/02/17 07:40:47, ahe wrote:
> > On 2012/02/17 06:03:31, kasperl wrote:
> > > Maybe add a comment on when this happens? What kind of elements do not
have
> a
> > > position? Would it make sense to have a helper on the element that runs
out
> > > through the enclosingElement chain until it finds an element with a
> position?
> > 
> > I'm not sure this API is where I want it. For example, if we need a
synthetic
> > "this" element, it's position should probably be the span of the entire
method
> > to which it corresponds. In other words, a Node, not a Token.
> > 
> > I think something is broken in the SSA builder and closurizer. It is
creating
> > ad-hoc elements (which is really something the resolver should provide), and
> it
> > creates the wrong kind of elements using "new Element" and doesn't record
> > positions. Later it gets confused by these ad-hoc elements and tries to
report
> > an internal error.
> > 
> > I think I need to write up something about how to use elements and how to
> > maintain a sane semantic model. Right now, I can see that the SSA builder
and
> > the type checker are repeating work that earlier phases should take of.
> 
> The thing is, the backend should not cancel on elements. It does not know
> whether an element is part of the model or is a synthetized element created
just
> for its purposes. Another alternative that we talked about with Florian is to
> have an interface BackendElement that all elements created by the backend
should
> implement.
> 
> Also, only the backend and the emitter see these synthetized elements. The
> resolver and type checker never see them.

Nicolas and I went back to the drawing board, and I'll follow up on that later.

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/lib/mocki...
File dart/frog/leg/lib/mockimpl.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/lib/mocki...
dart/frog/leg/lib/mockimpl.dart:105: class TimeZoneImplementation implements
TimeZone {
On 2012/02/17 10:03:50, ngeoffray wrote:
> Why do you need to copy this over?

I need the constructors.

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/resolver....
File dart/frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/resolver....
dart/frog/leg/resolver.dart:79: visitor.inInstanceContext = true;
On 2012/02/17 10:03:50, ngeoffray wrote:
> I don't think you need to set the flag. It's already set line 458 by checking
> isGenerativeConstructor.

Done.

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/ssa/build...
File dart/frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/ssa/build...
dart/frog/leg/ssa/builder.dart:323: "Could not find value", element: element);
On 2012/02/17 10:40:40, floitsch wrote:
> While you are it: with "Cannot find value" it fits on one line (I think).

Done.

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/ssa/closu...
File dart/frog/leg/ssa/closure.dart (right):

https://chromiumcodereview.appspot.com/9417031/diff/1/dart/frog/leg/ssa/closu...
dart/frog/leg/ssa/closure.dart:370: // parameters, etc. Other phases should only
visit statements.
On 2012/02/17 10:03:50, ngeoffray wrote:
> Not sure I understand this comment, because the backend does not create
> parameters.

The backend should run through "element.computeParameters(compiler)" instead of
calling visitChildren that will visit the very confusing parameter list.