`inst` defintion and its optional `stack_effect`

[nybblista]

The DSL specification syntax definition of the inst shows that the stack_effect is optional,
including its parentheses:

definition:
    "inst" "(" NAME ["," stack_effect] ")" "{" C-code "}"
...
stack_effect:
    "(" [inputs] "--" [outputs] ")"

But if we defined a new instruction, let's say, MY_INSTRUCTION as:

inst(MY_INSTRUCTION) {
  /* implementation */
}

We will get the following error:

SyntaxError: Extra stuff at the end of ./Python/bytecodes.c

However, if we defined MY_INSTRUCTION with an empty stack-effect, it will work!

inst(MY_INSTRUCTION, ( -- )) {
  /* implementation */
}

Linked PRs

• gh-133412: remove the brackets of the optional stack_effect #133708
• [3.14] gh-133412: amend docs for the inst definition (GH-133708) #133729
• [3.13] gh-133412: amend docs for the inst definition (GH-133708) #133730

[nybblista]

@brandtbucher

[picnixz]

I think it's easier to change the docs than to change the parser. And it's better, IMO, to keep an explicit stack effect as it helps readability and avoids having special cases during parsing.

[nybblista]

@picnixz I agree. Clearly defining the stack effect and enforcing it at the parser level will help ensure consistency in the inst definition across all instructions.

[nybblista]

The inst_header method of the parser shows that it is not dealing with the optional path. However, it will be enough to update the doc.

@contextual
def inst_header(self) -> InstHeader | None:
    # annotation* inst(NAME, (inputs -- outputs))
    # | annotation* op(NAME, (inputs -- outputs))
    annotations = []
    while anno := self.expect(lx.ANNOTATION):
        if anno.text == "replicate":
            self.require(lx.LPAREN)
            times = self.require(lx.NUMBER)
            self.require(lx.RPAREN)
            annotations.append(f"replicate({times.text})")
        else:
            annotations.append(anno.text)
    tkn = self.expect(lx.INST)
    if not tkn:
        tkn = self.expect(lx.OP)
    if tkn:
        kind = cast(Literal["inst", "op"], tkn.text)
        if self.expect(lx.LPAREN) and (tkn := self.expect(lx.IDENTIFIER)):
            name = tkn.text
            if self.expect(lx.COMMA):
                inp, outp = self.io_effect()
                if self.expect(lx.RPAREN):
                    if (tkn := self.peek()) and tkn.kind == lx.LBRACE:
                        return InstHeader(annotations, kind, name, inp, outp)
    return None

@picnixz @tomasr8

[tomasr8]

I think it should be enough to update the internal docs. I don't think there's any urgent need to support the optional syntax (imo, it's better to be explicit)

[picnixz]

I haven't merged your PR yet :). I'm only merging PRs when I'm on my Linux session and before 8PM (Paris time)