Use synthetic elements to recover from errors in signatures.dart.

Use synthetic elements to recover from errors in signatures.dart.

BUG=https://code.google.com/p/dart/issues/detail?id=22021
R=karlklose@google.com, zarah@google.com

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

[ahe, 2015-01-12 10:08:54]

Patchset #1 (id:1) has been deleted

[ahe, 2015-01-12 10:10:53]

ahe@google.com changed reviewers:
+ zarah@google.com

[karlklose, 2015-01-12 10:17:07]

karlklose@google.com changed reviewers:
+ karlklose@google.com

[karlklose, 2015-01-12 10:17:08]

https://codereview.chromium.org/845183004/diff/20001/dart/pkg/compiler/lib/sr...
File dart/pkg/compiler/lib/src/elements/modelx.dart (right):

https://codereview.chromium.org/845183004/diff/20001/dart/pkg/compiler/lib/sr...
dart/pkg/compiler/lib/src/elements/modelx.dart:1382: DartType
computeType(Compiler compiler) => type;
Why do we need to add this? Do these now flow into code that does not use the
type getter?

[ahe, 2015-01-12 10:25:33]

Thank you for taking a look, Karl!

https://codereview.chromium.org/845183004/diff/20001/dart/pkg/compiler/lib/sr...
File dart/pkg/compiler/lib/src/elements/modelx.dart (right):

https://codereview.chromium.org/845183004/diff/20001/dart/pkg/compiler/lib/sr...
dart/pkg/compiler/lib/src/elements/modelx.dart:1382: DartType
computeType(Compiler compiler) => type;
On 2015/01/12 10:17:07, karlklose wrote:
> Why do we need to add this? Do these now flow into code that does not use the
> type getter?

I've identified the place that causes an exception in signatures.dart. Since
that is part of the resolver, so I don't think it is a problem that it invokes
computeType.

https://codereview.chromium.org/845183004/diff/40001/dart/pkg/compiler/lib/sr...
File dart/pkg/compiler/lib/src/resolution/signatures.dart (right):

https://codereview.chromium.org/845183004/diff/40001/dart/pkg/compiler/lib/sr...
dart/pkg/compiler/lib/src/resolution/signatures.dart:121: element.typeCache =
fieldElement.computeType(compiler);
I added ErroneousFieldElementX.computeType to prevent a crash here.

[karlklose, 2015-01-12 11:29:02]

LGTM.

Is there a way you can test this using the incremental compiler unit tests?

[zarah, 2015-01-12 11:32:18]

lgtm

[ahe, 2015-01-12 13:55:24]

On 2015/01/12 11:29:02, karlklose wrote:
> Is there a way you can test this using the incremental compiler unit tests?

The incremental compiler isn't (yet) good a handling code with compile-time
errors. So I don't think it would help.

I think the coverage we get from existing tests is good enough for now. If I
hadn't synthesized new elements, the compiler would crash when I removed "throw
new CompilerCancelledException(null)". And no new CompilerCancelledException
should be introduced, as the next CL will delete that class.

A related thing we discussed in person: how can we ensure the compiler's state
is consistent after error recovery? I'm not sure we can. I expect that early
adopters of incremental compilation will find a lot of problems that stem from
poor error recovery, and we'll have to write new regression tests based on those
findings. It's unfortunate that early users will have to deal with that, but I
don't see any way around that.

[ahe, 2015-01-12 13:55:55]

Thank you, Karl and Sarah for taking a look!

[ahe, 2015-01-12 13:57:38]

Committed patchset #2 (id:40001) manually as 42772 (presubmit successful).

[Johnni Winther, 2015-01-13 09:24:54]

johnniwinther@google.com changed reviewers:
+ johnniwinther@google.com

[Johnni Winther, 2015-01-13 09:24:54]

lgtm