Skip to content

fix(generate): use topological sort for subtype map#4280

Merged
WillLillis merged 1 commit intotree-sitter:masterfrom
WillLillis:super_sort
Jul 10, 2025
Merged

fix(generate): use topological sort for subtype map#4280
WillLillis merged 1 commit intotree-sitter:masterfrom
WillLillis:super_sort

Conversation

@WillLillis
Copy link
Member

@WillLillis WillLillis commented Mar 11, 2025

Detailed explanation of the problem in #4277. In short, we need to impose a total ordering when supplying a custom comparator to any of the sort functions in Rust's stdlib, or use a topological sort instead.

I checked our other usages of sort_by in the project and they appear to already impose a total ordering.

@WillLillis WillLillis marked this pull request as draft March 12, 2025 01:27
@WillLillis WillLillis marked this pull request as ready for review March 12, 2025 05:14
@WillLillis WillLillis changed the title fix(generate): impose total ordering on subtype map fix(generate): use topological sort for subtype map Mar 13, 2025
@clason
Copy link
Contributor

clason commented Jun 6, 2025

rebase so @amaanq can take a look?

@clason
Copy link
Contributor

clason commented Jun 27, 2025

@WillLillis Can you say something about the performance (esp. memory) impact?

Also, why isn't the parser generate test being run on this PR? Is this because the crates refactor moved the generate out from under the cli? In this case, the paths in the workflow need to be adjusted -- what would be the relevant crates now? Or should we just run them unconditionally now?

(Maybe we could run the build workflow generally -- it's relatively quick -- and just gate the generate workflow behind crates/generate changes? I'd also love to see the full suite being run on any release PR, as a final chance to catch obvious regressions before pushing out a release.)

@clason
Copy link
Contributor

clason commented Jun 27, 2025

#4545

@clason
Copy link
Contributor

clason commented Jul 10, 2025

Testing with some complex grammars (systemverilog, nim) shows no measurable impact on Linux and a ~1% increase on macOS, which is negligible. So this looks good to go.

@WillLillis WillLillis merged commit d2e06bf into tree-sitter:master Jul 10, 2025
18 checks passed
@tree-sitter-ci-bot
Copy link

Backport failed for release-0.25, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-0.25
git worktree add -d .worktree/backport-4280-to-release-0.25 origin/release-0.25
cd .worktree/backport-4280-to-release-0.25
git switch --create backport-4280-to-release-0.25
git cherry-pick -x d2e06bf130a479ba90f2657fd67f129e5a9ad69f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tree-sitter generate may cause a panic in standard library's sort function

5 participants