More enum matching #7677

jDomantas · 2021-02-14T17:48:04Z

Renamed existing generate_enum_match_method to generate_enum_is_variant
Added two similar assists to generate into_ and as_ methods.
Made all of them general enough to work on record and tuple variants too.

For as_ method generation there's room to improve:

Right now it always returns Option<&Field>, even though Option<Field> would be nicer when Field: Copy. I don't know how to check if the field type implements Copy. If given suggestions I could try to fix this in a follow-up pr.
&String could be replaced with &str, &Box<_> with &_, and probably some more. I don't know what would be a good way to do that.

matklad · 2021-02-14T19:36:45Z

bors · 2021-02-14T19:36:45Z

✌️ yoshuawuyts can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

yoshuawuyts

Thanks for this PR @jDomantas! -- I think this is heading in a really good direction. To summarize some of the feedback from the PR:

The generated methods should be parameterized by the output type, rather than the variant name. So as_str rather than as_text.
The code generated by the into_ assist should pass clippy and adhere to the Rust naming conventions.
The shared code would likely go well into assists/utils.rs
The newly added lints should go into their own files. Especially if the first two points are addressed they'll be quite a bit different from the generate_enum_is_method assist.

I hope this feedback's helpful; thanks so much for this PR, and feel free to ping once you're ready for another review.

yoshuawuyts · 2021-02-14T20:03:41Z

crates/assists/src/handlers/generate_enum_match_method.rs

+enum VariantKind {
+    Unit,
+    /// Tuple with a single field
+    NewtypeTuple {
+        ty: Option<ast::Type>,
+    },
+    /// Tuple with 0 or more than 2 fields
+    Tuple,
+    /// Record with a single field
+    NewtypeRecord {
+        field_name: Option<ast::Name>,
+        field_type: Option<ast::Type>,
+    },
+    /// Record with 0 or more than 2 fields
+    Record,
+}


This seems like the kind of functionality for which I'd expect an abstraction to already exist within Rust-Analyzer. cc/ @Veykril

If it turns out this is something new, at least for now it'd still be good to have this live under assists/utils.rs so other assists can also make use of this.

There is no abstraction like this yet to my knowledge, most assists only required ast::StructKind which doesn't discern between single field record/tuples.

Also from the looks of it, it doesn't really make sense to keep the Option in the variants here. The binding_pattern and single_field_type functions just propogate the None values which are then propogated at every call site as well from what I can tell. I think making the variant_kind constructor return an Option<VariantKind> instead when any of those are None makes more sense, with that the inherent functions on VariantKind then can just return things directly.

yoshuawuyts · 2021-02-14T20:07:17Z

crates/assists/src/handlers/generate_enum_match_method.rs

+fn add_method_to_adt(
+    builder: &mut AssistBuilder,
+    adt: &ast::Adt,
+    impl_def: Option<ast::Impl>,
+    method: &str,
+) {
+    let mut buf = String::with_capacity(method.len() + 2);
+    if impl_def.is_some() {
+        buf.push('\n');
+    }
+    buf.push_str(method);
+
+    let start_offset = impl_def
+        .and_then(|impl_def| find_impl_block_end(impl_def, &mut buf))
+        .unwrap_or_else(|| {
+            buf = generate_impl_text(&adt, &buf);
+            adt.syntax().text_range().end()
+        });
+
+    builder.insert(start_offset, buf);
+}
+
+enum VariantKind {
+    Unit,
+    /// Tuple with a single field
+    NewtypeTuple {
+        ty: Option<ast::Type>,
+    },
+    /// Tuple with 0 or more than 2 fields
+    Tuple,
+    /// Record with a single field
+    NewtypeRecord {
+        field_name: Option<ast::Name>,
+        field_type: Option<ast::Type>,
+    },
+    /// Record with 0 or more than 2 fields
+    Record,
+}
+
+impl VariantKind {
+    fn pattern_suffix(&self) -> &'static str {
+        match self {
+            VariantKind::Unit => "",
+            VariantKind::NewtypeTuple { .. } | VariantKind::Tuple => "(..)",
+            VariantKind::NewtypeRecord { .. } | VariantKind::Record => " { .. }",
+        }
+    }
+
+    fn binding_pattern(&self) -> Option<(String, String)> {
+        match self {
+            VariantKind::Unit
+            | VariantKind::Tuple
+            | VariantKind::Record
+            | VariantKind::NewtypeRecord { field_name: None, .. } => None,
+            VariantKind::NewtypeTuple { .. } => Some(("(v)".to_owned(), "v".to_owned())),
+            VariantKind::NewtypeRecord { field_name: Some(name), .. } => {
+                Some((format!(" {{ {} }}", name.syntax()), name.syntax().to_string()))
+            }
+        }
+    }
+
+    fn single_field_type(&self) -> Option<&ast::Type> {
+        match self {
+            VariantKind::Unit | VariantKind::Tuple | VariantKind::Record => None,
+            VariantKind::NewtypeTuple { ty } => ty.as_ref(),
+            VariantKind::NewtypeRecord { field_type, .. } => field_type.as_ref(),
+        }
+    }
+}
+
+fn variant_kind(variant: &ast::Variant) -> VariantKind {
+    match variant.kind() {
+        ast::StructKind::Record(record) => {
+            if let Some((single_field,)) = record.fields().collect_tuple() {
+                let field_name = single_field.name();
+                let field_type = single_field.ty();
+                VariantKind::NewtypeRecord { field_name, field_type }
+            } else {
+                VariantKind::Record
+            }
+        }
+        ast::StructKind::Tuple(tuple) => {
+            if let Some((single_field,)) = tuple.fields().collect_tuple() {
+                let ty = single_field.ty();
+                VariantKind::NewtypeTuple { ty }
+            } else {
+                VariantKind::Tuple
+            }
+        }
+        ast::StructKind::Unit => VariantKind::Unit,
+    }
+}


Can we move these to assists/utils.rs instead?

yoshuawuyts · 2021-02-14T20:30:42Z

crates/assists/src/handlers/generate_enum_match_method.rs

+// enum Value {
+//  Number(i32),
+//  Text(String)$0,
+// }
+// ```
+// ->
+// ```
+// enum Value {
+//  Number(i32),
+//  Text(String),
+// }
+//
+// impl Value {
+//     fn into_text(self) -> Option<String> {
+//         if let Self::Text(v) = self {
+//             Some(v)
+//         } else {
+//             None
+//         }
+//     }
+// }


Function name

I would've expected the generated method to be parameterized by the name of the output type, rather than the name of the enum variant. as is always followed by the type we're converting into (e.g. slice::as_ptr or ThreadId::as_u64).

Suggested change

// enum Value {

// Number(i32),

// Text(String)$0,

// }

// ```

// ->

// ```

// enum Value {

// Number(i32),

// Text(String),

// }

//

// impl Value {

// fn into_text(self) -> Option<String> {

// if let Self::Text(v) = self {

// Some(v)

// } else {

// None

// }

// }

// }

// enum Value {

// Number(i32),

// Text(String)$0,

// }

// ```

// ->

// ```

// enum Value {

// Number(i32),

// Text(String),

// }

//

// impl Value {

// fn into_string(self) -> Option<String> {

// if let Self::Text(v) = self {

// Some(v)

// } else {

// None

// }

// }

// }

Output type

Running clippy on the generated function produces the following warning:

warning: methods called `into_*` usually take self by value; consider choosing a less ambiguous name --> src/lib.rs:15:20 | 15 | fn into_string(&self) -> Option<&String> { | ^^^^^ | = note: `#[warn(clippy::wrong_self_convention)]` on by default = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention

This matches the Rust naming guidelines and means we should probably only ever generate this function if we know we will never fail the conversion.

How did you get that? It does generate into_* methods with self as the parameter, and as_* methods with &self.

I disagree with your point about the function names. Naming the function by the type is good as long as the function is about the type conversion. A key difference is that these functions are intended to extract the payload from a specific enum variant, so they are precisely about the variant instead of the type. And when the function is generated for a specific variant it is weird to name them by type when some other variants have the same payload type, or when the payload is a generic type parameter. Also I think that naming them by the type would also be inconsistent with is_ functions which are very similar in purpose and are precise about variant.

The concrete example I had in mind when I saw the request for these was rowan's NodeOrToken methods. Another example is Result::ok and Result::err - they also use the name to indicate which variant is extracted, but don't use the as_ and into_ naming convention. (although I guess that the fact that both of these examples are generic enums does not really help my case as referring by type would be super weird)

So maybe the naming convention for these could be different - I don't know if there's actually a convention for enum payload extracting functions (by the way, the request in #7604 did ask for as_variant_name()). But in any case I think that the function needs to name the variant name, and not the payload type.

(although I guess that the fact that both of these examples are generic enums does not really help my case as referring by type would be super weird)

Maybe we can extrapolate a rule which is consistent with both cases: if the type we're referencing is a generic value we should use the variant name. If it's not, we should reference the output type. I agree that NodeOrToken::as_n() and Result::t() wouldn't be very good names. However I also think that Value::as_str which returns &str is clearer than Value::as_text.

It does generate into_* methods with self as the parameter

Ah apologies, that's my bad. I probably wrongly copied / edited in playground; we can indeed disregard my comments on clippy 😅 .

However I'm still somewhat uncomfortable with auto-generating into functions which return Option. This seems like a fallible conversion for which try_into would be a better name. But instead of returning Option, Result may be a better fit.

The Result::Err path here is important because it allow recovering the original type through the error path in case the conversion fails. An example of this is TryFrom<Vec<T, A>> for [T; N] which has Vec<T, A> as the error type. In the Rowan example, if the conversion fails the original type is gone. The Copy bound helps work around that; but Rust-Analyzer shouldn't assume all structs to have Copy bounds, which makes this hard to generalize.

I'd prefer either into_x conversions which always succeed, or try_into_x conversions which return the original type in case they fail (or both?).

I still disagree with naming by type. I looked for more examples, and I found a couple (who would have guessed that non-error enums would be so difficult to find):

serde_json::Value. This is a fun one. First of, it has as_object, as_array, and even as_null. These make much more sense than as_map, as_vec, or as_unit because they speak in json terminology, so when reading the conversions it is a lot more obvious about what json kind is expected. Naming by type could have also lead to some very non-useful names - as_map could have also ended up being as_btree_map or as_index_map, which is just silly for manipulating json values. Now the interesting part is that this enum also has as_i64, as_u64, and as_f64. These are clearly about type conversion - there is no variant which would be named like that or would have a payload of corresponding type. Instead these check both for Number variant, and also that the payload is actually representable as the requested type.

I also grepped rust-lang/rust for similar pattern, and the only real example I found was SocketAddr::as_pathname. Return type is Path, but the name says "pathname" because it actually checks if it a pathname address.

I think serde_json example also shows that conversion to a type can have a more complex meaning than just matching for a single variant and that meaning would not be appropriate for an assist to guess. Also, remember the case when multiple variants have the same payload type. It would be weird for a function named as_str to be only about one of the variants holding a string. How would you even name the function for the other variant?

I agree with your point about try_into_x though, being able to recover the original value is useful. I changed the assist to instead generate try_into_x returning Result<Payload, Self>.

yoshuawuyts · 2021-02-14T20:32:50Z

crates/assists/src/handlers/generate_enum_match_method.rs

+// }
+//
+// impl Value {
+//     fn as_text(&self) -> Option<&String> {


Same as with the into_ assist; the function here should be parameterized by the output type rather than the enum variant name.

yoshuawuyts · 2021-02-14T20:41:38Z

crates/assists/src/handlers/generate_enum_match_method.rs

+    )
+}
+
+// Assist: generate_enum_into_method


These assists seem different enough from the existing one that it'd probably be a good idea to put them in their own files.

Done, that indeed does look a lot nicer. The VariantKind enum was a bit contrived to be useful for both is_ assist and the new ones, but now it's not needed anymore and just got replaced with a single match in each method!

yoshuawuyts · 2021-02-14T20:42:13Z

crates/assists/src/handlers/generate_enum_match_method.rs

 };

-// Assist: generate_enum_match_method
+// Assist: generate_enum_is_method


If we're renaming the method we should rename the file probably too.

jDomantas · 2021-02-27T11:00:47Z

Hey, my last comment was not replied to in a week. Do I still need to change it to generate the name based on type? I did not find the argument in favor of that convincing and personally I wouldn't have a use for this assist then.

@yoshuawuyts

yoshuawuyts · 2021-02-27T14:28:55Z

@jDomantas It seems you've taken my feedback into serious consideration, and made a clear enough argument that there's a case for either variant. Though this variant doesn't have my preference, I think the best step forward from here is to merge this and get feedback from users.

bors r+

bors · 2021-02-27T14:39:19Z

Build succeeded:

lnicola · 2021-03-01T06:57:18Z

yoshuawuyts suggested changes Feb 14, 2021

View reviewed changes

jDomantas added 9 commits February 27, 2021 12:05

rename existing assist to generate_enum_is_method

f675860

make generate_enum_is_method work on any variants

4ab285a

add generate_enum_into_method assist

2dcd5d7

deduplicate some

6427869

add generate_enum_as_method assist

e0f08fc

cargo fmt

6f25fef

move into_ and as_ generation to a separate file

f098a2b

rename generate_enum_match_method file to match assist name

d4b6cbe

generate try_into instead of into

558bcf4

jDomantas force-pushed the more-enum-matching branch from d0f4100 to 558bcf4 Compare February 27, 2021 10:24

bors bot merged commit 4a24edd into rust-lang:master Feb 27, 2021

jDomantas deleted the more-enum-matching branch February 27, 2021 20:59

jDomantas mentioned this pull request Mar 3, 2021

generate_enum_match doesn't work for tuple and struct variants #7601

Closed

More enum matching #7677

More enum matching #7677

Uh oh!

Conversation

jDomantas commented Feb 14, 2021

Uh oh!

matklad commented Feb 14, 2021

Uh oh!

bors bot commented Feb 14, 2021

Uh oh!

yoshuawuyts left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veykril Feb 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Function name

Output type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jDomantas Feb 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jDomantas commented Feb 27, 2021

Uh oh!

yoshuawuyts commented Feb 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors bot commented Feb 27, 2021

Uh oh!

lnicola commented Mar 1, 2021

Uh oh!

Uh oh!

Veykril Feb 14, 2021 •

edited

Loading

jDomantas Feb 15, 2021 •

edited

Loading

yoshuawuyts commented Feb 27, 2021 •

edited

Loading