ast: introduce Parse trait (big refactor) #11807

The0x539 · 2025-09-17T00:49:20Z

Description

Yeah, this is pretty big. I wasn't able to identify a good "midway checkpoint", and I definitely regret not rejoining the chatroom before/during my work to discuss it. I hope this goes over well regardless.

The refactors to this module while I was away were very nice, and definitely helped pave the way for these changes (especially getting rid of parent pointers).

I'm perfectly willing to squash+rebase if/when this is just about ready to merge, but I'm keeping it as the original commits at first on the off chance it helps whatsoever with reading the diff.

Big-picture summary:

The FreestandingArgumentList type is gone. Only the freestanding_arguments field on Populator remains.
List nodes have been demoted from structs to humble typed aliases for boxed slices. Not only does this reduce code complexity, it allows for less indirection, most visibly in the Kind enum.
Migrated from a fn visit_mut(populator: &mut Populator, node: &mut Node) pattern to fn parse(pop: &mut Populator) -> Node. KindMut, NodeVisitorMut, and many other mut-flavored ast internals evaporated as a result.
- Parsing of leaf nodes got a fair bit more straightforward.
- Parsing of branch nodes still has a control flow reminiscent of the old style, due to the parser's unique error handling mechanism, but the dispatch for traversing the tree while constructing it should all be static now.
~~The trait-impl macros are now derive-style while still being defined with macro_rules!, with the help of macro_rules_attribute.~~ (Isolated to smaller PR and merged.)
JobList is still a list of JobConjunctions, but that might be an easier situation to unravel now.

TODOs:

(No intended user-facing changes.)

krobelus · 2025-09-25T11:32:36Z

Cargo.lock

 "fish-printf",
 "libc",
 "lru",
+ "macro_rules_attribute",


I'm perfectly willing to squash+rebase if/when this is just about
ready to merge, but I'm keeping it as the original commits at
first on the off chance it helps whatsoever with reading the diff.

We don't treat pre-commit and post-commit review differently.
We optimize for both the same way. We're not gonna do a big-bang squash
but rather move (incrementally) towards a history that is good enough
and merge that.

krobelus · 2025-09-25T11:32:36Z

Cargo.lock

 ]

+[[package]]
+name = "macro_rules_attribute"


I have not finished reviewing everything from
"ast: initial Parse trait rework" onwards,
I suspect these things can be made a bit easier to review
(the fact that there's things like back and forth renames left in history makes
me careful about investing time in review until these issues are fixed.
There might be other ways to efficiently explain the Parse-trait changes to
me but minimal and atomic diffs are a tried and tested way.)
So I'm leaving some comments now.
What I've read through looks good.

krobelus · 2025-09-25T11:32:36Z

src/ast.rs

-    fn kind(&self) -> Kind<'_>;
-
-    /// Return the kind of this node, as a mutable reference.
-    fn kind_mut(&mut self) -> KindMut<'_>;


7ecd2d0 (ast: move kind_mut back into AcceptorMut, 2025-09-03)
introduces LIST_KIND only to delete it later. Should probably
remove it from history outright (to make the sum of deltas smaller).

Also, this seems to be doing multiple things at once:

moving kind_mut() to AcceptorMut

replacing empty list objects with LIST_KIND

demoting JobList? (does that belong to a different one?)

Can it be split up to separate conerns?

krobelus · 2025-09-25T11:32:36Z

src/ast.rs

-    JobList(&'a JobList),
-}
-
-pub enum KindMut<'a> {


stray intermediate println!("parsing an arg"); in this commit

krobelus · 2025-09-25T11:32:36Z

src/ast.rs

-
-/// A freestanding_argument_list is equivalent to a normal argument list, except it may contain
-/// TOK_END (newlines, and even semicolons, for historical reasons).
-/// In practice the tok_ends are ignored by fish code so we do not bother to store them.


it's weird that there was no difference to ArgumentList,
because ArgumentList definitely should not contain newlines or semicolons.
AFAICT that difference is not touched by your changes, so that's fine.

But we should definitely consider preserving the comment describing this historical weirdness.
Can you move the comment to fn parse_argument_list ?

krobelus · 2025-09-25T11:32:36Z

src/ast/macros.rs

@@ -0,0 +1,327 @@
+/// Implement the node trait.
+macro_rules! Node {


I haven't yet finished reviewing,
so I don't know what the final module layout should look like,
but I'm not sure if the benefit to moving macros to a separate
file is worth tearing it apart from ast.rs.
IIRC, none can live without the other, so I'm not sure if it helps.

Of course we do want to look at extracting a fish-syntax crate in
future, which should be able to parse fish code.
So we should do whatever helps but this seems unrelated

krobelus · 2025-09-25T11:32:36Z

src/ast/macros.rs

+            $contents:ty => $name:ident {
+                chomp_newlines: $newlines:expr,
+                chomp_semis: $semis:expr,
+                $(stop_unwind: $unwind:expr,)?


The name of this is let $pop_:ident; in an earlier commit, which
is confusing to read. Can you fix that, i.e. use the better name from
the start?

krobelus · 2025-09-25T11:32:37Z

src/ast/traits.rs

+    fn kind(&self) -> Kind<'_> {
+        N::list_kind(self)
+    }
+}


(Just a brain dump:)
I wonder if we really want every Box<[N]> to implement Node?
That would make it easy to construct a new list node from existing
nodes, without going through the parser.
I guess that's not a problem in practice.
But if we actually wanted to guard this, we could use the newtype
pattern (similar to what we had before, but make it generic, i.e.

struct List<N> ( Box<[N]> )

krobelus · 2025-09-25T11:32:37Z

src/builtins/fish_indent.rs

                .is_err()
            {
-                for job in &brace_statement.jobs {
+                for job in &brace_statement.jobs[..] {


this (a0d9c23 (ast: fix MSRV breakage, 2025-09-11))
should be squashed, so that intermediate commits still build with MSRV

krobelus · 2025-09-25T11:32:37Z

src/tests/parser.rs

        assert_eq!(line_offset, expected);
    }

-    let pipelines: Vec<_> = ps.ast.walk().filter_map(ast::JobPipeline::cast).collect();


this commit should be squashed into the commit that removes Castable
(or precede that one),
so that every commit improves consistency

The0x539 · 2025-10-03T08:43:00Z

Apologies for my absence. I've been meaning to get back to this, but my attention has been elsewhere and difficult to wrangle.

Regarding the final state of the code:

it's weird that there was no difference to ArgumentList,

Yes, I was also surprised by this. It seems that the important behavior has actually been governed by the freestanding_arguments field for whatever amount of time. Either way, the tests still pass, and if we're not confident, we can add more test coverage. (I'd have maybe done so myself if I had a better understanding of the behavior at hand.)

I'm not sure if the benefit to moving macros to a separate file is worth tearing it apart from ast.rs.
IIRC, none can live without the other, so I'm not sure if it helps.

In my personal experience, ast.rs was a long enough file that it was pretty difficult to work with, cognitively. Splitting it up into a couple sub-concerns helped quite a bit. I want there to be a clear place to go to see the structs that make up the AST and the relationship between them, but that's mixed in with all the Populator stuff and the macros with their really dense syntax.

I wonder if we really want every Box<[N]> to implement Node?

I think the NodeKind enum, and the fact that every &impl Node can be converted to one, is the limiting factor for being able to do this.

Regarding the commit history:
This is kind of what's kept me away from this PR for the past week. I very much appreciate all the scrutiny, and I'd love to have a cleaner history for this that can be merged as-is, but actually the prospect of actually doing that tests the limits of my Git abilities - really, I'm not sure where to even start.
I'd appreciate some help on that front, so as to avoid needing to remake all the changes by hand on a fresh branch.

krobelus · 2025-10-03T12:13:03Z

src/tests/parser.rs

+        .ast
+        .walk()
+        .filter_map(|node| node.kind().try_into().ok())
+        .collect();


Regarding the final state of the code:

it's weird that there was no difference to ArgumentList,

Yes, I was also surprised by this. It seems that the important behavior
has actually been governed by the freestanding_arguments field for
whatever amount of time. Either way, the tests still pass, and if we're
not confident, we can add more test coverage. (I'd have maybe done so
myself if I had a better understanding of the behavior at hand.)

if they are using the same code, there's nothing to test.
IIRC that commit was obviously correct, maybe you can send a separate PR with only that.

I'm not sure if the benefit to moving macros to a separate file is worth tearing it apart from ast.rs.
IIRC, none can live without the other, so I'm not sure if it helps.

In my personal experience, ast.rs was a long enough file that it was
pretty difficult to work with, cognitively.
Splitting it up into a couple
sub-concerns helped quite a bit. I want there to be a clear place to go to
see the structs that make up the AST and the relationship between them,
but that's mixed in with all the Populator stuff and the macros with
their really dense syntax.

Hmm, I'd need to take a look.
Whatever the outcome may be, extracting this code to a different file is unrelated to the main point of the PR, so to make review easier and faster, move it to a separate PR.

I wonder if we really want every Box<[N]> to implement Node?

I think the NodeKind enum, and the fact that every &impl Node can be
converted to one, is the limiting factor for being able to do this.

but we could hide the fact behind a newtype wrapper around Box<[N]> I tried to sketch.
Then other modules could not directly create a new AST node by putting that into a box

Regarding the commit history:
This is kind of what's kept me away from this PR for the past week. I very much appreciate all the scrutiny, and I'd love to have a cleaner history for this that can be merged as-is, but actually the prospect of actually doing that tests the limits of my Git abilities - really, I'm not sure where to even start.
I'd appreciate some help on that front, so as to avoid needing to remake all the changes by hand on a fresh branch.

For one thing, create minimal PRs, then things become easier.
For example move the first three commits to their own PR. Worry about the rest later.

some helpful docs for history rewriting are:

https://git-rebase.io/

https://jj-vcs.github.io/jj/latest/ as alternative to Git

(note that you don't have to use Git; in theory you could do everything with diff and patch)

Creating a fresh branch shouldn't be necessary because your changes are close to ready.
You can get there incrementally by tweaking/removing individual commits.

For example I'd start with getting the first three commits in,
while keeping your other changes rebased on top of that.
If you use git, that's maybe

git branch use-macro_rules_attribute 719de30e547 # (ast: use macro_rules_attribute for the Acceptor trait, 2025-09-03) # rebase to update to lastest origin/master git rebase --update-refs # interactive rebase; stop at the first three commits to address any review comments git rebase --update-refs --interactive git push $your_remote_name use-macro_rules_attribute gh pr create --head The0x539:use-macro_rules_attribute

When using "git rebase --update-refs --interactive"
to edit some earlier commits, then you might see some conflicts in later commits.
Take a careful look at each conflict (I recommend using "git config merge.conflictStyle diff3")
until you understand why it happened; then it should be easy how to resolve it.

An the beauty of this approach is that, conflicts only happen to a small minority of changes, where Git can't automatically do the right thing.
So this approach minimizes the time spent "re-doing things by hand".

Let me know what if anything you need.
History cleanup problems are always man-made, so they are always solvable.

krobelus · 2025-10-20T01:58:32Z

src/highlight/highlight.rs

        // If we have seen a "--" argument, color all options from then on as normal arguments.
        let mut have_dashdash = false;
-        for v in &stmt.args_or_redirs {
+        for v in &stmt.args_or_redirs[..] {


can you extract some more of the unrelated changes to separate PRs?
BTW we also bumped the MSRV, so the changes in a0d9c23 (ast: fix MSRV breakage, 2025-09-11) are probably no longer needed.

Very nice, that should be helpful.
And sorry, yes, I've been meaning to isolate more changes, but I've been quite busy with other things. Hope to make something happen this week.

zanchey added the cleanup label Sep 17, 2025

zanchey added this to the fish-future milestone Sep 17, 2025

krobelus reviewed Sep 25, 2025

View reviewed changes

krobelus reviewed Oct 3, 2025

View reviewed changes

The0x539 mentioned this pull request Oct 3, 2025

ast: use macro_rules_attribute for the trait macros #11867

Closed

The0x539 added 21 commits October 3, 2025 15:51

ast: Demote "list" node types to aliases

3c89a60

ast: Continue demotion using a ListElement trait

ce7d9cd

ast: kill the FreestandingArgumentList type

d10086c

ast: allocate_populate_list

fe83401

ast: move kind_mut back into AcceptorMut

47cf6b1

ast: fix MSRV breakage

b49125c

ast: move chomps_extras checks into ListElement trait

3051259

ast: add some Debug derives

851d3f2

ast: initial Parse trait rework

28b0353

ast: move special Parse impl from JobConjunction to JobContinuation

f870a8c

ast: reintroduce fallible vs. infallible parsing

02c683e

ast: move the big macros to their own module

1aecc62

parse_util: remove unnecessary is_same_node use

dc0b5f2

ast: convert token/keyword parsing to new style

b219706

ast: simplify define_list_nodes macro

b343360

ast: move the traits into their own module

1c5cc51

ast: separate Acceptor and Parse derive macros

d042c64

ast: add enum version of Acceptor derive

97192bf

ast: move parse module back into parent

9a93914

test: remove lingering use of Castable

84704db

ast: touch up comments

5c3010b

The0x539 force-pushed the parse-trait-draft2 branch from 5c30198 to 5c3010b Compare October 3, 2025 20:53

krobelus reviewed Oct 20, 2025

View reviewed changes

		@@ -0,0 +1,327 @@
		/// Implement the node trait.
		macro_rules! Node {

ast: introduce Parse trait (big refactor) #11807

Are you sure you want to change the base?

ast: introduce Parse trait (big refactor) #11807

Conversation

The0x539 commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODOs:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

The0x539 commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

The0x539 commented Sep 17, 2025 •

edited

Loading

The0x539 commented Oct 3, 2025 •

edited

Loading