Improve clarity of `ast.rs` for newcomers to the codebase by wrzian · Pull Request #5784 · typst/typst

wrzian · 2025-01-30T06:12:54Z

This is a set of small changes to the typst-syntax/src/ast.rs file designed to improve clarity of the code for newcomers. I personally had a lot of trouble understanding this file the first time I encountered it (many months ago), and wanted to document its behavior and make some improvements while it's still top-of-mind.

Add big comment and require the struct keyword in the node! macro (e.g. node! {Raw} vs. node! {struct Raw}). I think my original confusion with ast.rs was with where all of the data-types were declared. The way enums were laid out combined with node! using only the SyntaxKind name was unhelpful. Adding struct makes it feel much clearer to me that node! is generating new types. And the added comment on node! should also be a nice improvement.
Removed comments on the enum variants of Expr (the structs they contain are already commented) and renamed the variants that didn't match their SyntaxKind names.
Changed the calls to node.cast().map() in from_untyped for the various implementations to construct the node structs more directly. The current node.cast().map() requires type inference that may be obvious to the compiler, but is not obvious to humans on first reading. It does reduce the dependency on the name of each node kind, but I think the current syntax obscures the real operation more than is worth avoiding one extra repeated name in each line. There are still calls to node.cast().map() in the from_untyped impl, but only in wildcard matches, where it should be easier to figure out what the desired effect is.

I'm very open to nitpicks or rewordings of the main long doc comment in commit 1. But I think it does roughly what it needs to atm.

Also fine with backing out any of these changes or maybe putting the module comment somewhere else. But my main goal is to improve clarity for newcomers to the codebase, and I think these changes should help with that.

T0mstone

The second commit also has a typo ("macth" instead of "match")

crates/typst-syntax/src/ast.rs

PgBiel

Great job, thank you! Here are some of my thoughts.

crates/typst-syntax/src/ast.rs

wrzian · 2025-02-08T18:14:13Z

I pushed the rewrites and also 4 small changes:

I made the conversion of SyntaxKind::Space to None more explicit in Expr::from_untyped, since it's an important edge case that doesn't deserve to be part of the wildcard match.
I moved the generic SyntaxNode casting impls from node.rs to ast.rs to improve locality. I also made cast_first_match and cast_last_match private when doing so, although if there are other crates that need them, we can revert that.
I added two new functions get_first() and get_last() to replace and shorten the very common idiom of cast_first_match().unwrap_or_default() which I think goes along with improving clarity.
I simplified some AST methods that seemed straightforward to improve.

crates/typst-syntax/src/ast.rs

Co-authored-by: PgBiel <9021226+PgBiel@users.noreply.github.com> Co-authored-by: T0mstone <39707032+T0mstone@users.noreply.github.com>

…ap_or_default()` calls

wrzian · 2025-02-25T22:31:31Z

Alright, rebased off main and tried to update to match the new feedback, I also reorganized the commits again so they should be more atomic.

There are still a few unresolved comments I'd like responses to, but I'm otherwise happy with this.

Thanks everyone for the feedback! I appreciate the help :)

laurmaedje · 2025-02-26T18:42:35Z

Looks good to merge for me.

There are still a few unresolved comments I'd like responses to, but I'm otherwise happy with this.

Which? I see only this one, but I think the wording is fine tbh.

wrzian · 2025-02-26T19:39:42Z

That quote was mainly for the other comment that we already resolved. (#5784 (comment))

laurmaedje · 2025-02-26T20:10:43Z

Okay. Then, thank you!

Co-authored-by: PgBiel <9021226+PgBiel@users.noreply.github.com> Co-authored-by: T0mstone <39707032+T0mstone@users.noreply.github.com>

* build: use nightly typst, typst-ansi-hl, typstyle, reflexo; update ttf-parser, fontdb * refactor: new eval_string api * refactor: typst::html -> typst_html fixup * refactor: new Source lines api in typst/typst/6308 * refactor: new ast::Expr enum names in typst/typst#5784 * refactor: new color api in typst/typst#6479 * refactor: new element api in typst/typst#6547 * refactor: new html frame api in typst/typst#6505 * refactor: new label constructing api in typst/typst#6332 * refactor: new symbol api in typst/typst#6441 * fix: catch_unwind when printing Ty's debug format * styl: fmt * refactor: use new LibraryExt mod fixup 2 * fix: increase recursion limit for tinymist-query * tests: update test snaps * ci: continue on error * build: bump world crates to 0.13.15-rc1 * build: pin reflexo, typstyle fixup 3 * build: bump to 0.13.15-rc1 * docs: changelog * ci: update windows version

* build: use nightly typst, typst-ansi-hl, typstyle, reflexo; update ttf-parser,fontdb * refactor: new eval_string api * refactor: typst::html -> typst_html * refactor: new Source lines api in typst/typst/6308 * refactor: new ast::Expr enum names in typst/typst#5784 * refactor: new color api in typst/typst#6479 * refactor: new element api in typst/typst#6547 * refactor: new html frame api in typst/typst#6505 * refactor: new label constructing api in typst/typst#6332 * refactor: new symbol api in typst/typst#6441 * fix: catch_unwind when printing Ty's debug format * styl: fmt * refactor: use new LibraryExt mod * fix: increase recursion limit for tinymist-query * tests: update test snaps * ci: continue on error * deps: update rustc * tests: update test snaps * deps: patch reflexo * refactor(typlite): replace manual highlight and strike with official impl * tests: update test snaps for shortened empty attributes syntax * tests(typlite): update test snaps to ignore outline temporarily * fix(typlite): convert link * feat: auto release nightly * feat: auto tag stable release

T0mstone reviewed Jan 30, 2025

View reviewed changes

crates/typst-syntax/src/ast.rs Outdated Show resolved Hide resolved

crates/typst-syntax/src/ast.rs Outdated Show resolved Hide resolved

wrzian closed this Jan 30, 2025

wrzian reopened this Jan 30, 2025

PgBiel reviewed Jan 30, 2025

View reviewed changes

wrzian force-pushed the ast-clarity branch 3 times, most recently from f8bd422 to 9046fef Compare February 8, 2025 17:34

laurmaedje reviewed Feb 24, 2025

View reviewed changes

wrzian force-pushed the ast-clarity branch from 9046fef to 4e8aadc Compare February 25, 2025 22:21

wrzian and others added 6 commits February 25, 2025 17:24

require struct in ast::node!

cb98173

update ast.rs module doc comment

99a0786

Co-authored-by: PgBiel <9021226+PgBiel@users.noreply.github.com> Co-authored-by: T0mstone <39707032+T0mstone@users.noreply.github.com>

rename Expr enum variants to macth SyntaxKind names

7cd486c

update ast:from_untyped impls to simplify node.cast().map() calls

3d8b657

make Expr::Space in from_untyped more explicit

a9a9beb

move SyntaxNode casting functions into ast.rs

5bedcfe

wrzian force-pushed the ast-clarity branch from 4e8aadc to e19091b Compare February 25, 2025 22:25

wrzian added 2 commits February 25, 2025 17:28

rename casting functions and add cast_{first,last} to replace `unwr…

38b4fb0

…ap_or_default()` calls

simplify two uses of try_cast_*()

56b3c73

wrzian force-pushed the ast-clarity branch from e19091b to 56b3c73 Compare February 25, 2025 22:28

laurmaedje approved these changes Feb 26, 2025

View reviewed changes

laurmaedje added this pull request to the merge queue Feb 26, 2025

Merged via the queue into typst:main with commit cfb3b1a Feb 26, 2025
6 checks passed

wrzian deleted the ast-clarity branch February 27, 2025 16:35

ParaN3xus added a commit to ParaN3xus/tinymist that referenced this pull request Jul 15, 2025

refactor: new ast::Expr enum names in typst/typst#5784

00abc26

ParaN3xus added a commit to ParaN3xus/tinymist that referenced this pull request Jul 15, 2025

refactor: new ast::Expr enum names in typst/typst#5784

c9a9db5

ParaN3xus added a commit to ParaN3xus/tinymist that referenced this pull request Jul 16, 2025

refactor: new ast::Expr enum names in typst/typst#5784

ac81101

ParaN3xus added a commit to ParaN3xus/tinymist that referenced this pull request Jul 28, 2025

refactor: new ast::Expr enum names in typst/typst#5784

78ad882

ParaN3xus added a commit to ParaN3xus/tinymist that referenced this pull request Jul 30, 2025

refactor: new ast::Expr enum names in typst/typst#5784

d1d3054

ParaN3xus added a commit to ParaN3xus/tinymist that referenced this pull request Jul 31, 2025

refactor: new ast::Expr enum names in typst/typst#5784

e02dcb7

ParaN3xus added a commit to ParaN3xus/tinymist that referenced this pull request Jul 31, 2025

refactor: new ast::Expr enum names in typst/typst#5784

c09f1c2

ParaN3xus added a commit to ParaN3xus/tinymist that referenced this pull request Jul 31, 2025

refactor: new ast::Expr enum names in typst/typst#5784

88c96a8

Uh oh!

Conversation

wrzian commented Jan 30, 2025

Uh oh!

T0mstone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PgBiel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wrzian commented Feb 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wrzian commented Feb 25, 2025

Uh oh!

laurmaedje commented Feb 26, 2025

Uh oh!

wrzian commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laurmaedje commented Feb 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wrzian commented Feb 26, 2025 •

edited

Loading