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

fix: use Result type aliases in "Wrap return type in Result" assist #18016

Merged

Conversation

IvarWithoutBones
Copy link
Contributor

@IvarWithoutBones IvarWithoutBones commented Aug 31, 2024

This commit makes the "Wrap return type in Result" assist prefer type aliases of standard library type when one is in scope, it uses at least one generic parameter, and it has the name Result.

The last restriction was made in an attempt to avoid false assumptions about which type the user is referring to, but that might be overly strict. We could also do something like this, in order of priority:

  • Use the alias named "Result".
  • Use any alias if only a single one is in scope, otherwise:
  • Use the standard library type.

This is easy to add if others feel differently that is appropriate, just let me know.

Fixes #17796

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 31, 2024

let ret_is_alias = matches!(ty, Some(hir::Adt::Enum(ret_type)) if ret_type == enum_ty);
if enum_ty != core_result || ret_is_alias {
// The current name is not a Result alias, or the return type is already this Result alias.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot stop looping in the second case since SemanticScope::process_all_names doesn't accept ControlFlow or return an iterator. This is a bit wasteful but I'm sure if its worth modifying the API, thoughts?

There's a good chance this is already possible with different API I missed, but I figured I might as well ask 😅

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 it'd be nicer for us to expose a resolve_mod_path function on Semantics that takes a ModPath to resolve, then we can just call that with a ModPath::from_segmenst(Plain, iter;:once(sym::Result))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I've pushed a fix to do so. I'm not entirely sure if the signature is sensible as of now though.


let ret_is_alias = matches!(ty, Some(hir::Adt::Enum(ret_type)) if ret_type == enum_ty);
if enum_ty != core_result || ret_is_alias {
// The current name is not a Result alias, or the return type is already this Result alias.
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 it'd be nicer for us to expose a resolve_mod_path function on Semantics that takes a ModPath to resolve, then we can just call that with a ModPath::from_segmenst(Plain, iter;:once(sym::Result))

This commit makes the "Wrap return type in Result" assist prefer type aliases of standard library
type when the are in scope, use at least one generic parameter, and have the name "Result".

The last restriction was made in an attempt to avoid false assumptions about which type the
user is referring to, but that might be overly strict. We could also do something like this, in
order of priority:

* Use the alias named "Result".
* Use any alias if only a single one is in scope, otherwise:
* Use the standard library type.

This is easy to add if others feel differently that is appropriate, just let me know.
@Veykril
Copy link
Member

Veykril commented Sep 2, 2024

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Sep 2, 2024

📌 Commit 4e9d17b has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Sep 2, 2024

⌛ Testing commit 4e9d17b with merge d534cc6...

@bors
Copy link
Collaborator

bors commented Sep 2, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing d534cc6 to master...

@bors bors merged commit d534cc6 into rust-lang:master Sep 2, 2024
11 checks passed
@IvarWithoutBones IvarWithoutBones deleted the wrap-return-ty-local-result branch September 2, 2024 13:04
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.

wrap_return_type_in_result should recognize if a Result alias is in scope
4 participants