proposal: use boolean in multiple return value to distinguish optional fields on AST nodes

[mewmew]

The current generation of the AST inserts /*opt*/ comments for optional fields on AST nodes. Besides the comments, no checks will help ensure that the AST node is not used incorrectly, except for always checking IsValid, which is not required for mandatory fields.

The suggestion is to change the method of optional fields from:

func (n AShrExpr) Exact() /*opt*/ Exact {
    return Exact{n.Child(selector.Exact)}
}

to:

func (n AShrExpr) Exact() (Exact, bool) {
    field := n.Child(selector.Exact)
    return Exact{field}, field.IsValid()
}

This would help distinguish optional AST node fields from mandatory fields in the package documentation, and also produce compile time errors if an optional field is used in the same way as a mandatory field.

Currently, to use an optional field, users would do something along the lines of:

if exact := n.Exact(); exact.IsValid() {
    // use exact field.
}

After the proposed change, uses would instead write something along the lines of:

if exact, ok := n.Exact(); ok {
    // use exact field.
}

The change may seem rather minimal, but it does help a lot to make sure that optional fields are never used without first checking for validity.

[inspirer]

Agreed, this is a very nice idea.

[mewmew]

Great :)

[mewmew]

Example diff as a user of the generated parser: https://github.com/llir/llvm/commit/e83017ac9459746a2c6e8eb591dca5b9831074a4

And example diff for the generated parser: https://github.com/llir/ll/commit/f8c2eef740b10d533f6a820217aad3dcaf8692f7

[mewmew]

Something that bites me as strange is how optional interface fields are not distinguished as optional fields.

Take for instance the optional calling convention on call instructions, as per the following grammar:

https://github.com/llir/grammar/blob/b01e345b685dd61b51787d1255a9ddfd0b6c9baf/ll.tm#L2617

CallInst -> CallInst
    : Tailopt 'call' FastMathFlags=FastMathFlag* CallingConvopt ReturnAttrs=ReturnAttribute* AddrSpaceopt Typ=Type Callee=Value '(' Args ')' FuncAttrs=FuncAttribute* OperandBundles=('[' (OperandBundle separator ',')+ ']')? Metadata=(',' MetadataAttachment)+?
;

%interface CallingConv;

CallingConv -> CallingConv
    : CallingConvEnum
    | CallingConvInt
;

The corresponding AST node for call instructions looks as follows:

https://github.com/llir/ll/blob/opt-field/ast/ast.go#L2235-L2237

func (n CallInst) CallingConv() CallingConv {
    return ToLlvmNode(n.Child(selector.CallingConv)).(CallingConv)
}

As such, the node is not optional from it's signature, however, upon further investigation, the interface implements nilNode, which essentially makes it potentially optional. Whether the field is optional or not depends on the production rule it is used in then, as should be, since CallingConv can be used both as a mandatory field e.g. CallingConv or an optional field e.g. CallingConvopt.

If possible, it would be great to understand exactly why this is the case and see if we can update the AST generation to also add the boolean check for optional fields of interface type.

func (n CallInst) CallingConv() (CallingConv, bool) {
    field := ToLlvmNode(n.Child(selector.CallingConv)).(CallingConv)
    return field, field.LlvmNode().IsValid()
}

[mewmew]

May we re-open this issue until we can decide how to handle optional interface fields? ref: https://github.com/inspirer/textmapper/issues/27#issuecomment-443191935

[inspirer]

Sure. I'll update the templates to produce the second boolean return value for optional interface fields.