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

internal: Migrate assists to the structured snippet API, part 3 #15260

Merged
merged 8 commits into from Jul 12, 2023

Conversation

DropDemBits
Copy link
Contributor

Continuing from #15231

Migrates the following assists:

  • add_missing_match_arms
  • fix_visibility
  • promote_local_to_const

The add_missing_match_arms changes are best reviewed commit-by-commit since they're relatively big changes compared to the rest of the commits.

`does_not_fill_wildcard_with_wildcard`
and `does_not_fill_wildcard_with_partial_wildcard_and_wildcard`
both made no modifications to the code,
which is a problem for mutable ast porting as it generates a best-effort
minimal set of text edits,
and assists require at least one text edit.
Requires a hack in order to work inside of macros
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2023
@DropDemBits
Copy link
Contributor Author

Also tried to migrate extract_variable this run, but it was too much for me at the time 😅

Copy link
Contributor

@lowr lowr left a comment

Choose a reason for hiding this comment

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

The migration itself looks great! Leaving a few small suggestions.

crates/ide-assists/src/handlers/fix_visibility.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/fix_visibility.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/fix_visibility.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/promote_local_to_const.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/promote_local_to_const.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/add_missing_match_arms.rs Outdated Show resolved Hide resolved
`clone_for_update` is relatively cheap in comparison, since making a
node require parsing an entire source text

Adds a test to make sure that it doesn't crash when multiple uses are
present.
@DropDemBits
Copy link
Contributor Author

Unfortunate thing to note is that currently rendering structured snippets into the real text form doesn't escape existing snippet bits so we're regressing to this behaviour of eating up backslashes 😔
#11897 (comment)

I was eventually planning to change up the rendering anyway, but probably a good idea to focus on it now so that assist migration doesn't end up further regressing behaviour.

@DropDemBits
Copy link
Contributor Author

After actually testing to see if the behaviour does regress for add_missing match_arms... it does not 🎉
This is still using the same structured snippet rendering process though, and the save comes from the tree diff only covering modified nodes (yay!).

I did still end up making a branch that did change the rendering process, so that will still be helpful when user-provided code ends up in the tree diff.

@lowr
Copy link
Contributor

lowr commented Jul 12, 2023

Thanks for the additional investigation (and the new PR ofc)! I believe this is good to go, but bors seems to be dormant now 🙃

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 12, 2023

📌 Commit a9889a0 has been approved by lowr

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jul 12, 2023

⌛ Testing commit a9889a0 with merge f45fdd9...

@bors
Copy link
Collaborator

bors commented Jul 12, 2023

☀️ Test successful - checks-actions
Approved by: lowr
Pushing f45fdd9 to master...

@bors bors merged commit f45fdd9 into rust-lang:master Jul 12, 2023
10 checks passed
@DropDemBits DropDemBits deleted the structured-snippet-migrate-3 branch November 12, 2023 01:58
bors added a commit that referenced this pull request Nov 15, 2023
…ykril

internal: Migrate assists to the structured snippet API, part 4

Continuing from #15260

Migrates the following assists:
- `add_turbo_fish`
- `add_type_ascription`
- `destructure_tuple_binding`
- `destructure_tuple_binding_in_subpattern`

I did this a while ago, but forgot to make a PR for the changes until now. 😅
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants