Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent infinite recurstion in indent #970

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

BlankParenthesis
Copy link

The error here is tricky to track down the root cause of (especially for someone new to the codebase) but the trigger for it as I understand is as follows:

  1. The indent rule begins to build offsets as a map of token beginnings to indent levels, each referencing its own parent. Among them are "root nodes" ­— entries in the map which have a type of 2 and no parent.
  2. A token will arrive with a starting index which overlaps a root node.
  3. The offset builder then inserts this new offset on top of the root node, providing it a new parent.
  4. This parent originally depended on that root node at some point in its ancestor chain, and thus a circular dependency is created.
  5. When later resolving this circular dependency, infinite recursion is occurs. This results in a stack limit error, crashing the program.

The ideal solution is probably to avoid inserting these tokens which overlap root nodes, or possibly disambiguate offsets by more than simply the start index. The fix here (simply ignoring attempts to overwrite root nodes) passes all the existing tests as far as I can see but I have not had time to extensively test it and simply discarding the information seems misguided — though I cannot currently imagine a scenario where the token which overlaps the root has a different indent level from the node that it overlaps.

The test here is far from ideal but serves to demonstrate a reproducible case of the bug. Having something to check for this sort of error is good, but someone with more knowledge of the root cause might be able to produce a better fix and related test. Of note, it seems to be the most minimal viable example I can find since the following changes prevented the error from occurring:

  • Removing the store unwrap token ($)
  • Removing the Enum
  • Making the Enum empty
  • Changing the script lang attribute to the empty string
  • Removing the script lang attribute
  • Adding <!-- prettier-ignore --> to the top of the file

The prettier ignore comment typically included at the top of these tests
prevent the bug.
Copy link

changeset-bot bot commented Dec 22, 2024

⚠️ No Changeset found

Latest commit: 5ef167b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@BlankParenthesis BlankParenthesis marked this pull request as draft December 22, 2024 04:36
@BlankParenthesis
Copy link
Author

Some further testing has revealed that this is indeed not an appropriate fix for the underlying cause: while this prevents crashing when encountering it, the reported required indent levels are often wrong for files containing both an enum and a store dereference ($). I've changed this to a draft for now since this feels like the sort of issue that should be addressed directly.

@AlbertMarashi
Copy link

AlbertMarashi commented Jan 7, 2025

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