Analyzer enum value constant initializers have some issues

[srawlins]

Some issues with enum value "constant initializers": If the constructor call is implicit, as in enum E { one, two; }, then for the element representing E.one (a ConstFieldElementImpl), then I see a constantInitializer field, an InstanceCreationExpressionImpl node, with the following:

• None of the AST nodes here claim to be synthetic, via isSynthetic. Not the InstanceCreationExpressionImpl, the ArgumentListImpl, etc.

• The toString() shows E(). Of course the constantInitializer of E.two also shows E(). This seems wrong, or at least disingenuous, to claim a call to a default constructor with no arguments, for both calls.

  The ConstFieldElementImpl does have an evaluationResult field, a DartObject showing accurate data, aligning with the spec. Namely that there are two fields, _name = 'one' and index = 0. 👍

  The spec suggests deguaring an enum like this here with a private constructor like E._$(int _$index, String_ $name), and an enum value as a static field like static const E one = E._$(0, "one");. (Indeed, during const evaluation, we appear to do something like this, based on the DartObject.)

  I suggest our constantInitializer should then look something like E(0, 'one') or even just E(0), in order to give some semblance of differentiation between E.one and E.two.

Dartdoc relies on the analyzer's Element model, and these constant initializers, to give some information about how an enum value is declared.

[bwilkerson]

None of the AST nodes here claim to be synthetic ...

I found the same issue when converting some of the code completion support. It might have been the wrong decision, but I decided to be pragmatic and work around the problem by writing an extension method:

extension on AstNode {
  /// Whether all of the tokens in this node are synthetic.
  bool get isFullySynthetic {}
}

I was concerned that changing the behavior of isSynthetic might have implications for a lot code and that cleaning it up right then would delay work that was blocking other people. But it's definitely something I think we should look into fixing.

[srawlins]

I meant to add: I'm not fully aware of the meaning of "isSynthetic." It might be "not found in source, AND created by the parser during parser error recovery." And if it has a specific purpose in regards to that meaning, it's not super important to me to "fix" the field or add a second field that means something like "is not found in the source." Various Tokens in this AST sub-tree have an offset of -1, which may also just be a meaningful indicator...

[bwilkerson]

I'm not fully aware of the meaning of "isSynthetic."

Then it clearly isn't documented well enough.

It might be "not found in source, AND created by the parser during parser error recovery."

Yes. The problem is that we don't always mark parent nodes as synthetic, even when all of the child nodes and tokens in the parent node are synthetic. (I can't remember a concrete example, but I could find it by looking at the uses of isFullySynthetic.) I think that's a bug in the implementation. The only question is whether there's now code depending on the bug.