Skip to content

Fix: Handle Self replacement contextually in inline assists #20049

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

phyBrackets
Copy link

@phyBrackets phyBrackets commented Jun 20, 2025

Only replace Self when inlining across different impl contexts, and use
bare type names for expression contexts to avoid turbofish issues.

Fixes #19827

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 20, 2025
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.

So, we should make PathTransform do this transformation (it might already have parts of it implemented), and then use that in the other places in this assist (so far its only used for generic argument rewriting). That way we don't special case things to this assist when its actually a more general applicable transformation

@phyBrackets
Copy link
Author

Thanks @Veykril for the feedback, I think it does make sense that we should leverage and extend the existing PathTransform mechanism that's already used for generic argument rewriting. I have updated the patch based on your review.

@phyBrackets phyBrackets requested a review from Veykril June 20, 2025 10:14
@phyBrackets phyBrackets force-pushed the fix/inline-self-replacement-context-aware branch from 0634b41 to 781ee7a Compare June 20, 2025 10:24
Comment on lines +251 to +282
fn is_path_in_expression_context(path: &ast::Path) -> bool {
let mut current = path.syntax().parent();
while let Some(node) = current {
// Expression contexts where we need bare type names
if let Some(call_expr) = ast::CallExpr::cast(node.clone()) {
if let Some(expr) = call_expr.expr() {
if expr.syntax().text_range().contains_range(path.syntax().text_range()) {
return true;
}
}
}
if ast::PathExpr::cast(node.clone()).is_some() {
return true;
}
if let Some(record_expr) = ast::RecordExpr::cast(node.clone()) {
if let Some(record_path) = record_expr.path() {
if record_path.syntax().text_range().contains_range(path.syntax().text_range()) {
return true;
}
}
}
// Stop at type/pattern boundaries
if ast::Type::cast(node.clone()).is_some()
|| ast::Pat::cast(node.clone()).is_some()
|| ast::RetType::cast(node.clone()).is_some()
{
return false;
}
current = node.parent();
}
false
}
Copy link
Member

Choose a reason for hiding this comment

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

This can be simplified (and also made more robust by that)

Suggested change
fn is_path_in_expression_context(path: &ast::Path) -> bool {
let mut current = path.syntax().parent();
while let Some(node) = current {
// Expression contexts where we need bare type names
if let Some(call_expr) = ast::CallExpr::cast(node.clone()) {
if let Some(expr) = call_expr.expr() {
if expr.syntax().text_range().contains_range(path.syntax().text_range()) {
return true;
}
}
}
if ast::PathExpr::cast(node.clone()).is_some() {
return true;
}
if let Some(record_expr) = ast::RecordExpr::cast(node.clone()) {
if let Some(record_path) = record_expr.path() {
if record_path.syntax().text_range().contains_range(path.syntax().text_range()) {
return true;
}
}
}
// Stop at type/pattern boundaries
if ast::Type::cast(node.clone()).is_some()
|| ast::Pat::cast(node.clone()).is_some()
|| ast::RetType::cast(node.clone()).is_some()
{
return false;
}
current = node.parent();
}
false
}
fn is_path_in_expression_context(path: &ast::Path) -> bool {
let path = path.top_path();
let Some(parent) = path.syntax().parent() else { return false; };
parent.kind() == SyntaxKind::PATH_EXPR
}

I don't think we need to consider record expression constructors, as their path is actually in type namespace if I am not mistaken.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, my original concern was about struct constructor calls like Foo { field: value } where we might want Foo instead of Self::Foo, but thinking about it more, record expressions are indeed type namespace references and even if we wanted bare names there, PathTransform should handle the namespace correctly, so this complexity isn't justified indeed, thanks.


// Context-aware replacement
let replacement = if is_path_in_expression_context(&path) {
let bare_name = get_bare_type_name(ty_str);
Copy link
Member

Choose a reason for hiding this comment

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

I think the more correct approach would be to replace the Self with the fully qualified path in turbofished form instead of dropping the generics entirely.

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.

Inline function assist produces invalid code when inlining across generic impl blocks
3 participants