How can I get analyzer/fasta to report compile errors in extensions?

[munificent]

I'm working on adding dartfmt support for extension methods. I'm using analyzer 0.38.1 and front_end 0.1.23 and so far it looks like it's parsing them and giving me good ASTs. Yay!

I have an extension that erroneously declares a field:

extension A on B {
  var a = 1;
}

In this case, I expect analyzer to report a syntax/compile error, which dartfmt will report to the user and abort formatting. Instead, it just silently gives me an extension with all of the fields removed.

Is there a setting I can turn on for this?

[bwilkerson]

No, there's only one flag for everything.

When I paste that into a package that has the experiment enabled (and add a declaration of B), I see the following error in the IDE:

error: Extensions can't declare instance fields. (extension_declares_instance_field at [test_package] lib/test.dart:41)

(ignore the line offset, there was other code in the file)

Unfortunately, that isn't a parse error. The grammar in the proposal just says that the body of an extension consists of memberDeclaration*, which doesn't preclude instance fields or constructors. (Static fields are allowed, so dartfmt might want to just deal with instance fields anyway.)

[stereotype441]

@munificent I'm surprised that you're seeing an extension with all of the fields removed. What I would expect, since the formatter only asks the analyzer to parse and not to do any further analysis, would be that you would see the illegal field in the AST, and so you could format it normally, just as you would format any other code that had a non-parse-related error.

In fact, we definitely don't want the fields to be removed from the AST, because a lot of other analysis features (completion, quick fixes, etc.) rely on having an AST that completely describes the code, even if there are errors. If it's the analyzer that's dropping the fields, I'm interested in investigating. Do you have a repro you could upload somewhere?

[munificent]

What I would expect, since the formatter only asks the analyzer to parse and not to do any further analysis, would be that you would see the illegal field in the AST

Maybe it's in there somewhere and I'm not handling it right. I haven't dug into it much. Here's an example I saw:

The formatter produced unexpected output. Input was:
extension A on B {
  var a = 1;
  b() {}
  c() => null;
  get d {}
  get e => null;
  set f(value) {}
  set g(value) => null;
  var h = 1;
}

Which formatted to:
extension A on B {
  b() {}
  c() => null;
  get d {}
  get e => null;
  set f(value) {}
  set g(value) => null;
}

The primary issue was that I'm not getting an error reported that I expected to show up. But I suppose a valid approach might be to have dartfmt actually format variable declarations inside extensions even though they are compile errors. Formatting other kinds of broken code is hard because it's not even clear how you would format it. But, in this case, the formatting is obvious.

Do you have a repro you could upload somewhere?

For you, sure. :)

Clone https://github.com/dart-lang/dart_style/tree/wip-extension-methods. Then run dart example/format.dart. That's the code I mentioned in my comment here. You can also run the tests pub run test test/formatter_test.dart to run some other extension method tests. (Don't worry about failures, though, this is WIP.)

[stereotype441]

It looks like the analyzer is reporting the errors in the AstBuilder, and dropping the fields from the ASTs. It's doing a similar thing with constructors and abstract methods that it sees in the extension declaration. I believe what it should do instead is the same thing that all the other AstBuilder error detection logic does: report the errors but go ahead and include the erroneous code in the ASTs, to aid in error recovery. Not only will this allow the formatter to format an extension with illegal fields, it means all the other analyzer features (such as completions) will still mostly work.

I started to prototype this in https://dart-review.googlesource.com/c/sdk/+/114380/1, but it's failing some tests because the presence of the illegal members in the AST is causing later crashes in the analyzer. @bwilkerson, does this sound like a reasonable idea to you?

[bwilkerson]

... a lot of other analysis features (completion, quick fixes, etc.) rely on having an AST that completely describes the code, even if there are errors.

I hope that "rely" isn't actually true. There are lots of error cases that we can't represent in the AST. We do generally try to represent as much as possible of the original code in the AST, but we shouldn't have code that relies on the AST being complete.

I believe what it should do instead is the same thing that all the other AstBuilder error detection logic does: report the errors but go ahead and include the erroneous code in the ASTs, to aid in error recovery.

That is the approach we generally take. I suspect that this approach would be fine for abstract methods and instance fields, but we don't currently have a way for extension declarations to store a reference to a constructor, so constructors might be a bit harder to handle that way.

I'd be interested in knowing what crashes you saw, because that might give me more information about how difficult this will be to change.

I'm also interested to know whether @danrubel knows of any reason why we shouldn't take this approach in this case.

[stereotype441]

@bwilkerson

I'd be interested in knowing what crashes you saw, because that might give me more information about how difficult this will be to change.

You can see the crashes in the trybot failures from the CL I mentioned in my last comment: https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8904313982492702928/+/steps/test_results/0/logs/unapproved_new_test_failures__logs_/0

[bwilkerson]

Thanks! I should have thought of looking there.

As expected, those are all related to constructors. If we leave the constructors in the AST then we expect to be able to find elements for them in the element model, but we're currently not creating elements for them.

We don't have the same problem with mixins (which are also not allowed to have constructors) because we chose to use a ClassElement to represent them. We chose to not use a ClassElement to represent extensions because, unlike mixins, extensions don't define a type.

That said, I think we might be confusing two related but possibly separable issues. One is the issue of what to represent in the AST and element model when given invalid code. This is an important decision, and a discussion we should have, but not the immediate question.

The issue that dartfmt is facing is that there is syntactically invalid code that it doesn't recognize as being invalid. I suspect that dartfmt is filtering the list of errors to look for those whose errorCode is a ParserErrorCode. I think that's generally the right thing for it to do because analyzer doesn't guarantee that only parse errors will be reported when clients ask for a parse result.

In my opinion, the underlying problem is that AstBuilder is using a CompileTimeErrorCode to report what probably ought to be a parse error. I say "probably" because the proposal has an incomplete definition of the grammar:

Extensions are declared using the syntax:

<extension> ::= `extension' <identifier>? <typeParameters>? `on' <type> `?'?
  `{'
    <memberDeclaration>*
  `}'

where extension becomes a built-in identifier and does not allow instance variables, constructors or abstract members. It does allow static members.

If the final grammar accepted in the specification allows and disallows the same things, then constructors, instance fields and abstract members will all clearly be parse errors. If not, then it's a little harder to determine whether they ought to be parse errors or not.

I think we should strongly consider treating these errors as parse errors either way.

@danrubel Is there a reason why they aren't currently parse errors (produced by the parser)?

@johnniwinther How is the front-end treating such errors? What would the impact be of changing these to be true parse errors?

[stereotype441]

@bwilkerson those are really good points. I'd support the idea of making these true parse errors, produced by the parser. @johnniwinther can correct me if I'm wrong, but believe there would be practically zero impact on the front end of changing these into true parse errors, because no one relies heavily on the error recovery behavior of the front end.

So I guess what we should be doing here is changing the parser so that it reports the errors, and changing the parser/AstBuilder so that error recovery happens in a way that's consistent with the assumptions of element model. For example, the AstBuilder could recover from instance fields in extensions by pretending they are static and creating synthetic "static" tokens. And the parser could report constructors found in extensions using beginMethod/endMethod, so that they show up in the AST as (mostly) legal method declarations. Does that sound right?

[danrubel]

@bwilkerson

Is there a reason why they aren't currently parse errors (produced by the parser)?

Originally when I mapped fasta messages to existing analyzer error codes, I did not change any CompileTimeErrorCode to ParserErrorCode. Just recently I fixed a dartfmt bug by doing just that in https://dart-review.googlesource.com/c/sdk/+/113760.

There are 4 places in AstBuilder where we produce CompileTimeErrorCode. Should I change all of them to ParserErrorCode or only the 3 where you have added comments?

[bwilkerson]

Should I change all of them to ParserErrorCode or only the 3 where you have added comments?

I think all of them should be parse errors (unless I'm missing something there are 4 references to 3 codes, all of which are new and have to do with extensions). But the question is, can we catch them in the parser rather than in AstBuilder? If we can't catch them in the parser then front-end won't see them. (Still, better to have them reported as parse errors, and moving them into the parser can be a separate step, even assuming it's possible.)

... AstBuilder could recover from instance fields in extensions by pretending they are static and creating synthetic "static" tokens.

Possibly. That would make them show up as static members when completing after the name of the extension, which might not be good. If (and I have no data here) the more common fix is for users to convert instance fields into getter/setter pairs then it might be better to leave them as instance fields.

And the parser could report constructors found in extensions using beginMethod/endMethod, so that they show up in the AST as (mostly) legal method declarations.

The right way to recover here is even less clear in my mind. That approach matches the advice analyzer currently produces: to try converting the constructor into a static method. However, that's only going to be valid in a subset of cases, and I don't know what name we'd create for whatever method we produced.

For abstract members I think it's OK if we just leave them abstract. We'll want to make sure we don't have follow-on errors from doing so, but I suspect they won't be too hard to suppress if there are any.

[munificent]

I suspect that dartfmt is filtering the list of errors to look for those whose errorCode is a ParserErrorCode.

Close. The relevant bit in dartfmt is:

    // Fasta produces some semantic errors, which we want to ignore so that
    // users can format code containing static errors.
    if (error.errorCode.type != ErrorType.SYNTACTIC_ERROR) return;

I'm happy to tweak this if it will help. My main concern is that I want to make sure that I have good dartfmt test coverage for all code that it will parse without error. I don't want to end up in the uncanny valley where there is some code the parser accepts but I don't realize it accepts it and thus don't test it. There is a failsafe in dartfmt to avoid deleting user code if this happens, but I don't want to rely on that. :)

Also, I don't generally want to be in the business of writing dartfmt tests of which invalid Dart programs do not parse without error. I don't want to duplicate all of the existing tests for that. This does put dartfmt in a somewhat weird position where an important behavior that it relies on — that erroneous code will report an error — isn't fully tested. But I felt that was the right pragmatic choice and so far it's worked out mostly OK.

[bwilkerson]

I think it's the right choice. I just had the mechanics a little different, but I think the approach it's currently using should be equivalent, and from a design perspective might be slightly better.