Don't recreate CoreTypes in transformers. Pass it in.

Don't recreate CoreTypes in transformers. Pass it in.

The same will be done later for ClassHierarchy. So, it would be up to
the client to decide which flavour to create, and whether the same
instance can be reused.


R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=

Committed: https://github.com/dart-lang/sdk/commit/3803374d3216ce2b0e6694a96b3fb62cb0618d74

[Paul Berry, 2017-05-26 19:04:55]

https://codereview.chromium.org/2904203003/diff/1/pkg/front_end/test/fasta/ko...
File pkg/front_end/test/fasta/kompile.status (right):

https://codereview.chromium.org/2904203003/diff/1/pkg/front_end/test/fasta/ko...
pkg/front_end/test/fasta/kompile.status:15: await: Crash
Why do all these tests crash now?  This seems like a sign that there may be a
bug in the CL.

[scheglov, 2017-05-26 19:17:58]

PTAL

https://codereview.chromium.org/2904203003/diff/1/pkg/front_end/test/fasta/ko...
File pkg/front_end/test/fasta/kompile.status (right):

https://codereview.chromium.org/2904203003/diff/1/pkg/front_end/test/fasta/ko...
pkg/front_end/test/fasta/kompile.status:15: await: Crash
On 2017/05/26 19:04:54, Paul Berry wrote:
> Why do all these tests crash now?  This seems like a sign that there may be a
> bug in the CL.

We need to create CoreTypes in AnalyzerLoader too.
Fixed.

[Paul Berry, 2017-05-26 19:20:17]

lgtm assuming Kevin or Peter approves

[ahe, 2017-05-29 14:08:46]

LGTM, provided all the comments below are addressed.

https://codereview.chromium.org/2904203003/diff/20001/pkg/analyzer/lib/src/fa...
File pkg/analyzer/lib/src/fasta/analyzer_loader.dart (right):

https://codereview.chromium.org/2904203003/diff/20001/pkg/analyzer/lib/src/fa...
pkg/analyzer/lib/src/fasta/analyzer_loader.dart:37: coreTypes = new
CoreTypes(program);
Add:

    ticker.logMs("Computed core types");

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/fasta.dart (right):

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta.dart:225: CoreTypes coreTypes = new
CoreTypes(program);
This has already been computed, so we shouldn't compute it again.

It's available as kernelTarget.loader.coreTypes in the above try-block.

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:259:
loader.computeHierarchy(_program);
If there's an InputError during buildOutlines, we shouldn't call buildProgram.

[scheglov, 2017-05-30 00:06:10]

https://codereview.chromium.org/2904203003/diff/20001/pkg/analyzer/lib/src/fa...
File pkg/analyzer/lib/src/fasta/analyzer_loader.dart (right):

https://codereview.chromium.org/2904203003/diff/20001/pkg/analyzer/lib/src/fa...
pkg/analyzer/lib/src/fasta/analyzer_loader.dart:37: coreTypes = new
CoreTypes(program);
On 2017/05/29 14:08:46, ahe wrote:
> Add:
> 
>     ticker.logMs("Computed core types");

I will, but I'm curious what the purpose of this logging.
Given that creating of CoreTypes is very cheap now, and is done lazily, we
neither measure time spent, nor verify that some step is successfully performed.

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/fasta.dart (right):

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta.dart:225: CoreTypes coreTypes = new
CoreTypes(program);
On 2017/05/29 14:08:46, ahe wrote:
> This has already been computed, so we shouldn't compute it again.
> 
> It's available as kernelTarget.loader.coreTypes in the above try-block.

Fixed.
We now extract CoreTypes in addition to Program from KernelTarget.

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:259:
loader.computeHierarchy(_program);
On 2017/05/29 14:08:46, ahe wrote:
> If there's an InputError during buildOutlines, we shouldn't call buildProgram.

"We shouldn't call" as "It is a known existing problem and we need to fix it
someday" or as "This is a new problem added in this CL and you need to fix it
before landing the CL"?

[ahe, 2017-05-30 09:35:15]

https://codereview.chromium.org/2904203003/diff/20001/pkg/analyzer/lib/src/fa...
File pkg/analyzer/lib/src/fasta/analyzer_loader.dart (right):

https://codereview.chromium.org/2904203003/diff/20001/pkg/analyzer/lib/src/fa...
pkg/analyzer/lib/src/fasta/analyzer_loader.dart:37: coreTypes = new
CoreTypes(program);
On 2017/05/30 00:06:10, scheglov wrote:
> On 2017/05/29 14:08:46, ahe wrote:
> > Add:
> > 
> >     ticker.logMs("Computed core types");
> 
> I will, but I'm curious what the purpose of this logging.
> Given that creating of CoreTypes is very cheap now, and is done lazily, we
> neither measure time spent, nor verify that some step is successfully
performed.

The only way to tell if no time is being spent computing core types is to log
it.

If you don't call the log method, the time (however minuscule) will be
attributed incorrectly to the next phase. So to be able to correctly analyze the
time spent in each phase, it's important that logMs is called at the end of the
phase.

If we don't call log here and someone slows down CoreTypes, we may not notice
it.

You might argue that we can just get those numbers by using the observatory, but
that's not entirely correct. The profiler in the observatory doesn't work well
for measuring short-running programs that may not have been fully optimized yet.

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:259:
loader.computeHierarchy(_program);
On 2017/05/30 00:06:10, scheglov wrote:
> On 2017/05/29 14:08:46, ahe wrote:
> > If there's an InputError during buildOutlines, we shouldn't call
buildProgram.
> 
> "We shouldn't call" as "It is a known existing problem and we need to fix it
> someday" or as "This is a new problem added in this CL and you need to fix it
> before landing the CL"?

I didn't know about it before seeing this change. I'm not aware of any crashes
on any of our existing test input, so I'm inclined to think that this is a new
problem.

I don't think computing the hierarchy is a solution to the problem.

Do you have an example of how this problem occurs? Could you leave out these
lines and mark a test as failing or crashing? Then we can look at it together
later today in our 1:1?

[scheglov, 2017-05-30 16:10:21]

https://codereview.chromium.org/2904203003/diff/20001/pkg/analyzer/lib/src/fa...
File pkg/analyzer/lib/src/fasta/analyzer_loader.dart (right):

https://codereview.chromium.org/2904203003/diff/20001/pkg/analyzer/lib/src/fa...
pkg/analyzer/lib/src/fasta/analyzer_loader.dart:37: coreTypes = new
CoreTypes(program);
On 2017/05/30 09:35:14, ahe wrote:
> On 2017/05/30 00:06:10, scheglov wrote:
> > On 2017/05/29 14:08:46, ahe wrote:
> > > Add:
> > > 
> > >     ticker.logMs("Computed core types");
> > 
> > I will, but I'm curious what the purpose of this logging.
> > Given that creating of CoreTypes is very cheap now, and is done lazily, we
> > neither measure time spent, nor verify that some step is successfully
> performed.
> 
> The only way to tell if no time is being spent computing core types is to log
> it.
> 
> If you don't call the log method, the time (however minuscule) will be
> attributed incorrectly to the next phase. So to be able to correctly analyze
the
> time spent in each phase, it's important that logMs is called at the end of
the
> phase.
> 
> If we don't call log here and someone slows down CoreTypes, we may not notice
> it.
> 
> You might argue that we can just get those numbers by using the observatory,
but
> that's not entirely correct. The profiler in the observatory doesn't work well
> for measuring short-running programs that may not have been fully optimized
yet.

I will not argue with this, you're right Observatory.

OK, now I understand why we need to log here.
Thank you.

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:259:
loader.computeHierarchy(_program);
On 2017/05/30 09:35:14, ahe wrote:
> On 2017/05/30 00:06:10, scheglov wrote:
> > On 2017/05/29 14:08:46, ahe wrote:
> > > If there's an InputError during buildOutlines, we shouldn't call
> buildProgram.
> > 
> > "We shouldn't call" as "It is a known existing problem and we need to fix it
> > someday" or as "This is a new problem added in this CL and you need to fix
it
> > before landing the CL"?
> 
> I didn't know about it before seeing this change. I'm not aware of any crashes
> on any of our existing test input, so I'm inclined to think that this is a new
> problem.
> 
> I don't think computing the hierarchy is a solution to the problem.
> 
> Do you have an example of how this problem occurs? Could you leave out these
> lines and mark a test as failing or crashing? Then we can look at it together
> later today in our 1:1?

Well, it is a pre-existing problem.

My feeling is that error handling in Fasta is written to be suitable for
one-shot compilers, not for the library that parses code for other reasons than
compile and fail fast on the very first error.

Specifically this program causes InputError in "loader.buildOutlines()".

----
class A {
  int foo() native 'foo';
}
----

But fasta still calls [buildProgram], which we agreed it should not do.

I call fasta using the following command line:
/Users/scheglov/Source/Dart/sdk.git/sdk/pkg/front_end/tool/fasta compile
--platform=/Users/scheglov/Source/Dart/sdk.git/sdk/xcodebuild/ReleaseX64/patched_sdk/outline.dill
bin/test3.dart

I will send you a tiny patch that demonstrates that [buildOutlines] completes
with InputError and [buildProgram] is still called. You can patch it in without
this CL and get convinced that this CL does not change the status quo, it merely
makes test cases on which Fasta throws InputError, and which have already been
in Fail, to be still Fail instead of Crash.

[ahe, 2017-05-31 11:20:16]

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:259:
loader.computeHierarchy(_program);
On 2017/05/30 16:10:21, scheglov wrote:
> On 2017/05/30 09:35:14, ahe wrote:
> > On 2017/05/30 00:06:10, scheglov wrote:
> > > On 2017/05/29 14:08:46, ahe wrote:
> > > > If there's an InputError during buildOutlines, we shouldn't call
> > buildProgram.
> > > 
> > > "We shouldn't call" as "It is a known existing problem and we need to fix
it
> > > someday" or as "This is a new problem added in this CL and you need to fix
> it
> > > before landing the CL"?
> > 
> > I didn't know about it before seeing this change. I'm not aware of any
crashes
> > on any of our existing test input, so I'm inclined to think that this is a
new
> > problem.
> > 
> > I don't think computing the hierarchy is a solution to the problem.
> > 
> > Do you have an example of how this problem occurs? Could you leave out these
> > lines and mark a test as failing or crashing? Then we can look at it
together
> > later today in our 1:1?
> 
> Well, it is a pre-existing problem.
> 
> My feeling is that error handling in Fasta is written to be suitable for
> one-shot compilers, not for the library that parses code for other reasons
than
> compile and fail fast on the very first error.

I completely understand why you get that feeling when Fasta throws an
InputError. When it does so, it's because it has given up and didn't have any
good way to report an error.

Ideally, Fasta should never throw an InputError, because that solution is only
good for a one-shot compiler. We agree 100% on that, I think.

However, there are situations that are hard to recover from, so I took some
shortcuts initially, especially when it comes to missing files and unrecoverable
parser errors. We need to remove those shortcuts.

> Specifically this program causes InputError in "loader.buildOutlines()".
> 
> ----
> class A {
>   int foo() native 'foo';
> }
> ----
> 
> But fasta still calls [buildProgram], which we agreed it should not do.
> 
> I call fasta using the following command line:
> /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/front_end/tool/fasta compile
>
--platform=/Users/scheglov/Source/Dart/sdk.git/sdk/xcodebuild/ReleaseX64/patched_sdk/outline.dill
> bin/test3.dart
> 
> I will send you a tiny patch that demonstrates that [buildOutlines] completes
> with InputError and [buildProgram] is still called. You can patch it in
without
> this CL and get convinced that this CL does not change the status quo, it
merely
> makes test cases on which Fasta throws InputError, and which have already been
> in Fail, to be still Fail instead of Crash.

So it turns out I was wrong, buildProgram may be called after an error has
occurred, but it's supposed to stop early. Which it does because it checks
errors.isNotEmpty. However, it calls trimDependenciesInProgram which it probably
shouldn't do.

I've created a CL which removes the call to trimDependenciesInProgram on line
286 below, and I've tested that it enables you to remove these lines. See
https://codereview.chromium.org/2912353002

[scheglov, 2017-05-31 16:49:37]

PTAL

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:259:
loader.computeHierarchy(_program);
On 2017/05/31 11:20:16, ahe wrote:
> On 2017/05/30 16:10:21, scheglov wrote:
> > On 2017/05/30 09:35:14, ahe wrote:
> > > On 2017/05/30 00:06:10, scheglov wrote:
> > > > On 2017/05/29 14:08:46, ahe wrote:
> > > > > If there's an InputError during buildOutlines, we shouldn't call
> > > buildProgram.
> > > > 
> > > > "We shouldn't call" as "It is a known existing problem and we need to
fix
> it
> > > > someday" or as "This is a new problem added in this CL and you need to
fix
> > it
> > > > before landing the CL"?
> > > 
> > > I didn't know about it before seeing this change. I'm not aware of any
> crashes
> > > on any of our existing test input, so I'm inclined to think that this is a
> new
> > > problem.
> > > 
> > > I don't think computing the hierarchy is a solution to the problem.
> > > 
> > > Do you have an example of how this problem occurs? Could you leave out
these
> > > lines and mark a test as failing or crashing? Then we can look at it
> together
> > > later today in our 1:1?
> > 
> > Well, it is a pre-existing problem.
> > 
> > My feeling is that error handling in Fasta is written to be suitable for
> > one-shot compilers, not for the library that parses code for other reasons
> than
> > compile and fail fast on the very first error.
> 
> I completely understand why you get that feeling when Fasta throws an
> InputError. When it does so, it's because it has given up and didn't have any
> good way to report an error.
> 
> Ideally, Fasta should never throw an InputError, because that solution is only
> good for a one-shot compiler. We agree 100% on that, I think.
> 
> However, there are situations that are hard to recover from, so I took some
> shortcuts initially, especially when it comes to missing files and
unrecoverable
> parser errors. We need to remove those shortcuts.
> 
> > Specifically this program causes InputError in "loader.buildOutlines()".
> > 
> > ----
> > class A {
> >   int foo() native 'foo';
> > }
> > ----
> > 
> > But fasta still calls [buildProgram], which we agreed it should not do.
> > 
> > I call fasta using the following command line:
> > /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/front_end/tool/fasta compile
> >
>
--platform=/Users/scheglov/Source/Dart/sdk.git/sdk/xcodebuild/ReleaseX64/patched_sdk/outline.dill
> > bin/test3.dart
> > 
> > I will send you a tiny patch that demonstrates that [buildOutlines]
completes
> > with InputError and [buildProgram] is still called. You can patch it in
> without
> > this CL and get convinced that this CL does not change the status quo, it
> merely
> > makes test cases on which Fasta throws InputError, and which have already
been
> > in Fail, to be still Fail instead of Crash.
> 
> So it turns out I was wrong, buildProgram may be called after an error has
> occurred, but it's supposed to stop early. Which it does because it checks
> errors.isNotEmpty. However, it calls trimDependenciesInProgram which it
probably
> shouldn't do.
> 
> I've created a CL which removes the call to trimDependenciesInProgram on line
> 286 below, and I've tested that it enables you to remove these lines. See
> https://codereview.chromium.org/2912353002

Nice.
Thank you.
I removed this workaround.

[scheglov, 2017-05-31 16:50:23]

On 2017/05/31 16:49:37, scheglov wrote:
> PTAL
> 
>
https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
> File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):
> 
>
https://codereview.chromium.org/2904203003/diff/20001/pkg/front_end/lib/src/f...
> pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:259:
> loader.computeHierarchy(_program);
> On 2017/05/31 11:20:16, ahe wrote:
> > On 2017/05/30 16:10:21, scheglov wrote:
> > > On 2017/05/30 09:35:14, ahe wrote:
> > > > On 2017/05/30 00:06:10, scheglov wrote:
> > > > > On 2017/05/29 14:08:46, ahe wrote:
> > > > > > If there's an InputError during buildOutlines, we shouldn't call
> > > > buildProgram.
> > > > > 
> > > > > "We shouldn't call" as "It is a known existing problem and we need to
> fix
> > it
> > > > > someday" or as "This is a new problem added in this CL and you need to
> fix
> > > it
> > > > > before landing the CL"?
> > > > 
> > > > I didn't know about it before seeing this change. I'm not aware of any
> > crashes
> > > > on any of our existing test input, so I'm inclined to think that this is
a
> > new
> > > > problem.
> > > > 
> > > > I don't think computing the hierarchy is a solution to the problem.
> > > > 
> > > > Do you have an example of how this problem occurs? Could you leave out
> these
> > > > lines and mark a test as failing or crashing? Then we can look at it
> > together
> > > > later today in our 1:1?
> > > 
> > > Well, it is a pre-existing problem.
> > > 
> > > My feeling is that error handling in Fasta is written to be suitable for
> > > one-shot compilers, not for the library that parses code for other reasons
> > than
> > > compile and fail fast on the very first error.
> > 
> > I completely understand why you get that feeling when Fasta throws an
> > InputError. When it does so, it's because it has given up and didn't have
any
> > good way to report an error.
> > 
> > Ideally, Fasta should never throw an InputError, because that solution is
only
> > good for a one-shot compiler. We agree 100% on that, I think.
> > 
> > However, there are situations that are hard to recover from, so I took some
> > shortcuts initially, especially when it comes to missing files and
> unrecoverable
> > parser errors. We need to remove those shortcuts.
> > 
> > > Specifically this program causes InputError in "loader.buildOutlines()".
> > > 
> > > ----
> > > class A {
> > >   int foo() native 'foo';
> > > }
> > > ----
> > > 
> > > But fasta still calls [buildProgram], which we agreed it should not do.
> > > 
> > > I call fasta using the following command line:
> > > /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/front_end/tool/fasta compile
> > >
> >
>
--platform=/Users/scheglov/Source/Dart/sdk.git/sdk/xcodebuild/ReleaseX64/patched_sdk/outline.dill
> > > bin/test3.dart
> > > 
> > > I will send you a tiny patch that demonstrates that [buildOutlines]
> completes
> > > with InputError and [buildProgram] is still called. You can patch it in
> > without
> > > this CL and get convinced that this CL does not change the status quo, it
> > merely
> > > makes test cases on which Fasta throws InputError, and which have already
> been
> > > in Fail, to be still Fail instead of Crash.
> > 
> > So it turns out I was wrong, buildProgram may be called after an error has
> > occurred, but it's supposed to stop early. Which it does because it checks
> > errors.isNotEmpty. However, it calls trimDependenciesInProgram which it
> probably
> > shouldn't do.
> > 
> > I've created a CL which removes the call to trimDependenciesInProgram on
line
> > 286 below, and I've tested that it enables you to remove these lines. See
> > https://codereview.chromium.org/2912353002
> 
> Nice.
> Thank you.
> I removed this workaround.

Ah, actually I think I fixed all your comments.
So, I will land it now.

[scheglov, 2017-05-31 16:54:33]

Description was changed from

==========
Don't recreate CoreTypes in transformers. Pass it in.

The same will be done later for ClassHierarchy. So, it would be up to
the client to decide which flavour to create, and whether the same
instance can be reused.


R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

to

==========
Don't recreate CoreTypes in transformers. Pass it in.

The same will be done later for ClassHierarchy. So, it would be up to
the client to decide which flavour to create, and whether the same
instance can be reused.


R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=

Committed:
https://github.com/dart-lang/sdk/commit/3803374d3216ce2b0e6694a96b3fb62cb0618d74
==========

[scheglov, 2017-05-31 16:54:34]

Committed patchset #4 (id:60001) manually as
3803374d3216ce2b0e6694a96b3fb62cb0618d74 (presubmit successful).

[ahe, 2017-05-31 17:34:40]

On 2017/05/31 16:50:23, scheglov wrote:
> Ah, actually I think I fixed all your comments.
> So, I will land it now.

Yes. Thank you!