Repository navigation
Name the ToString override a node has to carry - #1042
Merged
Merged
Conversation
Every one of the 69 node types has 'public override string ToString() => Stringize();' and this guide, which exists to list every place a new node must be taught about, does not mention it. It is not optional and it is not a compiler error when missed. Entity is a record, so C# synthesises ToString from PrintMembers, which prints every public property; Evaled and InnerSimplified are public and Entity-valued, so each of them calls ToString again. A node that omits the override does not print wrongly -- it exhausts the stack, at a call site nowhere near the node. Found while measuring what an Entity subclass outside the kernel assembly needs in order to work (#1026).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Docs/Contributing/AddingNode.csexists to list every place a new node must be taught about —AGENTS.mdsays to read it before adding one. It does not mentionToString.Every one of the 69 node types carries
public override string ToString() => Stringize();:Why omitting it is worse than a wrong string
Entityis arecord, so C# synthesisesToStringfromPrintMembers, which prints every publicproperty. Two of
Entity's areEntity-valued:So the synthesised
ToStringcallsToStringon each of them, which calls it on theirs, and so on.A node missing the override does not print the wrong thing — it exhausts the stack, and it does so
at a call site nowhere near the node, which is the hard kind of failure to trace back.
Nothing enforces it. It is not a compiler error, no analyzer checks it, and the 69 existing
occurrences are the only reason it has never bitten: every node was copied from one that had it,
which is exactly the kind of invariant that survives until the first person who does not copy.
Scope
One line of guidance in
AddingNode.cs, +6 lines. No code, no behaviour change, noBREAKING-CHANGES.mdentry. Touches no file any other open PR touches.Found while measuring what an
Entitysubclass outside the kernel assembly needs in order to work —#1026, and the design document for it is
#1040.