Skip to content

Conversation

@boattime
Copy link
Contributor

This PR adds dereferencing suggestions for Copy types. When a function expects a non-reference type T and we have a &T where T: Copy, we now suggest the dereferenced value in completions.

#18873

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. has-merge-commits S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 11, 2025
@rustbot

This comment has been minimized.

@rustbot rustbot removed has-merge-commits S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 11, 2025
Comment on lines 458 to 468
let (label, edit_text) = match mode {
CompletionItemRefMode::Reference(Mutability::Shared) => {
(format!("&{}", self.label.primary), "&")
}
CompletionItemRefMode::Reference(Mutability::Mut) => {
(format!("&mut {}", self.label.primary), "&mut ")
}
CompletionItemRefMode::Dereference => (format!("*{}", self.label.primary), "*"),
};

(label, ide_db::text_edit::Indel::insert(offset, String::from(edit_text)), relevance)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let (label, edit_text) = match mode {
CompletionItemRefMode::Reference(Mutability::Shared) => {
(format!("&{}", self.label.primary), "&")
}
CompletionItemRefMode::Reference(Mutability::Mut) => {
(format!("&mut {}", self.label.primary), "&mut ")
}
CompletionItemRefMode::Dereference => (format!("*{}", self.label.primary), "*"),
};
(label, ide_db::text_edit::Indel::insert(offset, String::from(edit_text)), relevance)
let prefix= match mode {
CompletionItemRefMode::Reference(Mutability::Shared) => "&",
CompletionItemRefMode::Reference(Mutability::Mut) => "&mut ",
CompletionItemRefMode::Dereference =>"*",
};
let label = format!("{prefix}{}", self.label.primary),
(label, ide_db::text_edit::Indel::insert(offset, String::from(prefix)), relevance)

}
}

if let Some(completion_ty_without_ref) = completion_ty.remove_ref() {
Copy link
Member

Choose a reason for hiding this comment

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

We can move this to the start of the function as let expected_type_without_ref = expected_type.remove_ref()?; since the branch above makes use of it as well either way to deduplicate a bit

Copy link
Contributor Author

@boattime boattime Jan 12, 2025

Choose a reason for hiding this comment

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

Just to clarify - are you suggesting we move both remove_ref() calls to the start of the function, or just the expected_type.remove_ref() call? Its also possible that the expected_type.remove_ref() returns None and would early return so there is still a need for the following code in the first branch.

if let Some(expected_without_ref) = &expected_type_without_ref 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Veykril I pushed the update to move both of these methods to the top of the function. Let me know if this is not what you were looking for.

Comment on lines 137 to 145
match ref_mode {
CompletionItemRefMode::Reference(mutability) => match mutability {
Mutability::Shared => hasher.update("&"),
Mutability::Mut => hasher.update("&mut"),
},
CompletionItemRefMode::Dereference => {
hasher.update("*");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Similar to above, we can move just return the strings in the match, then call hasher.update on that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

Thanks!

@Veykril Veykril added this pull request to the merge queue Jan 15, 2025
Merged via the queue into rust-lang:master with commit d82e1a2 Jan 15, 2025
9 checks passed
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.

3 participants