Add wrap/unwrap return type in Option #18294
Conversation
rustbot
added
the
S-waiting-on-review
Veykril
left a comment
Veykril
left a comment
There was a problem hiding this comment.
Hmm, there shouldn't be a need to fully duplicate the two modules. I'd expect us to be able to re-use a lot of code here and abstract it over Option/Result
|
That's a good point, I'll start working on that tonight. |
|
Let me know if there's anything else you want to see changed. I'm not really sure what the precedent is for when to group assists, so I decided to leave them separate, but I'm open to suggestions for that as well. |
Veykril
left a comment
Veykril
left a comment
There was a problem hiding this comment.
Generally good to me, just raising 2 perf things that would be good to fix
| } | ||
| } | ||
|
|
||
| fn wrap_return_type(acc: &mut Assists, ctx: &AssistContext<'_>, kind: WrapperKind) -> Option<()> { |
There was a problem hiding this comment.
The way this is setup we do this work twice now (always), might be better to only have one entry point, do the filtering logic once and then add both assists at once. We also probably want to group the two assist under one assist group (label being something "Wrap return type in ...").
| unwrap_return_type(acc, ctx, UnwrapperKind::Result) | ||
| } | ||
|
|
||
| fn unwrap_return_type( |
There was a problem hiding this comment.
In similar fashion we can combine the entry points here to one and then check for either types after resolve_type to figure out which one is applicable
|
Sounds good, I'll get that wrapped up today. |
|
|
||
| let kind = UnwrapperKind::ALL | ||
| .iter() | ||
| .find(|k| matches!(k.core_type(&famous_defs), Some(core_type) if ret_enum == core_type))?; |
There was a problem hiding this comment.
If this seems a bit too dense I could easily rewrite this as an if that checks against each UnwrapperKind individually, just let me know.
|
Thanks! |
|
☀️ Test successful - checks-actions |
4 participants
I pretty much just copied over the code and tests for wrapping/unwrapping return types in
Resultand then did a bunch of find and replace changes.I handled unwrapping statements returning
Noneby just replacingNonewith the unit type, but I'm open to suggestions for more intuitive behavior here.