dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.25k stars 1.58k forks source link

Exception in AstBuilder #39230

Open bwilkerson opened 5 years ago

bwilkerson commented 5 years ago

Run analyzer on the following code:

class C {
  C()
    /
    : super();
}

This will produce the exception below.

@jensjoha I don't know whether the bug is in the parser (in that an operator should never be pushed as the name of a constructor) or in AstBuilder for not handling this case. The slash certainly doesn't appear in the place where a constructor name would normally be found. (If I try to use it as a constructor name C./() : super(); it doesn't crash and produces a MISSING_IDENTIFIER diagnostic.)

UnimplementedError: name is an instance of _OperatorName in endClassConstructor
package:analyzer/src/fasta/ast_builder.dart 696:7                  AstBuilder.endClassConstructor
package:_fe_analyzer_shared/src/parser/parser_impl.dart 3448:20    Parser.parseMethod
package:_fe_analyzer_shared/src/parser/parser_impl.dart 6537:19    Parser.parseInvalidOperatorDeclaration
package:_fe_analyzer_shared/src/parser/parser_impl.dart 6580:14    Parser.recoverFromInvalidMember
package:_fe_analyzer_shared/src/parser/parser_impl.dart 3196:16    Parser.parseClassOrMixinOrExtensionMemberImpl
package:_fe_analyzer_shared/src/parser/parser_impl.dart 2974:15    Parser.parseClassOrMixinOrExtensionBody
package:_fe_analyzer_shared/src/parser/parser_impl.dart 1820:13    Parser.parseClass
package:_fe_analyzer_shared/src/parser/parser_impl.dart 1779:14    Parser.parseClassOrNamedMixinApplication
package:_fe_analyzer_shared/src/parser/parser_impl.dart 574:14     Parser.parseTopLevelKeywordDeclaration
package:_fe_analyzer_shared/src/parser/parser_impl.dart 470:14     Parser.parseTopLevelDeclarationImpl
package:_fe_analyzer_shared/src/parser/parser_impl.dart 352:15     Parser.parseUnit
test/generated/parser_fasta_test.dart 2100:12                      FastaParserTestCase.parseCompilationUnit2
test/generated/parser_fasta_test.dart 2057:9                       FastaParserTestCase.parseCompilationUnit
jensjoha commented 5 years ago

It is recovered like this:

class C {
  C() {}
  operator/(): super();
}

The operator is ended with an endClassConstructor because it has an initializer. I can update that so it doesn't if it's an operator --- https://dart-review.googlesource.com/c/sdk/+/124121/

bwilkerson commented 5 years ago

That seems like a particularly poor way to recover.

Generally speaking, recovery works by either inserting or skipping tokens until we have valid code. It's also generally (though certainly not always) better to recover in the way that requires the smallest number of changes. Skipping 1 token (the slash) seems like a better option than inserting 5 tokens.

Especially given that the goal of recovery is to interpret the input as being what the user intended and not as it was entered. I think it's clear to any human reader that the user intended to have a single constructor, not a constructor and an invalid operator declaration.

Any chance that we could improve recovery in this situation?

jensjoha commented 5 years ago

I don't think anything about "intend" for invalid code is "clear". Improving recovery is scheduled, I think, for next quarter.