Attempt to define the AST

[mlechu]

Desugaring is massive: in both flisp and JuliaLowering, it is larger in bytes
than all other lowering passes combined. Being the first pass after parsing and
macro-expansion, it's also the judge of whether or not an AST is structurally
valid, causing all sorts of issues:

1. It isn't readable despite it being the de-facto specification of the
   lowerable AST. Combining this with any incompleteness or outdatedness in the
   AST developer documentation, macro authors are left to guess what inputs
   they'll see and outputs they can produce.

2. Desugaring is responsible for both checking input and transforming it. This
   is because macros can produce anything, and parsing produces a superset of
   lowering-recognized ASTs anyway. The checking here is especially unwanted
   given that much of desugaring's input is output from other desugaring
   functions (related to (4) below), which shouldn't go through the same level of
   checking as arbitrary user input.

3. Desugaring is limited in its capacity to produce errors. It won't detect
   multiple errors per pass, and doesn't point at more than one tree per error.
   There's no reason it can't, but the benefit of upgraded errors isn't
   clearly worth adding even more desugaring complexity.

4. How each expression gets through desugaring is generally unknown. In both
   implementations, we often take a sledgehammer approach:

   # flisp: (expand-forms (expand-foo foo))
   
   ex = expand_foo(ctx, foo) # we know we need to desugar away foo...
   expand_forms_2(ctx, ex)   # ...but some form we introduced needs more
                             # desugaring, and we don't know which one

   where it isn't clear if desugaring will terminate. (There have been cases
   where it hasn't: Stackoverflow while attempting to show error due to malformed :(...) syntax JuliaLang/julia#51572, Lowering of invalid . Expr loops forever JuliaLang/julia#57733)

5. Likely due to (1) making this difficult to even plan to implement, syntax
   evolution at the AST level is unimplemented.

This PR

The goal of this PR is to fix issues 1-3 and allow us to address issues 4 and 5
over time by pulling desugaring's "define and check the AST" responsibility into
a separate pass.

The new valid_st1([vcx::Validation1Context] st::SyntaxTree)::Bool function
check that st is a structurally valid input to lowering/output of macro
expansion. It and its callees serve three main purposes:

1. A readable reference for the julia AST structure
2. A set of assumptions we can use in lowering so we can simplify desugaring
   over time. Desugaring that assumes its input has passed valid_st1 can
   focus on transformations instead of checking what it has at every step.
3. The producer of useful user-facing errors for bad macro (or parser) output.

valid_st1 is complete enough to pass all JuliaLowering tests, and I've set it
up to run right before desugaring. Some error messages have changed (some a bit
better, others a bit worse). We can work on more helpful errors over time.

valid_st0 is also provided (nearly the same AST, but quotes and macros are
unexpanded). This is also necessary as a callee of valid_st1 since embedded
module and toplevel in a macro-expanded expression should not be expanded.

These functions only check the JuliaSyntax/JuliaLowering-produced AST, which is
different from the Expr AST, but only by rearranging Expr in a few well-defined
cases. If we wanted a version checking the Expr AST, most of the current
version of validation.jl could be taken and used.

@stm "syntax tree match"

This PR also contains a SyntaxTree pattern-matching macro, inspired by racket
match, but much simpler.

This is to keep validation functions grammar-like, and to keep the amount of
validation code manageable. Much more detail is available in the docstring.

This macro is arguably even better for post-validation desugaring since the
structure of its inputs are known.

Here's an example of a desugaring helper we could refactor:

# Match `x<:T<:y` etc, returning `(name, lower_bound, upper_bound)`
# A bound is `nothing` if not specified
function analyze_typevar(ctx, ex)
    k = kind(ex)
    if k == K"Identifier"
        (ex, nothing, nothing)
    elseif k == K"comparison" && numchildren(ex) == 5
        kind(ex[3]) == K"Identifier" || throw(LoweringError(ex[3], "expected type name"))
        if !((kind(ex[2]) == K"Identifier" && ex[2].name_val == "<:") &&
             (kind(ex[4]) == K"Identifier" && ex[4].name_val == "<:"))
            throw(LoweringError(ex, "invalid type bounds"))
        end
        # a <: b <: c
        (ex[3], ex[1], ex[5])
    elseif k == K"<:" && numchildren(ex) == 2
        kind(ex[1]) == K"Identifier" || throw(LoweringError(ex[1], "expected type name"))
        (ex[1], nothing, ex[2])
    elseif k == K">:" && numchildren(ex) == 2
        kind(ex[2]) == K"Identifier" || throw(LoweringError(ex[2], "expected type name"))
        (ex[1], ex[2], nothing)
    else
        throw(LoweringError(ex, "expected type name or type bounds"))
    end
end

Assuming the AST is valid, we can reduce this to the following. Note that
adding the comparison case with ">:" is a bugfix.

analyze_typevar_refactored(ctx, ex) = @stm st begin
    [K"Identifier"] -> (ex, nothing, nothing)
    ([K"comparison" lb op t _ ub], when=op.name_val==="<:") -> (t, lb, ub)
    ([K"comparison" ub op t _ lb], when=op.name_val===">:") -> (t, lb, ub)
    [K"<:" t ub] -> (t, nothing, ub)
    [K">:" t lb] -> (t, lb, nothing)
end

For the curious, the relevant validation function looks something like:

vst1_typevar_decl(vcx, st) = @stm st begin
    [K"Identifier"] -> true
    [K"<:" [K"Identifier"] old] -> vst1_value(vcx, old)
    [K">:" [K"Identifier"] old] -> vst1_value(vcx, old)
    ([K"comparison" old1 [K"Identifier"] [K"Identifier"] [K"Identifier"] old2],
     when=st[2].name_val===st[4].name_val && st[2].name_val in ("<:", ">:")) ->
         vst1_value(vcx, old1) & vst1_value(vcx, old2)

    [K"<:" x _] -> fail(vcx, x, "expected type name")
    [K">:" x _] -> fail(vcx, x, "expected type name")
    [K"comparison" _...] -> fail(vcx, st, "expected `lb <: type_name <: ub` or `ub >: type_name >: lb`")
    _ -> fail(vcx, st, "expected type name or type bounds")
end

RFC: Interaction with "strict mode"

If I understand the discussions on a potential strict mode correctly, this PR solves
a different problem. Strict mode would disallow specific inputs that can
produce undesirable behaviour (but not undesirable to enough people to be
completely disallowed). This PR aims to define the AST-space that strict mode
operates in.

This is still somewhat related to strict mode since valid_st1 would be the
easiest place to pass in options and enforce any AST-related rules. However, I
think we should avoid this for now given our improved grip on the AST structure.
Can we instead take all questionable or ambiguous syntax and disallow it in a
future AST version instead? See the section below.

RFC: Syntax evolution

I'd like to make non-macro-breaking AST evolution possible. I suggest we have
validation.jl serve as the specification of an AST version and write a pass
transforming each AST of version n to version n+1, which runs after
expanding macros that expect an older AST. This assumes a syntax version is
defined at the project level and tied to a major version with no mixtures of
macros expecting different AST versions. We may want something more complex.

Note that many discussions on "syntax evolution" are about changes in parsing,
which would need a separate versioning mechanism. Maybe each parser version
targets one AST version, and the user-facing syntax version is just the parser
version?

Rust's editions are usually mentioned here. I'm not familiar with this
mechanism, but I know it's well-regarded and that I should do some research here
before making any more suggestions.

Potential next steps

• There are many places the AST can be improved (see AST for macro writers and lowering #77 for some). We don't
  need AST evolution as described above until macros start taking SyntaxTree, so
  now is the time to break and improve things.
• Not urgent, but we can start simplifying desugaring based on the invariants we
  get from validation
• (Probably after JuliaLowering has moved to the julia repo) Set up a pkgeval
  that runs all code through valid_st1 before flisp lowering to make sure
  valid_st1 actually allows all syntax we plan to support. I'm prepared to
  add nasty we-should-have-never-allowed-this cases here.
• It's definitely more important to have a definition/validator for the IR, but a
  few practical reasons made me wait:
  • There is no easy way to get "reference" IR in SyntaxTree form; we would need
    to either live without pattern matching, or flisp-lower and write
    Expr->SyntaxTree for lowered code.
  • We already have some amount of IR validation in Base
  • I know less about what constitutes valid IR
• Write a fuzzer based on the AST description

Misc

Why now?

I think writing down our input language will help us achieve compatibility with
existing code. We'll likely see some crazy broken syntax in our long tail of
testing. Validation will let us properly disallow it with a useful error,
provide a place to write weird syntax down if we plan to support it, and
hopefully (through syntax evolution) let us disallow it in future versions.

Alternative approach

I considered an alternative valid_st1 that isn't responsible for generating
nice user-facing errors, which can simply return a boolean and describe errors
if it's easy to do so. This approach could keep valid_st1 simpler and more
grammar-like---we can just describe the set of valid ASTs, return false eagerly
on error, and not waste bytes figuring out or reporting what exactly went wrong.
This would be optimal for a human-readable specification. However, bytes not
wasted on disambiguating and describing errors still need to exist in the first
lowering pass, and we wouldn't get most of the benefits of this PR.

Still, I'd like to keep the main part of each validation function free of
error-determination code to keep it more like a grammar, even if it means
redoing some work in the error case. This is so someone using it as a reference
can immediately see what's allowed, and doesn't need to read it top-to-bottom.
I haven't succeeded in all cases.

[topolarity]

Many of these messages got better / clearer, but several seem to be somewhat worse now, in the sense that they either expose the user to more syntax- / lowering-isms (e.g. "bracescat" / "keyword-separating" / "kind") or refer to less specific constructs (e.g. "this syntax" vs. "`module`")

Do you think we can improve that?

[mlechu]

Yes, I should tweak some of the errors. Some of the worse ones including ccall and friends are because pre-desugaring (random AST tweaks performed at the same time as macro expansion) changes them from identifiers to a different kind K"core", which we would need to check for everywhere. I think we should move away from pre-desugaring instead.

"keyword-separating" is an attempt at clarity given many valid uses of semicolons outside of calls

Errors like this one (reachable via keyboard mash and parse) should probably avoid syntax-isms, though exposing more of them was intentional in some cases. A lot of the vaguer errors will only be seen by macro writers, and we point at source text anyway, so repeating the exact text in the error is less necessary than before.

I can bring back the old error in this case. It doesn't feel super helpful given that I don't know what the "user" intended, but it does seem better than the default.

[c42f]

I don't think we need run? We have normal semicolon separated block syntax for a more general version of that, as long as we specify that any variables defined in the when clause are also available in the body:

    ([K"=" [K"call" _...] _...], when=(run=if_valid_get_args(st[1]); !isnothing(run)) ->
        "deprecated call-equals form with args $run"

[mlechu]

It isn't needed, but removing it doesn't really simplify the implementation (basically when=(code; true)), and it's convenient for revise-repl-printf debugging.

[c42f]

I guess my thought was that I'd prefer to read code looking like the following rather than needing to use the special variable name run:

    ([K"=" [K"call" _...] _...], when=(args=if_valid_get_args(st[1]); !isnothing(args)) ->
        "deprecated call-equals form with args $args"

maybe I'm misunderstanding what it's for

[mlechu]

I'm good with removing the assignment part

[c42f]

Unfortunately, yes, due at least to the .. operator (maybe there's no other cases):

import A.:(..)

In future syntax evolution, I'd like to argue that this should be deprecated in favor of

import A.var".."

Julia has other cases where "quoted for no reason" occurs and is allowed by lowering, but is semantically quite questionable and should be deprecated if possible. For example, defining methods for operators

function Base.:(+)(x, y)
    body
end

Possibly we could actually fix this with the new parser. If not, we could specify that var should be used, as that's much more semantically reasonable than a random quote node where an identifier is expected.

[c42f]

The pattern matching here is cool, I think it could even be useful to get it in as a separate PR - maybe with a couple of uses in desugaring to test out the benefits. (I assume it's a zero cost abstraction? It certainly seems like it can be.)

I also think it's excellent to have an executable specification of what's valid input AST. I love this.

However, I do have some design misgivings about the details - it feels rather like a repetition of large parts of the matching code within desugaring, even though in a nicely condensed form. Also it contains (IIUC?) random pieces of logic which are currently in other passes, such as const not being valid in local scopes and global not being valid in value position. Those things seem to require correctly modeling some parts of the later passes of lowering but on the non-simplified surface AST. In the past, we've found that to be bug prone (see the many bugs which resulted from trying to model hygiene in vars-introduced-by-patterns in macroexpand.scm). Perhaps that's not a problem if we limit the types of validation we're doing to things which are "easy enough" to do on the surface AST, but it does make me worry we can end up in a similar situation.

I like that you've put proper error reporting in here. That seems really important to make this practical for end users rather than macro authors - keeping in mind that non-macro-authors are the vast majority of Julia users. For error reporting I think we need the principle that "if the parser can produce a form, we should assume an end user will see the error", and go to the effort of trying to write good messages. With that in mind, there's a fair few cases here where I think the errors got worse because we now talk about the names of Kinds. I do think we should avoid that where possible when expressions are likely to be generated directly from the source code. A particular example is the expression [a, b; c] which is extremely likely to be typed by a user given that [a,b] and [a b; c] are both valid code. In general, the distinction of "end user vs macro author" is the distinction I currently give in desugaring between writing a custom message vs just using @chk without any special message.

In the current state, I'm feeling this is ok, but also I feel like it doesn't hit satisfying tradeoffs in terms of avoiding duplication with the rest of lowering. However, I have a few thoughts about future design/implementation directions which might get us from "eh that's implicitly a lot of code duplication" to "wow this was a great improvement":

1. As an executable specification, can we use this pattern matching to drive desugaring itself? This would alleviate a lot of my concerns about duplicating this logic and about having two different specifications to maintain and synchronize (one within the validation code and one implicit in the recursion of desugaring).
2. Can we use it to produce a "`K"error"-poisoned but structurally valid" AST, such that desugaring can run easily over that AST, allowing us to implement Error propagation via AST #29 without tons of custom matching?

Even better if we can combine both of these ideas.

[aviatesk]

You understand far better than me how lowering should work, so my comments might not be super helpful. But with that disclaimer, I'd still like to share my thoughts.

I think the basic idea of this PR is to separate validation from transformation and have the validation act as a readable specification. But the implementation still ends up being recursive, and from a general user's perspective, I think this validation (even if it looks much easier to read than desugaring, probably thanks to the beautiful pattern matching system) would still be hard to use directly as a specification? In many cases, users will probably still rely on error messages.

As an executable specification, can we use this pattern matching to drive desugaring itself?

So I agree with @c42f's point above.
From an implementation perspective, separating validation and transformation seems to lead to unavoidable code duplication (even if pattern matching abstracts most of it, structural duplication might still be there AFAIU). So, merging them might be better in terms of implementation simplicity. Even if we merge them, the pattern matching syntax from this PR could still greatly improve readability. (Maybe if we use agents nicely, we could even generate a human-readable specification from a more readable desugaring implementation? IDK)

Also, if we think about implementing Claire's point (2), we'd likely need to make use of validation information during transformation. So, based on a rough implementation sketch I can think of right now, doing both in one place would probably result in a simpler implementation.

But other points @mlechu brought up might need more thought.

The checking here is especially unwanted given that much of desugaring's input is output from other desugaring functions (related to (4) below), which shouldn't go through the same level of checking as arbitrary user input.

Regarding this, my understanding is that doing validation first can help avoid unnecessary validation when recursive transformations happen in the transformation phase. From a performance perspective, though, I don't think the order of nodes explored will change, so there might not be a huge performance difference. Or maybe code generated by macros needs fundamentally different validation/transformation than regular source input? I do think we need to improve validation messages for invalid code generated from macro contexts. JL already keeps the macro expansion context, so I believe we can use that to dispatch appropriate messages even with a one-path validation implementation.

How each expression gets through desugaring is generally unknown.

This point is very interesting. Does avoiding this problem mean we need to do extensive validation as a separate pass first? The simplest (naivest) solution I can think of is to add an attribute to nodes that have already been desugared and use that flag to decide if desugaring is still needed. However, we might need to think about how this changes the complexity compared to doing validation as a separate pass first.

[mlechu]

(I assume it's a zero cost abstraction? It certainly seems like it can be.)

Pattern-matching can be faster than the equivalent if chain with a smarter implementation! Right now, @stm produces the easiest possible expansion though.

In the current state, I'm feeling this is ok, but also I feel like it doesn't hit satisfying tradeoffs in terms of avoiding duplication with the rest of lowering.

To be clear, my intention with this PR is to delete more than its weight in desugaring code. As satisfying as that would be now, we're actively trying to achieve parity with flisp, so I want to wait before doing that.

1. As an executable specification, can we use this pattern matching to drive desugaring itself? This would alleviate a lot of my concerns about duplicating this logic and about having two different specifications to maintain and synchronize (one within the validation code and one implicit in the recursion of desugaring).

I may have failed to argue my point. I don't think validation and desugaring should happen in the same pass. Desugaring often needs to eat its own output, which doesn't need to follow the rules of its input, and doesn't need to go through validation.

Squeezing desugaring into the framework in validation.jl would also be much more painful than bringing @stm and invariants from validation into desugaring piece by piece.

I don't think desugaring should be an AST specification; the aim is to have one AST specification that macro authors follow (or see errors about) and desugaring gets to assume its inputs follow. This isn't the same as keeping two specifications synchronized. If desugaring handles a superset of the specification, we don't need to care; the validator has run. This way, we don't get the "how did this fall through lowering?" or "macro author found a clever trick: produce a lowering-internal form" situations we know and love from flisp.

There would be code duplication with this approach, but the current state is the most duplicated it can be (all destructuring and error checking implemented twice). Things will only shrink from here, especially if we manage to simplify the AST and desugaring.

2. Can we use it to produce a "`K"error"-poisoned but structurally valid" AST, such that desugaring can run easily over that AST, allowing us to implement Error propagation via AST #29 without tons of custom matching?

I don't think we should do this, and would at least like to wait before trying it out. The benefit of partial lowering doesn't seem worth the cost of every node now being valid or an error node, which sounds like "two cases" but actually means "two cases, per place in the SyntaxTree, per lowering pass." I fear it would be like Expr(:escape) wrappers in expr_to_syntaxtree or LineNumberNode in macros, only less polite about where it appears and much less clear what the semantics of it are.

For error reporting I think we need the principle that "if the parser can produce a form, we should assume an end user will see the error", and go to the effort of trying to write good messages.

Agree; some thoughts are in this conversation. I still need to bring some validation.jl errors up to baseline, but the goal is for valid_st1 to be our tool to produce fancier errors in the future.

[c42f]

I may have failed to argue my point. I don't think validation and desugaring should happen in the same pass. Desugaring often needs to eat its own output, which doesn't need to follow the rules of its input, and doesn't need to go through validation.

I'm actually not arguing that validation and desugaring should happen in the same pass :-)

Instead, what I'm hoping/dreaming of is using one canonical set of patterns to drive both desugaring and validation.

For example, matching all the variants of function signatures is an annoying piece of code which both validation and desugaring need to do. It would be wonderful if they could both share the same matching code. The tricky thing about this vague dream is that matching comes in two parts (1) the pattern specification and (2) the actions to execute when patterns match. The patterns might be shared, but the actions very much are not. The big design unknown with this idea is whether there's a convenient way to factor the actions out separately. If not, the idea is doomed.

Desugaring often needs to eat its own output

It currently does, though I feel this is not ideal. It might be feasible to stop doing this and there's a few ideas we could try. But it's definitely a bunch of work. Some ideas:

• Use different kinds internally within lowering to the kinds of the input. (Downside: a proliferation of kinds with almost the same semantics). The idea of using an attribute for these instead could possibly help, but there's some complexity associated with that as well.
• Define helper functions with restricted expansion semantics, and pass pieces of AST to them rather than generating a big blob of new AST and passing that to the outer expand_forms_2. For example, if we currently generate a new assignment lhs = rhs and call generic expand_forms_2 on that, maybe we should instead pass it to a slightly refactored expand_assignment(ctx, srcref, lhs, rhs) rather than making a new AST and passing it to the outer expansion function.

At the outset I favor the second of these ideas but it could be a lot of refactoring. Would need some prototyping to test how feasible it is.

[c42f]

I fear it would be like Expr(:escape) wrappers in expr_to_syntaxtree or LineNumberNode in macros, only less polite about where it appears and much less clear what the semantics of it are.

Right, so addressing this concern is what I mean by "structurally valid" in "K"error"-poisoned but structurally valid". The goal is to enforce "enough" structural validity in the AST up front precisely so that future passes can always match the structures they expect. For example, if the input AST is [K"=" lhs] (ie, missing the rhs) and we expand this into [K"=" lhs [K"error"]], desugaring would be able to run without issues. My intuition is that desugaring would still need minor adjustments but they wouldn't be intrusive.

[mlechu]

Instead, what I'm hoping/dreaming of is using one canonical set of patterns to drive both desugaring and validation.

I see, and agree this would be nice if a convenient factoring exists.

Define helper functions with restricted expansion semantics, and pass pieces of AST to them rather than generating a big blob of new AST and passing that to the outer expand_forms_2

This is the goal! I think knowing what our input looks like will help. Producing surface AST and re-desugaring is convenient, but we could probably take a more direct route through desugaring in some places.

Knowing what desugaring's output looks like would also help, but doesn't feel critical given that it's only a problem for us, and not macro authors. If we do end up wanting a similar tree specification for intermediate trees, it would make sense to take the alternate simple-errors approach from above and only run it in testing.

For example, if the input AST is [K"=" lhs] (ie, missing the rhs) and we expand this into [K"=" lhs [K"error"]], desugaring would be able to run without issues. My intuition is that desugaring would still need minor adjustments but they wouldn't be intrusive.

If we receive a tree [K"=" something], we can't assume something was meant to be on the left or right. I think lowering should just throw an error.

If you mean to only allow K"error" in generic-statement-or-value position (i.e. not a function name, foreigncall name, assignment lhs, loop iteration spec, etc), I agree it wouldn't be as disruptive to further passes. I'd still prefer "partial lowering" be the responsibility of the consumer of it, with our responsibility being to either figure out where the AST is broken, or produce correct output (both still WIP).