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(analyze & lsp): code action and text mutation #2237

Merged
merged 9 commits into from
Apr 2, 2024

Conversation

Sec-ant
Copy link
Contributor

@Sec-ant Sec-ant commented Mar 29, 2024

Summary

This PR fixes the unexpected code deletion and repetition when quickfix.biome is enabled and some import-related rules are applied.

This PR includes 3 fixes:

  1. Reverse the order of code actions, so the previous code action won't affect its following action.
  2. In code actions of rules noUnusedImports and noRedundantUseStrict, remove the trivia of the import statements or "use strict" directives instead of transferring them to their siblings.
  3. Calculate TextEdits from CommitChanges directly, instead of .commit().to_string() and diffing the entire code string.

To make this PR code review experience friendly, I grouped the changes into several commits:

  1. fix(code_actions): reverse order: 0bf3f09
    Reverse the order of code actions, so actions will be applied from back to front.
    This will make sure following code actions won't be invalidated by their previous code actions.

  2. fix(rules): remove leading trivia instead of transferring them: 4f1e9cd
    This helps to get the correct result when applying code actions one by one.
    This can also avoid potential problems where comment directives like @ts-expect-error being transferred to an unexpected place.

  3. fix(text_range_and_text_edit): refactor and reimplementation: 22780e8
    The core logic is in file crates/biome_rowan/src/ast/batch.rs. This commit reimplements how we calculate the text range and text edit from batch mutations:

    • SyntaxSlot::Empty now contains an index position of the empty slot. This is to aid the calculation of text edit from commit changes.
    • TextEdit has a new public method with_unicode_words_diff. This is to aid the calculation of text edit from commit changes.
    • The key of CommitChange that will affect its order in a sorted list is changed. This is to ensure changes will be addressed from back to front.
    • Add an is_from_action field to CommitChange. This is to distinguish whether a commit change is from the action or is propagated from the children. This is to aid the calculation of text edit from commit changes.
    • Rename as_text_edits to as_text_range_and_edit for clarification.
    • The core logic of commit is moved into commit_with_text_range_and_edit. That's because when calculating the text range and text edit, we need to commit the changes. So they should be addressed together.
    • When the new tree, text range and text edit are all required, we should use commit_with_text_range_and_edit instead of commit and as_text_range_and_edit separately to avoid unncessary clone. That's because when calculating the text range and text edit, we also need to commit the changes. This should be done in one pass.
  4. chore: update test snapshots: 41bc14c
    Most of the updates are the result of the reimplementation of how we calculate the text edits.
    Previously, the text edit is acquired from a naive text diff of the code before and after commit.
    We now populate the text edit struct manually when applying changes.

    Some of the updates are the reuslt of discarding leading trivia when removing a node.

  5. chore: regenerate web docs40cde73
    Regenerate the web docs to reflect the changes.

  6. chore: update changelog: 434627f
    Update the changelog to reflect the fix.

  7. chore: update changelog: db8b4cf
    Unlink some issues in the changelog.

Test Plan

CI should pass. There'll be many snapshot updates (see above for the reasons). But the final result should be correct.

Code actions are tested locally with VSCode:

  1. noUnusedImports

    Video Demos
    before after
    no-unused-imports-before.mp4
    no-unused-imports-after.mp4
    no-unused-imports-before.mp4
    no-unused-imports-after.mp4
  2. useImportType

    Video Demos
    before after
    use-import-type-before.mp4
    use-import-type-after.mp4

I cannot reproduce others from the linked issues. Please provide the code and settings (include biome and vscode) if someone has a stable minimal reproduction.

@Sec-ant Sec-ant marked this pull request as draft March 29, 2024 12:22
@github-actions github-actions bot added A-Core Area: core A-Linter Area: linter A-Parser Area: parser A-LSP Area: language server protocol L-JavaScript Language: JavaScript and super languages labels Mar 29, 2024
Copy link

netlify bot commented Mar 29, 2024

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit b4679d3
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/660bc3ffe30a8800081ef175
😎 Deploy Preview https://deploy-preview-2237--biomejs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 100 (no change from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 93 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codspeed-hq bot commented Mar 29, 2024

CodSpeed Performance Report

Merging #2237 will improve performances by 7.51%

⚠️ No base runs were found

Falling back to comparing Sec-ant:fix/code-action-mutation (b4679d3) with main (2950aeb)

Summary

⚡ 1 improvements
✅ 92 untouched benchmarks

Benchmarks breakdown

Benchmark main Sec-ant:fix/code-action-mutation Change
big5-added.json[cached] 2.4 ms 2.2 ms +7.51%

@github-actions github-actions bot added the A-Website Area: website label Mar 29, 2024
@Sec-ant Sec-ant changed the title fix(analyze): code action mutation fix(analyze & lsp): code action mutation Mar 29, 2024
@github-actions github-actions bot added A-Project Area: project L-CSS Language: CSS L-JSON Language: JSON and super languages labels Mar 30, 2024
@Sec-ant Sec-ant changed the title fix(analyze & lsp): code action mutation fix(analyze & lsp): code action and text mutation Mar 31, 2024
@github-actions github-actions bot added the A-Changelog Area: changelog label Mar 31, 2024
@Sec-ant
Copy link
Contributor Author

Sec-ant commented Mar 31, 2024

!bench_cli

@Sec-ant
Copy link
Contributor Author

Sec-ant commented Mar 31, 2024

I unlinked #1550 and #1570 because I'm not sure if this PR will fix them. I cannot reproduce them either.

@Sec-ant Sec-ant marked this pull request as ready for review March 31, 2024 07:07
@Sec-ant Sec-ant requested a review from ematipico March 31, 2024 07:08
@Sec-ant
Copy link
Contributor Author

Sec-ant commented Mar 31, 2024

The only concern I have is the regression in readability of the diffs. I would like to hear if you have any ideas on how to address this in the future.

I wouldn't say it is a regression because these kind of diffs already exist in our existing code base. This PR merely changes some of them into another form. I also don't think the solution to address them has anything to do with this PR, but I can take a look into the problem in the future 😉.

Also, have you tested your change on a large repository to see if there is a perf regression or, conversely, a perf gain?

I did attempt to use the command in #2237 (comment) to get a benchmark. I was expecting there would be a report commented by a robot, but I got nothing 😆. What's the correct way to benchmark things?

@Conaclos
Copy link
Member

I did attempt to use the command in #2237 (comment) to get a benchmark. I was expecting there would be a report commented by a robot, but I got nothing 😆. What's the correct way to benchmark things?

If I remember correctly, these magic coments were removed when we added automated benchmark using Codspeed.

@Sec-ant
Copy link
Contributor Author

Sec-ant commented Apr 1, 2024

The release build of this PR has a small perf regression (~%4) when running biome lint compared to the release build on main. I tested the CLI in the TypeScript repo (tests files ignored):

biome-lint-bench

But no regression in biome ci:

biome-ci-bench

@github-actions github-actions bot added L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS L-JSON Language: JSON and super languages labels Apr 1, 2024
@ematipico
Copy link
Member

Out of curiosity, how did you compile the binary? We have some settings that we push during the CI

@Sec-ant
Copy link
Contributor Author

Sec-ant commented Apr 1, 2024

Out of curiosity, how did you compile the binary? We have some settings that we push during the CI

Isn't it cargo build --release 🤯?

@ematipico
Copy link
Member

If both benchmarks run on the same built artifact, then it should be fine.

During the CI we pass some environment variables: https://github.com/biomejs/biome/blob/main/.github%2Fworkflows%2Frelease_cli.yml#L128-L132

@Sec-ant
Copy link
Contributor Author

Sec-ant commented Apr 1, 2024

Oh yes I didn't set the RUSTFLAGS but I used the same command to build these two binaries, one from the main branch, the other from this PR branch (rebased on main). So they may not reflect the real performance of a published version.

Reverse the order of code actions, so actions will be applied from back to front.
This will make sure following code actions won't be invalidated by their previous code actions.
Transferring the leading trivia in code actions of some rules can invalidate comment directives.
This also helps to get the correct result when applying code actions one by one.
The core logic is in file `crates/biome_rowan/src/ast/batch.rs`. This commit reimplements how we calculate the text range and text edit from batch mutations:
- `SyntaxSlot::Empty` now contains an index position of the empty slot. This is to aid the calculation of text edit from commit changes.
- `TextEdit` has a new public method `with_unicode_words_diff`. This is to aid the calculation of text edit from commit changes.
- The `key` of `CommitChange` that will affect its order in a sorted list is changed. This is to ensure changes will be addressed from back to front.
- Add an `is_from_action` field to `CommitChange`. This is to distinguish whether a commit change is from the action or is propagated from the children. This is to aid the calculation of text edit from commit changes.
- Rename `as_text_edits` to `as_text_range_and_edit` for clarification.
- The core logic of `commit` is moved into `commit_with_text_range_and_edit`. That's because when calculating the text range and text edit, we need to commit the changes. So they should be addressed together.
- When the new tree, text range and text edit are all required, we should use `commit_with_text_range_and_edit` instead of `commit` and `as_text_range_and_edit` separately to avoid unncessary clone. That's because when calculating the text range and text edit, we also need to commit the changes. This should be done in one pass.
Most of the updates are the result of the reimplementation of how we calculate the text edits.
Previously, the text edit is acquired from a naive text diff of the code before and after commit.
We now populate the text edit struct manually when applying changes.

Some of the updates are the reuslt of discarding leading trivia when removing a node.
This is to keep them consistent with the existing code.
@Sec-ant
Copy link
Contributor Author

Sec-ant commented Apr 2, 2024

When debug symbols are stripped, the performance regression is reduced to less than %1.

image

@Sec-ant
Copy link
Contributor Author

Sec-ant commented Apr 2, 2024

@ematipico Do you have other suggestions or can I merge this?

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

The changes look good to me; I left only a few nits. That was some hard work you poured into this PR @Sec-ant, fantastic! 💪

crates/biome_rowan/src/ast/batch.rs Outdated Show resolved Hide resolved
crates/biome_rowan/src/ast/batch.rs Outdated Show resolved Hide resolved
crates/biome_rowan/src/ast/batch.rs Outdated Show resolved Hide resolved
crates/biome_rowan/src/ast/batch.rs Outdated Show resolved Hide resolved
crates/biome_rowan/src/ast/batch.rs Outdated Show resolved Hide resolved
crates/biome_text_edit/src/lib.rs Show resolved Hide resolved
@Sec-ant
Copy link
Contributor Author

Sec-ant commented Apr 2, 2024

Should be good to go. I'm gonna merge this now!

@Sec-ant Sec-ant merged commit 4719012 into biomejs:main Apr 2, 2024
18 checks passed
@Sec-ant Sec-ant deleted the fix/code-action-mutation branch April 2, 2024 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-CLI Area: CLI A-Core Area: core A-Linter Area: linter A-LSP Area: language server protocol A-Parser Area: parser A-Project Area: project A-Website Area: website L-CSS Language: CSS L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages
Projects
None yet
3 participants