-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Assist to convert nested function to closure. #13467
Assist to convert nested function to closure. #13467
Conversation
f690e59
to
7cb1192
Compare
☔ The latest upstream changes (presumably #13516) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also check that the function has no generic parameters, as we can't transform that into a closure (+ a test that verifies this)
crates/ide-assists/src/handlers/convert_nested_function_to_closure.rs
Outdated
Show resolved
Hide resolved
function | ||
.syntax() | ||
.parent() | ||
.map(|p| p.ancestors().any(|a| a.kind() == SyntaxKind::FN)) | ||
.unwrap_or(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will trigger incorrectly if we have an associated function nested in a function (very rare that someone does this but it could happen), so it might be nicer to do the following
function.ancestors().skip(1).find_map(ast::Item::cast).unwrap_or(false, |it| matches!(it, ast::item::Fn(_))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what that snippet is supposed to do, namely because there are two arguments that are passed to unwrap_or
. Would it be easier to just check if the function is an associated function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh my bad, this was supposed to be a map_or_else
. But yes it would be easier to just check if it is an assocaited function actually, that is go from the ast::Fn
to the hir
version and check if that is an assoc item.
crates/ide-assists/src/handlers/convert_nested_function_to_closure.rs
Outdated
Show resolved
Hide resolved
crates/ide-assists/src/handlers/convert_nested_function_to_closure.rs
Outdated
Show resolved
Hide resolved
Sorry for the late review, was a bit busy. |
Ping @mdx97, are you going to come back to this? I can take over and rebase if not. |
@jplatte You can take this over if you'd like. I don't really have a timeline on when I could get back to this. |
Closing in favor of #14455 |
I'm a bit worried about the
has_semicolon
function. The syntax node returned forAssistsContext::find_node_at_offset
doesn't seem to capture the semicolon of the nested function if it has one. The current implementation was the only workaround that I could think of. If there is a better way let me know! Maybe we have to fix something upstream in Rowan?closes #13230