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

Add Convert match to let-else assist #13516

Merged
merged 6 commits into from
Nov 1, 2022

Conversation

unexge
Copy link
Contributor

@unexge unexge commented Oct 29, 2022

Closes #13254

@@ -333,6 +333,8 @@ pub struct InferenceResult {
assoc_resolutions: FxHashMap<ExprOrPatId, AssocItemId>,
pub diagnostics: Vec<InferenceDiagnostic>,
pub type_of_expr: ArenaMap<ExprId, Ty>,
/// For each match expr, record diverging arm's expr.
pub diverging_arms: FxHashMap<ExprId, Vec<ExprId>>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to get diverging arms by looking types of arm expressions via hir::semantics::Semantics::type_of_expr but we coerce all match arm expressions and the whole match expression to the Never type when we encounter any diverging arm, so I couldn't find a way to distinguish diverging and non-diverging arms without recording this information.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it help if you look at the unadjusted type? In an expression like

    match Some(0) {
        Some(_) => {}
        None => return
    }

return has type ! but is adjusted to (), so the unadjusted type should provide the information you need

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I found the issue – your tests use Option but don't declare it. I'll push a fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohh 😅 great! Thanks for the fix.

@jonas-schievink
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 1, 2022

📌 Commit 72d5b45 has been approved by jonas-schievink

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 1, 2022

⌛ Testing commit 72d5b45 with merge d90cb1e...

@bors
Copy link
Contributor

bors commented Nov 1, 2022

☀️ Test successful - checks-actions
Approved by: jonas-schievink
Pushing d90cb1e to master...

@bors bors merged commit d90cb1e into rust-lang:master Nov 1, 2022
@unexge unexge deleted the add-convert-match-to-let-else-assist branch November 1, 2022 19:19
bors added a commit that referenced this pull request Nov 2, 2022
…-return-assist, r=jonas-schievink

Use let-else statements in `Convert to guarded return` assist

Follow up for #13516, addresses remaining part of #13254 (comment)
@fmease
Copy link
Member

fmease commented Nov 7, 2022

Does this also fix #11908?

@liamhession
Copy link

@unexge - I was hoping you could explain to me what you mean by the terms "diverging arm" and "extracting arm" in match statements? I'm not seeing that term used elsewhere so i'm not sure what exactly they're supposed to describe. The reason i'm interested is that i'm trying to expand the functionality of this convert-match-to-let-else assist that you created!

My current guess is that a diverging arm is one which simply returns? Extracting arm is everything else?

Would really appreciate any more context you may be able to give about how you approached creating this assist, thanks!

@Veykril
Copy link
Member

Veykril commented Nov 19, 2022

diverging arm = a match arm that has type ! (never type), so an arm that never yields a value (due to it returning for example).

extracting I imagine is the arm that "extracts" the value of interest from the value being match on

@unexge
Copy link
Contributor Author

unexge commented Nov 21, 2022

@liamhession I believe @Veykril answered your question but to give a bit more context:
There is a common theme that you are only interested some subpattern from a pattern, like you have Option<T> but you are only interested with Some(T) or you have ast::Expr but you are only interested with ast::AwaitExpr and the usual way to encode this pattern is you match on the given value (and now you can also use let-else statement :)) and you usually have 2 match arms:
one that diverges, meaning stops the execution of the function by return, continue, panic! or any other never ! type
one that extracts the value you are interested.

You can see that pattern a lot in Rust Analyzer especially in ide-assists for example:

// This only applies for tuple patterns, types, and initializers.
let tuple_pat = match pat { // <- extracts `ast::Pat::TuplePat` from `ast::Pat`
    ast::Pat::TuplePat(pat) => pat, // <- extracting arm
    _ => return None, // <- diverging arm
};
// ...
let tuple_init = match init { // <- extracts `ast::Expr::TupleExpr` from `ast::Expr`
    ast::Expr::TupleExpr(expr) => expr, // <- extracting arm
    _ => return None, // <- diverging arm
};

from

// This only applies for tuple patterns, types, and initializers.
let tuple_pat = match pat {
ast::Pat::TuplePat(pat) => pat,
_ => return None,
};
let tuple_ty = ty.and_then(|it| match it {
ast::Type::TupleType(ty) => Some(ty),
_ => None,
});
let tuple_init = match init {
ast::Expr::TupleExpr(expr) => expr,
_ => return None,
};

Happy to help if you have any questions and I can also try to give you some pointers if you have some specific improvements in your mind!

@liamhession
Copy link

Very nice context to have, thanks @unexge and @Veykril

My confusion came about in part because i couldn't find really any other use of those terms "extracting arm" and "diverging arm" through Googling. But their names make sense in the context you laid out.

The improvements i'm aiming for are to expand the variety of "scrutinee expressions" (just learned that vocab from https://doc.rust-lang.org/reference/expressions/match-expr.html) this assist can be used for. The relevant issue within rust-analyzer is #13540

I've first been working on tweaking it so that it will recognize a statement with tuple after the let keyword, as outlined in the issue. But i hope to leave it even more general-purpose than just something that additionally recognizes that case.

I think first i'll need to update the find_binding function to add arms to the match statement it uses, and then update rename_variable to be able to use alternate sub-types of ast::Pat. Do you think that sounds about right @unexge ?

@unexge
Copy link
Contributor Author

unexge commented Nov 22, 2022

@liamhession yes I believe, you need to update following functions to be more generic:
find_binding: in addition to ast::IdentPat it should also find ast::TuplePat and other patterns that makes sense
find_extracted_variable: should probably return Vec<ast::Name> for every binding? instead of a single name
rename_variable: should be updated according to return types of previous functions and should handle new cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Convert match to let-else" assist
6 participants