MassaLabs: Add semicolon in AirScript

[sydhds]

No description provided.

[bitwalker]

Once we have a proper IR to lower the AST into, we should be able to simplify the grammar by flattening it, i.e. we won't need to nest StatementBlocks inside Let.

For now, it's necessary because we're using the AST both as a syntax tree and as an intermediate representation, so keeping the language structure expression-oriented simplifies aspects of that. One upside of having that limitation, if I had to pick one, is that it forces us to avoid syntax that would require execution semantics we can't or don't want to support, but that's something we can still accomplish by sticking to functional-immutable semantics.

Anyway, nothing actionable here, just wanted to provide some context on this, since I'm sure it seems weird.

[bitwalker]

Overall, looks great! If you could fix the clippy errors so the build passes, that's the only real blocker.

I would like to get @bobbinth's input on whether semicolons are the right choice for internal declarations of trace_columns and the like, but even if we're not convinced that it's the best choice of syntax, we can always address it later, so we don't need to hold up merging this on that aspect.

[bitwalker]

@bobbinth Perhaps it would make more sense to use , with these top-level definitions? Semicolons feels a bit bizarre in what amounts to a data structure definition.

In statement blocks and stuff, it feels pretty natural, but less so here.

[bobbinth]

Agreed - in declarations like these, commas would be a more natural choice.

[bobbinth]

Looks great! Thank you! I don't have much additional comments besides @bitwalker 's suggestion to use commas in the "declarative" sections. Once this is addressed, we can merge.

[bobbinth]

Following @bitwalker 's comment, I think in these sections, we should use commas - e.g., they would look like so:

public_inputs {
    stack_inputs: [16],
}

trace_columns {
    main: [s, a, b, a0, a1, a2, a3, b0, b1, b2, b3, zp, z, dummy],
}

periodic_columns {
    k0: [1, 0, 0, 0, 0, 0, 0, 0],
    k1: [1, 1, 1, 1, 1, 1, 1, 0],
}

boundary_constraints and integrity_constraints sections below look good as is.

[sydhds]

Fixed :)

[bitwalker]

@sydhds I've fixed the clippy warnings in next, so once you rebase your changes, the build should be green again. There may be some conflicts due to the function implementation, but shouldn't be anything major, mostly just the fact that we likely touched the same files and tests, but I think the actual changes are pretty disjoint.

[sydhds]

@sydhds I've fixed the clippy warnings in next, so once you rebase your changes, the build should be green again. There may be some conflicts due to the function implementation, but shouldn't be anything major, mostly just the fact that we likely touched the same files and tests, but I think the actual changes are pretty disjoint.

Rebase done so I think it should be all good :)

[bitwalker]

LGTM! Thanks @sydhds!

[bobbinth]

Looks good! Thank you! I left a couple of comments inline about adding commas to separate clauses of the match statement. But if that's difficult to do - we can do that in a separate PR.

[bobbinth]

Should we add commas to the end of each case clause? For example, something like this:

enf match {
    case s[1] & s[2]: is_unchanged([clk, s[0]]),
    case !s[1] & !s[2]: next_is_one([clk]),
};

[sydhds]

I was planning to add that in another PR but let me know if you want in this one?

[bobbinth]

A follow-up PR is fine! I'll merge this one now then.

[bobbinth]

Similar comment as the one above here.