prettier icon indicating copy to clipboard operation
prettier copied to clipboard

Fix infinite recursion issue for new Array nodes

Open pieterv opened this issue 2 years ago • 3 comments
trafficstars

Description

In the js language printer, when an array is printed the printArrayElements util will take each its elements and call isLineAfterElementEmpty. This checks the elements locEnd position and uses a recursive function to walk through the file to find its related ",". For cases where the node is dynamically created the location will not match the source text (often range will just be set to [1, 1]). In some rare cases the file source text will not contain a "," at all which will cause the function to infinitely recurse through each char position until it its the maximum stack size. I understand the codemod/loc not matching source case is not expected to be supported by prettier but this case does tend to work well today and this specific fix does not add much complexity or overhead so would be nice to include it.

Specific changes:

  • Changed recursive skipToComma function to be iterative, since even in a normal case this could add a good amount of overhead as it walks through the text one char at a time.
  • Additionally i added a backstop to the iteration so it does not walk past the end of the file.
  • Inline skipComment logic, no need for the function overhead here.
  • To guard against the codemod use cases i abort if the node has a 0 size loc, this should not be possible outside this use case.

Checklist

  • [ ] I’ve added tests to confirm my change works.
  • [ ] (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • [ ] (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • [x] I’ve read the contributing guidelines.

Try the playground for this PR

pieterv avatar Jun 21 '23 18:06 pieterv

Can you add a test?

fisker avatar Jun 25 '23 07:06 fisker

@CodiumAI-Agent please review

coditamar avatar Jul 07 '23 06:07 coditamar

PR Analysis

  • 🎯 Main theme: Fixing an infinite recursion issue in the js language printer when printing arrays
  • 🔍 Description and title: Yes
  • 📌 Type of PR: Bug fix
  • 🧪 Relevant tests added: No
  • Minimal and focused: Yes, the PR is focused on fixing a specific issue in the js language printer and does not include unrelated changes.
  • 🔒 Security concerns: No, the changes made in this PR do not introduce any obvious security concerns.

PR Feedback

  • 💡 General PR suggestions: The PR fixes an important issue and the changes seem to be well thought out. However, it would be beneficial to add tests that confirm the fix works as expected. This would help prevent regression in the future.

  • 🤖 Code suggestions:

    • relevant file: src/language-js/print/array.js suggestion content: Consider adding a comment explaining the reason for the check if (currentIdx === locStart(node)). This would help other developers understand the need for this check. [medium]

    • relevant file: src/language-js/print/array.js suggestion content: Consider adding a comment explaining the while loop logic, specifically why we are skipping inline and trailing comments. This would help other developers understand the logic and its purpose. [medium]

How to use

Tag me in a comment '@CodiumAI-Agent' to ask for a new review after you update the PR. You can also tag me and ask any question, for example '@CodiumAI-Agent is the PR ready for merge?'

CodiumAI-Agent avatar Jul 07 '23 06:07 CodiumAI-Agent