Avoid crashing on synthetic elements.

Avoid crashing on synthetic elements.

R=asgerf@google.com

Committed: https://github.com/dart-lang/rasta/commit/eadf8f6d1a6fbf5d115e624affdf70259cf3c422

[ahe, 2016-08-02 08:23:42]

ahe@google.com changed reviewers:
+ asgerf@google.com, kasperl@google.com

[asgerf, 2016-08-02 09:00:17]

LGTM for this CL, although I am concerned about the code health implications.

https://codereview.chromium.org/2207473003/diff/1/lib/unresolved.dart
File lib/unresolved.dart (right):

https://codereview.chromium.org/2207473003/diff/1/lib/unresolved.dart#newcode90
lib/unresolved.dart:90: possiblyErroneousFunctionToIr(candidateTarget);
Is there a way to get the parameter names without resolving the method?

https://codereview.chromium.org/2207473003/diff/1/test/kernel/regression/unre...
File test/kernel/regression/unresolved_recovery.dart.txt (right):

https://codereview.chromium.org/2207473003/diff/1/test/kernel/regression/unre...
test/kernel/regression/unresolved_recovery.dart.txt:15: throw
core::_genericNoSuchMethod@dart.core(null, #testMissingTry, <dynamic>[],
<dynamic, dynamic>{}, null);
Not a direct issue for this CL, but this really should be a call to the method
containing the syntax error, right?

After reading the docs for isSyntheticError I am thinking the semantic visitor
should dispatch this call as an ordinary resolved static call (i.e. it should
end up in handleStaticInvoke).  Otherwise every visitUnresolvedSomething will
have to do another step of disambiguation.

[ahe, 2016-08-02 09:03:28]

On 2016/08/02 09:00:17, asgerf wrote:
> LGTM for this CL, although I am concerned about the code health implications.

Could you elaborate on what you mean by "code health implications"?

[asgerf, 2016-08-02 09:45:57]

On 2016/08/02 09:03:28, ahe wrote:
> On 2016/08/02 09:00:17, asgerf wrote:
> > LGTM for this CL, although I am concerned about the code health
implications.
> 
> Could you elaborate on what you mean by "code health implications"?

It adds another layer of disambiguation after the semantic visitor. Why do we
split into so many cases, when many of share the same handling, and the split
is not even sufficient?

On another note:
I think there is a huge cost associated with adding more concepts to an API, 
one that is too easily overlooked because the concept is always fresh in mind
and easy to understand at the time it was introduced.

We are starting to rely more on this notion of "synthetic error elements".
That means we have:
1. unresolved things (as defined by the semantic visitor)
2. erroneous things (isError)
3. synthetic erroneous things (isSyntheticError)
4. malformed things (isMalformed)

The bug was fixed for now, but I don't see why the same bug isn't likely to show
up again next week.  The steps needed to "safely" get the parameter names of a
function feel completely arbitrary.  There is no way anyone would realistically 
write that at the first attempt based on the documentation.

What does hasFunctionSignature mean? And why is defined on a different interface
than the one with functionSignature?  The documentation does contain not the
useful details we need to write error-free client code against the element model
and the semantic visitor.


I didn't want to dump a rant like this here because these are not issues
I would expect to be fixed in this CL.  It just displays some of the symptoms
of what I consider to be a problematic API, hence code health "implications".

[ahe, 2016-08-02 11:31:21]

On 2016/08/02 09:45:57, asgerf wrote:
> On 2016/08/02 09:03:28, ahe wrote:
> > On 2016/08/02 09:00:17, asgerf wrote:
> > > LGTM for this CL, although I am concerned about the code health
> implications.
> > 
> > Could you elaborate on what you mean by "code health implications"?
> 
> It adds another layer of disambiguation after the semantic visitor. Why do we
> split into so many cases, when many of share the same handling, and the split
> is not even sufficient?
> 
> On another note:
> I think there is a huge cost associated with adding more concepts to an API, 
> one that is too easily overlooked because the concept is always fresh in mind
> and easy to understand at the time it was introduced.
> 
> We are starting to rely more on this notion of "synthetic error elements".
> That means we have:
> 1. unresolved things (as defined by the semantic visitor)
> 2. erroneous things (isError)
> 3. synthetic erroneous things (isSyntheticError)
> 4. malformed things (isMalformed)
> 
> The bug was fixed for now, but I don't see why the same bug isn't likely to
show
> up again next week.  The steps needed to "safely" get the parameter names of a
> function feel completely arbitrary.  There is no way anyone would
realistically 
> write that at the first attempt based on the documentation.
> 
> What does hasFunctionSignature mean? And why is defined on a different
interface
> than the one with functionSignature?  The documentation does contain not the
> useful details we need to write error-free client code against the element
model
> and the semantic visitor.
> 
> 
> I didn't want to dump a rant like this here because these are not issues
> I would expect to be fixed in this CL.  It just displays some of the symptoms
> of what I consider to be a problematic API, hence code health "implications".

I agree. Generally, I think the dart2js element model has too many elements, and
that Kernel AST has found a good balance.

[ahe, 2016-08-02 12:59:35]

Thank you for the comments, Asger. They really got me thinking about error
recovery in dart2js, but as you put it yourself, it's not really an issue for
this CL.

https://codereview.chromium.org/2207473003/diff/1/lib/unresolved.dart
File lib/unresolved.dart (right):

https://codereview.chromium.org/2207473003/diff/1/lib/unresolved.dart#newcode90
lib/unresolved.dart:90: possiblyErroneousFunctionToIr(candidateTarget);
On 2016/08/02 09:00:16, asgerf wrote:
> Is there a way to get the parameter names without resolving the method?

There's a deprecated method `Element.computeType`, but I don't think we should
be using that.

https://codereview.chromium.org/2207473003/diff/1/test/kernel/regression/unre...
File test/kernel/regression/unresolved_recovery.dart.txt (right):

https://codereview.chromium.org/2207473003/diff/1/test/kernel/regression/unre...
test/kernel/regression/unresolved_recovery.dart.txt:15: throw
core::_genericNoSuchMethod@dart.core(null, #testMissingTry, <dynamic>[],
<dynamic, dynamic>{}, null);
On 2016/08/02 09:00:16, asgerf wrote:
> Not a direct issue for this CL, but this really should be a call to the method
> containing the syntax error, right?

In principle, yes, but it's complicated and something I should fix eventually.
It has to do with error recovery.

Currently, the parser will mark any method that contains a parse error as being
malformed. The parser doesn't distinguish between unrecoverable errors and
recoverable errors, and it doesn't matter if we could parse the signature if we
ignored the method body.

So if there is a parse error, we may not be able to compute a function
signature. We should be able to come up with a better model for this, that's
also intuitive to use.

Just how bad the situation is wasn't clear to me when I wrote isSyntheticError,
and it isn't really fulfilling its potential.

> After reading the docs for isSyntheticError I am thinking the semantic visitor
> should dispatch this call as an ordinary resolved static call (i.e. it should
> end up in handleStaticInvoke).  Otherwise every visitUnresolvedSomething will
> have to do another step of disambiguation.

FWIW, I noticed a typo in the documentation of isSyntheticError (it should say:
"However, returns *false* for elements that have an unrecoverable syntax
error.")

[ahe, 2016-08-02 13:05:30]

Description was changed from

==========
Avoid crashing on synthetic elements.
==========

to

==========
Avoid crashing on synthetic elements.

R=asgerf@google.com

Committed:
https://github.com/dart-lang/rasta/commit/eadf8f6d1a6fbf5d115e624affdf70259cf...
==========

[ahe, 2016-08-02 13:05:31]

Committed patchset #3 (id:40001) manually as
eadf8f6d1a6fbf5d115e624affdf70259cf3c422 (presubmit successful).