Derive FromParam for Enum #2832

loikki · 2024-07-25T15:03:01Z

Implement #2826

the10thWiz

Overall, this is a great start.

Ideally, we should include some ui-fail tests with this macro, see core/codegen/tests/ui-fail/from_form.rs for some examples.

@SergioBenitez, I think this should be good to go with the changes I've recommended, let us know if you have any other comments.

core/codegen/src/derive/from_param.rs

core/codegen/src/lib.rs

core/codegen/tests/from_param.rs

core/codegen/src/lib.rs

loikki · 2024-08-02T15:25:03Z

Hi @the10thWiz

Thanks for the feedback. I have applied all your suggestions but I am not sure what is the point of core/codegen/tests/ui-fail/from_form.rs.

From the name, I guess everything should fail in it right? How do you check that each structure fails? In my understanding, just one failing would be sufficient for the test to pass, right?

the10thWiz · 2024-08-02T17:01:00Z

In my understanding, just one failing would be sufficient for the test to pass, right?

Not in this case. We use trybuild to run the ui-fail test cases, which actually validates the compiler output generated by the build failures. When you run the test, you can set an evironment variable to have trybuild save the current outpuut.

Use ./scripts/test.sh --ui to execute the ui tests locally. (They are ignored by default, and only run on one platforms in CI since they can be somewhat finnicky).

loikki · 2024-08-02T21:25:35Z

Thanks for the explanations. trybuild looks like a nice project :)

I have updated the tests with the correct stderr. Should I add it to nightly too or just stable?

the10thWiz

I think we're very close! In terms of the ui-fail test, I think we will need a nightly version, but I might just take care of it myself.

the10thWiz · 2024-08-03T03:23:21Z

core/codegen/src/derive/from_param.rs

+                   .fields_validate(|_, fields| {
+                       if !fields.is_empty() {
+                           return Err(fields.span().error("Only empty enums are accepted"));
+                       }
+
+                       Ok(())
+                   })
+        )
+        .inner_mapper(MapperBuild::new()
+                      .enum_map(|_, data| {
+                          let mut matches = vec![];
+
+                          for field in data.variants() {
+                              let field_name = &field;
+                              matches.push(quote!(
+                                  stringify!(#field_name) => Ok(Self::#field_name),
+                              ))
+                          }
+
+                          quote! {
+                              type Error = &'a str;
+                              fn from_param(param: &'a str) -> Result<Self, Self::Error> {
+                                  match param {
+                                      #(#matches)*
+                                      _ => Err("Failed to find enum")
+                                  }
+                              }
+                          }
+                      })


Can you take a look at the indentation here?

I don't see any issue with it. I checked with the auto-indent in my IDE and with cargo fmt --check.

Rocket intentionally disables rustfmt (cargo fmt), since it doesn't handle some of the more complex trait bounds well. I suspect your IDE's auto-indent is based on rustfmt as well (I don't think there is another well built option out there yet), so that's likely why it's not picking up on the issues here.

The specific issue here is that not all the indentations are the same size (This is extremely obvious in my editor, since I have indent guides turned on). If you would like, I can push a fixed version to this PR.

Even after enabling the indent guide, I don't see it. So it would be nice if you can do it.

core/codegen/tests/ui-fail/from_param.rs

core/codegen/src/lib.rs

loikki · 2024-08-06T08:33:26Z

I have pushed the requested changes and wrote a few comments

the10thWiz · 2024-08-07T01:45:22Z

You're close, just a few things to fix. The indentation on the one file needs to be fixed, and the test failure in the one doc-test needs to be addressed.

Other than that, I'm quite happy with this feature.

SergioBenitez · 2024-08-07T03:14:40Z

core/codegen/tests/ui-fail-nightly/from_param.stderr

+error: [note] error occurred while deriving `FromParam`
+ --> tests/ui-fail-stable/from_param.rs:8:10
+  |
+8 | #[derive(FromParam)]
+  |          ^^^^^^^^^
+  |
+  = note: this error originates in the derive macro `FromParam` (in Nightly builds, run with -Z macro-backtrace for more info)


It's surprising that this isn't an actual note, since they exist on nightly. Is this being tested? Is this the output that's actually being produced?

My bad. I supposed that it would produce the same output. Just tested and fixed it.

SergioBenitez · 2024-08-07T03:15:54Z

core/codegen/src/derive/from_param.rs

+        .validator(ValidatorBuild::new()
+                   .fields_validate(|_, fields| {
+                       if !fields.is_empty() {
+                           return Err(fields.span().error("Only empty enums are accepted"));


Maybe the wording here should be ~~"only enums without variants are supported"~~ ah, I confused myself. The actual constraint here is "only enum variants with nullary (zero-length) fields".

Probably Only enums without data fields are supported.

I think the language I was looking for was getting away form "enums with foo" and instead going for "enum variants with foo," since that's the actual constant. We should also make sure the error is on the fields themselves.

We should also make sure the error is on the fields themselves. Do you mean the error message?

He means the associated span, and it looks like you already have it correct.

SergioBenitez · 2024-08-07T03:17:13Z

core/codegen/src/derive/from_param.rs

+                          let mut matches = vec![];
+
+                          for field in data.variants() {
+                              let field_name = &field;
+                              matches.push(quote!(
+                                  stringify!(#field_name) => Ok(Self::#field_name),
+                              ))
+                          }


This can probably be a .iter().map(). I don't recall if quote can use the iterator directly, but if it can, then that would save us the allocation here.

core/codegen/src/lib.rs

SergioBenitez

Does this correctly handle raw idents? IE:

enum Foo {
    r#for,
}

// parse "for" as `Foo::r#for`

Finally, it want to note that this macro is almost exactly the FromFormField macro except that macro also allows renaming fields. Is there a reason to have two implementations, or can we simply factor out the existing FromFormField implementation for FromParam?

loikki · 2024-08-07T19:44:24Z

I agree that FromFormField is very similar. I have little experience with rust macros and especially with devise (which is extremely badly documented). So I am not sure how would be the best approach to do it.

I am just wondering if it makes sense to merge them together. Is there any chance that FromFormField will be extended to more complex structures? FromParam is really restricted to "object" that can be matched to string as it is used in the query path.

By the way, raw identifiers are not working. How would do you like to proceed? Removing the r# from the string or add a parameter to rename the field (e.g. #[field(value="for")] for FromFormField.

the10thWiz

Looks great! I'll take care of formatting and raw idents (Ident has an unraw method that strips the r#), and merge these changes.

Looking forward, I think we will want to add support for a rename attribute, as well as refactor both this and the FromFormField derive to share code as much as possible. FromFormField may eventually wind up with a more complex macro, since it does have quite a bit more context (i.e. the name, and potentially a whole file). It also matches case-insensitively, but I think the difference in behavior is justified.

the10thWiz · 2024-08-08T03:54:36Z

Merged in 38cebeb!

Derive FromParam for Enum rwf2#2826

9746832

the10thWiz requested changes Aug 2, 2024

View reviewed changes

Apply suggestions

9df9014

loikki force-pushed the implement-derive-parafrom branch from 079ee9f to 9df9014 Compare August 2, 2024 15:25

Correctly implement failed tests

69dac9a

the10thWiz requested changes Aug 3, 2024

View reviewed changes

Add nightly ui-fail test + update ui-fail, add assert in doc string

01f6762

SergioBenitez reviewed Aug 7, 2024

View reviewed changes

core/codegen/src/lib.rs Show resolved Hide resolved

SergioBenitez reviewed Aug 7, 2024

View reviewed changes

core/codegen/src/lib.rs Show resolved Hide resolved

SergioBenitez reviewed Aug 7, 2024

View reviewed changes

Apply suggestions

fe8506d

the10thWiz approved these changes Aug 8, 2024

View reviewed changes

the10thWiz closed this Aug 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Derive FromParam for Enum #2832

Derive FromParam for Enum #2832

loikki commented Jul 25, 2024

the10thWiz left a comment

loikki commented Aug 2, 2024

the10thWiz commented Aug 2, 2024

loikki commented Aug 2, 2024

the10thWiz left a comment

the10thWiz Aug 3, 2024

loikki Aug 3, 2024

the10thWiz Aug 7, 2024

loikki Aug 7, 2024

loikki commented Aug 6, 2024

the10thWiz commented Aug 7, 2024

SergioBenitez Aug 7, 2024

loikki Aug 7, 2024

SergioBenitez Aug 7, 2024 •

edited

Loading

the10thWiz Aug 7, 2024

SergioBenitez Aug 7, 2024

loikki Aug 7, 2024

the10thWiz Aug 7, 2024

SergioBenitez Aug 7, 2024 •

edited

Loading

SergioBenitez left a comment •

edited

Loading

loikki commented Aug 7, 2024

the10thWiz left a comment

the10thWiz commented Aug 8, 2024

Derive FromParam for Enum #2832

Derive FromParam for Enum #2832

Conversation

loikki commented Jul 25, 2024

the10thWiz left a comment

Choose a reason for hiding this comment

loikki commented Aug 2, 2024

the10thWiz commented Aug 2, 2024

loikki commented Aug 2, 2024

the10thWiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loikki commented Aug 6, 2024

the10thWiz commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

SergioBenitez left a comment • edited Loading

Choose a reason for hiding this comment

loikki commented Aug 7, 2024

the10thWiz left a comment

Choose a reason for hiding this comment

the10thWiz commented Aug 8, 2024

SergioBenitez Aug 7, 2024 •

edited

Loading

SergioBenitez Aug 7, 2024 •

edited

Loading

SergioBenitez left a comment •

edited

Loading