NullRefException in CollectAtoms if Expansion(Exp e, string) called with e != Exp.Atom

[bentorkington]

The constructor leaves Tail as null, which eventually crashes in CollectedAtoms()

Should Expansion(Exp, string) create an empty list, or Assert(symbol == Exp.Atom)?

[maetl]

When I was writing the code originally, I think the intention was that there would be certain constraints around Tail and Term being mutually exclusive (the expansion either contains a list of children, or is a leaf node with a string) with Term only applying to Atom symbols.

The Assert definitely establishes what the expected behaviour is for developers. I prefer that to the empty list, but not sure if there is a more elegant way to express this intention.

Am also wondering if this would be a good candidate for using static factory/builder methods to construct instances and maybe make the constructors protected or private so that the only way to get subtree expansions is to go via the expected interface, rather than running into this.

Expansion exp = Expansion.Create(Exp.Expression, Expansion.CreateAtom(term));

Not particularly smart or elegant but does hide the need to care about the Term/Tail constraints from syntax nodes and productions (or maybe just sweeps it under the rug, there might be a better answer using a polymorphic interface).

[maetl]

It really does seem like I need to sketch out a specification for the result objects as I can see the missing details and lack of clarity around the original dynamically typed recursive array implementations is going to have ongoing friction in C# (and similar, if we decide to port Calyx to C++ or Rust etc for Unreal in future, the same problems will repeat in that context too).

I haven’t explored this yet, but I have been mulling over the idea of being able to generate enums and structs in addition to text, as that would be a huge value-add for Unity projects, in terms of being useful for statblocks, procgen weapons, magic spells, etc. Would be doable with some ugly hacks to Result and Grammar.Generate(), but I’ve been wondering if some sort of decorator API might be a more robust extension point here.

Also wonder about letting people do something like Result<MyValueType> and Grammar.Generate<MyValueType>().

But yeah, this is just speculative sketching and haven’t really thought it through. I can see how some more robust thinking around the way the expansion tree works could be beneficial and lay the ground work for this.

[bentorkington]

Am also wondering if this would be a good candidate for using static factory/builder methods to construct instances and maybe make the constructors protected or private so that the only way to get subtree expansions is to go via the expected interface, rather than running into this.

After our discussion in #21 I was thinking making Expansion abstract and having a two subclasses for Atom and NonAtom expansions (Strategy pattern)

//Expansion.cs (Abstract)
protected virtual void CollectAtoms() { }

//Atom.cs, inherits Expansion
public readonly string Term;
protected void CollectAtoms(StringBuilder sb) {
  sb.Append(Term);
}

//NonAtom.cs (Abstract, inherits Expansion, all nonatom classes inherit)
public List<Expansion> Tail;
protected void CollectAtoms(StringBuilder sb) {
  Tail.ForEach((t) => t.CollectAtoms(sb)
}

This should probably stop most foot-guns without needing a factory

[maetl]

Thanks, I was hoping you would articulate your thoughts on that. It makes a lot more sense just to go with the flow and use polymorphic dispatch, rather than trying to wrangle constructors to maintain invariants.

[bentorkington]

Wish I hadn't opened my mouth. I started refactoring all this and put myself straight in a world of pain because I underestimated it all and should have planned more.

I thought I was clever writing Equals() overrides for the Expansion type to clean up all those Tail[0].Tail[0].Tail[0]… strings in the unit tests, forgetting that sooner or later I'd hit one that only cares about the structure and not equality;

      Assert.That(exp.Symbol, Is.EqualTo(Exp.Template));
      Assert.That(exp.Tail[0].Symbol, Is.EqualTo(Exp.Memo));
      Assert.That(exp.Tail[0].Tail[0].Symbol, Is.EqualTo(Exp.UniformBranch));
      Assert.That(exp.Tail[0].Tail[0].Tail[0].Symbol, Is.EqualTo(Exp.Template));
      Assert.That(exp.Tail[0].Tail[0].Tail[0].Tail[0].Symbol, Is.EqualTo(Exp.Atom));

      Assert.That(exp.Tail[1].Symbol, Is.EqualTo(Exp.Memo));
      Assert.That(exp.Tail[1].Tail[0].Symbol, Is.EqualTo(Exp.UniformBranch));
      Assert.That(exp.Tail[1].Tail[0].Tail[0].Symbol, Is.EqualTo(Exp.Template));
      Assert.That(exp.Tail[1].Tail[0].Tail[0].Tail[0].Symbol, Is.EqualTo(Exp.Atom));

      Assert.That(exp.Tail[2].Symbol, Is.EqualTo(Exp.Memo));
      Assert.That(exp.Tail[2].Tail[0].Symbol, Is.EqualTo(Exp.UniformBranch));
      Assert.That(exp.Tail[2].Tail[0].Tail[0].Symbol, Is.EqualTo(Exp.Template));
      Assert.That(exp.Tail[2].Tail[0].Tail[0].Tail[0].Symbol, Is.EqualTo(Exp.Atom));

      string firstTerm = exp.Tail[0].Tail[0].Tail[0].Tail[0].Term;
      string secondTerm = exp.Tail[1].Tail[0].Tail[0].Tail[0].Term;
      string thirdTerm = exp.Tail[2].Tail[0].Tail[0].Tail[0].Term;
      Assert.That(new[] { firstTerm, secondTerm }, Is.All.EqualTo(thirdTerm));

Was hoping to get a PR to you today that fixes this bug (the very very long way) but not so sure now