Skip to content

[taplo-common] fix infinite recursion with composed allOfs#644

Closed
sunshowers wants to merge 1 commit intotamasfe:masterfrom
sunshowers:allof-composed
Closed

[taplo-common] fix infinite recursion with composed allOfs#644
sunshowers wants to merge 1 commit intotamasfe:masterfrom
sunshowers:allof-composed

Conversation

@sunshowers
Copy link
Copy Markdown

@sunshowers sunshowers commented Aug 11, 2024

Hi there!

I was trying to write a schema for nextest's configuration, and I ran into a call stack exhaustion issue. It turned out to be because an allOf wasn't getting removed even though there was an attempt to do so, leading to infinite recursion.

I couldn't find any tests for schemas so I added some. Please let me know if I can rearrange this to your liking somehow, thanks!

(And once this is landed a release would be lovely -- this is a hard blocker for nextest's schema at the moment.)

.remove("allOf");

let mut merged_all_of = Value::Object(serde_json::Map::default());

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed below in line 569, and above in line 512/518/525, that depth wasn't getting subtracted by 1. Is this expected? Naively I'd expect depth to be subtracted here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it looks like #353 is related.

@DavisVaughan
Copy link
Copy Markdown

DavisVaughan commented Feb 6, 2025

This one would be really nice to merge! Looks to me like it is just fixing a small bug and making the intended behavior actually happen. It did cause parsing of ruff's toml to break (astral-sh/ruff#15978) which they ended up working around but it would be nice to not have to.

I did confirm that this PR (rebased on master) does fix the ruff problem

@sunshowers
Copy link
Copy Markdown
Author

Ah looks like the project is active again -- apologies for pinging you directly @panekj but I'm wondering if this PR can be merged. The bug is pretty clear and it's a hard blocker for nextest's schema.

@sunshowers
Copy link
Copy Markdown
Author

Closing and re-doing this after rebasing.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants