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.

Sign up for GitHub

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

Jump to bottom

internal: Migrate convert_bool_then to SyntaxEditor #19253

Merged
merged 1 commit into from
Mar 2, 2025
Merged

internal: Migrate convert_bool_then to SyntaxEditor #19253

merged 1 commit into from
Mar 2, 2025

Conversation

ShoyuVanilla
Copy link
Member

Part of #18285

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 28, 2025
@ShoyuVanilla ShoyuVanilla force-pushed the migrate-convert-bool-then branch from 30c23d7 to f6ea1c4 Compare February 28, 2025 20:17
@ShoyuVanilla
Copy link
Member Author

Oh, I accidentally added part of another migration - convert_closure_to_fn - I was working on 😅
I'll continue working on it in a follow-up PR or this PR, if this isn't merged by then

DropDemBits
DropDemBits approved these changes Mar 1, 2025
View reviewed changes
Copy link
Contributor

@DropDemBits DropDemBits left a comment

Choose a reason for hiding this comment

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

I've only looked at the convert_bool_then-related changes, but looks good to me & approving those changes.

Waiting on convert_closure_to_fn changes to be split out. Surprised that it's using splice_children in the first place, but shouldn't be too hard to replace it with the SyntaxEditor equivalent.

@DropDemBits DropDemBits added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2025
@ShoyuVanilla ShoyuVanilla force-pushed the migrate-convert-bool-then branch from f6ea1c4 to 37822d5 Compare March 1, 2025 04:14
@ShoyuVanilla
Copy link
Member Author

Dropped unfinished convert_closure_to_fn and will address it in a new PR

@Veykril Veykril added this pull request to the merge queue Mar 2, 2025
Veykril
Veykril reviewed Mar 2, 2025
View reviewed changes
crates/ide-assists/src/handlers/convert_bool_then.rs
@@ -8,12 +8,13 @@ use ide_db::{
};
use itertools::Itertools;
use syntax::{
ast::{self, edit::AstNodeEdit, make, HasArgList},
ted, AstNode, SyntaxNode,
ast::{self, edit::AstNodeEdit, syntax_factory::SyntaxFactory, HasArgList},
Copy link
Member

Choose a reason for hiding this comment

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

This does still use AstNodeEdit for indentation, I assume that's mainly to not regress indentation for now? (merging either way)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, currently we have two ways to indent things (except manually indenting all lines in a syntax node):

rust-analyzer/crates/syntax/src/ast/edit.rs

Lines 119 to 133 in 81ff38f

/// Soft-deprecated in favor of mutable tree editing API `edit_in_place::Ident`.
pub trait AstNodeEdit: AstNode + Clone + Sized {
fn indent_level(&self) -> IndentLevel {
IndentLevel::from_node(self.syntax())
}
#[must_use]
fn indent(&self, level: IndentLevel) -> Self {
fn indent_inner(node: &SyntaxNode, level: IndentLevel) -> SyntaxNode {
let res = node.clone_subtree().clone_for_update();
level.increase_indent(&res);
res.clone_subtree()
}
Self::cast(indent_inner(self.syntax(), level)).unwrap()
}

rust-analyzer/crates/syntax/src/ast/edit_in_place.rs

Lines 964 to 970 in 81ff38f

pub trait Indent: AstNode + Clone + Sized {
fn indent_level(&self) -> IndentLevel {
IndentLevel::from_node(self.syntax())
}
fn indent(&self, by: IndentLevel) {
by.increase_indent(self.syntax());
}

and both of them utilizes mutable syntax trees.l

I'll add a non-mutable syntax tree alternative and replace previous usages with it in next PR 😄

Merged via the queue into rust-lang:master with commit 1ce1f08 Mar 2, 2025
9 checks passed
@ShoyuVanilla ShoyuVanilla deleted the migrate-convert-bool-then branch March 2, 2025 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Veykril Veykril Veykril approved these changes

@DropDemBits DropDemBits DropDemBits approved these changes

Assignees
No one assigned
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ShoyuVanilla @Veykril @DropDemBits @rustbot