carbon-lang

carbon-lang copied to clipboard

Description of the bug:

No response

What did you do, or what's a simple way to reproduce the bug?

base class Foo { }
class Base {
  extend base Foo;
}

$ carbon compile a.carbon

What did you expect to happen?

In this particular case, it could've recovered with an assumed :, but I'd also (or perhaps initially/first) like to fix the more general case, where a recovery might not be feasible/correct/helpful.

What actually happened?

CHECK-fail/crash:

FATAL failure at toolchain/parse/parse.cpp:61: Invalid tree returned by Parse(): NodeId #node14 couldn't be extracted as a ClassDefinition. Trace:
Aggregate N6Carbon5Parse15ClassDefinitionE: begin
Vector: begin
NodeIdInCategory Decl: kind InvalidParseSubtree consumed
NodeIdInCategory Decl: kind InvalidParse consumed
NodeIdInCategory Decl error: kind ExtendModifier doesn't match
Vector: end
NodeIdForKind error: wrong kind ExtendModifier, expected ClassDefinitionStart
Aggregate N6Carbon5Parse15ClassDefinitionE: error
Error: ExtendModifier node left unconsumed.

Any other information, logs, or outputs that you want to share?

I know we (myself, @zygoloid and @jonmeow ) have had some conversations about this, but some was in person/un-recorded, or the records are hard for me to find/synthesize.

Currently, the failure in https://github.com/carbon-language/carbon-lang/blob/f922988c8ccaaf68e703e3126a4a26709efb460f/toolchain/parse/handle_base.cpp#L14-L25 constructs an invalid parse tree that looks like this:

  {kind: 'BaseIntroducer', text: 'base'},
  {kind: 'ExtendModifier', text: 'extend'},
{kind: 'BaseDecl', text: ';', has_error: yes, subtree_size: 3},

Specifically, this does not match the "shape" a BaseDecl expects, which is this:

  {kind: 'BaseIntroducer', text: 'base'},
  {kind: 'ExtendModifier', text: 'extend'},
  {kind: 'BaseColon', text: ':'},
  {kind: 'IdentifierNameExpr', text: 'Foo'},
{kind: 'BaseDecl', text: ';', subtree_size: 5},

And so the above crash/check-fail occurs.

I know we had talked about different representations for the failure and recovery.

But I'd like to focus on the case where recovery isn't feasible.

What should we do in that case? (eg: if the name (Foo) is missing entirely)

I thought we could, in check, skip any error nodes in Check's https://github.com/carbon-language/carbon-lang/blob/f922988c8ccaaf68e703e3126a4a26709efb460f/toolchain/check/check_unit.cpp#L345-L347

  while (auto maybe_node_id = traversal.Next()) {
    node_id = *maybe_node_id;
    auto parse_kind = context.parse_tree().node_kind(node_id);
    if (context.parse_tree().node_has_error(node_id)) {
      return false;
    }

But judging by the code in https://github.com/carbon-language/carbon-lang/blob/f922988c8ccaaf68e703e3126a4a26709efb460f/toolchain/check/handle_noop.cpp that's not the intent (the intent seems to be that we do walk through even the error nodes) - but perhaps the proposed skip ^ is the way to address these TODOs? (or perhaps some more general "we shouldn't check a parse tree with any invalid nodes at all" might be even more general/failing earlier?)

After this discussion, I wouldn't mind discussing how to improve the error recovery - I know we talked about some kind of "synthetic token" we could use to put the colon into the parse tree?