Initial derive-Error proc-macro implementation #103

ffuugoo · 2019-11-04T11:56:40Z

First part of #92.

Not based on utils::State and supports only source attribute/method, so far.

Attribute syntax is the one suggested by @tyranron. Lots of examples in tests/error.rs.

ffuugoo · 2019-11-04T11:59:12Z

JelteF · 2019-11-04T12:13:22Z

Thank you for opening the WIP PR. I do still want you to use utils::State.

The main reason I want this to use utils::State is so that it uses the same attribute semantics as all the other derives. For example, these two would be equivalent.

#[derive(Error)]
struct WithoutSource(#[error(not(source))] i32);

#[derive(Error)]
#[error(not(source))] 
struct WithoutSource(i32);

Also, the not semantics will be useful for the attributes of other derives as well. And finally, utils::State also standardizes the parsing of enums. So this code for error could be used as an example to implement #99.

JelteF · 2019-11-04T12:15:57Z

When using utils::State ignore the fact that other attribute parameters defined in utils (e.g. forward or ref), would be allowed (and ignored) for now. This is a separate issue and is tracked in #104.

tyranron

@ffuugoo ☝️

JelteF · 2019-11-06T08:44:09Z

Another nice thing that the Error derive would get as a free benefit from State is this (once it's implemented): #102

JelteF · 2019-11-07T08:28:00Z

@ffuugoo FYI, I've implemented checking of supported attribute parameters for State on master now.

ffuugoo · 2019-11-07T08:31:26Z

@ffuugoo FYI, I've implemented checking of supported attribute parameters for State on master now.

This is great.

I've worked on moving derive-Error to using State and I have few concerns. I'm writing an elaborate explanation about them RN.

ffuugoo · 2019-11-07T08:50:48Z

So, I've worked on on moving derive-Error to using State I realized that the way State process (and expose) attributes is not flexible enough for implementing derive-Error attribute-interface that was agreed upon in #92.

So, as far as I understand, the way State currently parses attributes is:

State::new constructs "default" FullMetaInfo structure
Attributes are parsed on everything (enums/structs, enum variants, fields) as MetaInfo
Parsed MetaInfo converted to FullMetaInfo by replacing all unspecified fields with values from "default" FullMetaInfo

The problem is, if we want to implement generic not predicate for attributes, we have to distinguish three cases: explicitly-enabled (e.g., [#error(source)]), explicitly-disabled (e.g., [#error(not(source))]) and not-specified.

So, I see two ways going from here:

Discard the idea of generic not predicate and use ignore instead. It will make possible attribute formats less flexible, but it requires minimal changes to State and won't break existing code.
Implement generic not predicate and expose original MetaInfo in addition to FullMetaInfo in some way.

JelteF · 2019-11-07T09:10:12Z

@ffuugoo Awesome that you're working so much on this! It's a shame that the current MetaInfo approach is not flexible enough, but I already expected to run into it at some point. I mainly wanted to keep it simple while it was not needed yet. I think option 2 would be most desirable, the not predicate would also be very useful for other derives. One simple way of exposing MetaInfo is by embedding it into FullMetaInfo as a field.

JelteF · 2019-11-07T09:18:54Z

As far as I understand embedding it should also not break existing code.

ffuugoo · 2019-11-07T09:24:57Z

I also really don't like the whole MetaInfo/FullMetaInfo idea. Why not just use a HashMap<T, bool>? That's what I did in my derive-Error implementation.

This way we:

Can build much more generic attribute parsing code (as we won't have to manually extend MetaInfo/FullMetaInfo with new fields and manually modify map-attribute-to-structure-field code each time new macro is added).
If we really want to be type safe, we can get a trade-off (I've used this exact approach in current derive-Error implementation, check it for an example):
- define an enum with variants representing all the valid attributes
- define a function, mapping enum variant to attribute name (e.g., derive Display on it)
- define const array with all enum variants (so we can iterate on all valid attributes)
- btw, all of the above can be done with a derive-proc-macro; and chances are, it's already implemented somewhere on crates.io (:
- with such architecture we can ensure (statically-type-safe ensure, I must say), that only a well-known limited set of attributes can be used (I mean, used by clients of State) and have a generic attributes parser implementation that requires absolutely minimal changes to a well scoped piece of code (i.e., enum, enum-to-str function and enum-variants array)
Automatically get "explicitly-enabled/disabled or not-specified" distinction (cause HashMap::get returns Option<bool>, where None is obvious "not-specified").

The only real downside I see, is that there is no trivial way to "default-merge" a HashMap as you do with MetaInfo/FullMetaInfo. But I can see two partial solutions that are, probably, good enough:

is_explicitly_enabled/is_explicitly_disabled methods:

fn is_explicitly_enabled<T>(hash_map: &HashMap<T, bool>, key: &T) -> bool {
    match hash_map.get(key) {
        Some(true) => true,
        _ => false,
      }
}

merge it with well-known pre-filled HashMap and use hash_map.get(key).unwrap()

ffuugoo · 2019-11-07T09:27:55Z

One simple way of exposing MetaInfo is by embedding it into FullMetaInfo as a field.

Didn't think about it. Seems simple/good enough. I'll see how far I can get with this. (:

JelteF · 2019-11-07T10:05:21Z

I'm definetly not sold on the current design of MetaInfo/FullMetaInfo. The main reason it is like it is now is because it was simple to write/understand and did the job. If embedding FullMetaInfo is good enough for you I think doing that is best for now, since it seems another simple solution.

In the future I definitely think rethinking the MetaInfo/FullMetaInfo approach is a good idea. Things to keep in mind though:

A simple Hashmap<T, bool> doesn't work for the value parameter described in Add a default and value attribute parameters #102, since it's not a boolean.
Using an enum you would still have to map the strings to a variant, which seems about as much work as mapping them to a field (I might be wrong here though).

ffuugoo · 2019-11-07T10:15:52Z

Using an enum you would still have to map the strings to a variant, which seems about as much work as mapping them to a field (I might be wrong here though).

#[derive(Copy, Clone, <stuff required for HashMap key>)]
enum ValidAttr {
  Source,
  Backtrace,
};

impl ValidAttr {
  fn str(&self) -> &'static str {
    match self {
      Self::Source => "source",
      Self::Backtrace => "backtrace",
    }
  }
}

const VALID_ATTRS: &[ValidAttr] = [
  ValidAttr::Source,
  ValidAttr::Backtrace,
];

fn test(parsed_attrs: &mut HashMap<ValidAttr, bool>, path: &Path) {
  let attr = VALID_ATTRS.find(|attr| path.is_ident(attr.str()));
  parsed_attrs.insert(attr, true);
}

Kinda the same amount of work, but very trivial and well scoped.

JelteF · 2019-11-07T15:37:18Z

I see you added some more commits. I'll try to take a look at them soon-ish. One thing that's important, I want to release this with some preliminary backtrace() support. Since I would like not to make a backwards incompatible change on this. The important part for this is the automatic detection of the backtrace field, i.e. check if the type is Backtrace for tuple or if the field is backtrace for struct version. The backtrace attribute doesn't necessarily need to be implemented, since that can be added be without a backward compatibility concern. The default behaviour should be what we want for the final version though.

tyranron · 2019-11-07T15:55:19Z

@JelteF I suggest to implement backtrace() support in a separate PR after this one, so keeping PRs simple and iterative.

JelteF · 2019-11-07T16:20:11Z

@tyranron I think that indeed makes sense, I've changed this PR to merge into the error branch of my repo (which is a copy of master). That way we can keep the features in separate PRs, but merge them into master at the same time (by merging error into master once all features are merged in there).

@ffuugoo For clarity, is this one still just a WIP PR? Or is it ready for a full review.
Also, feel free to open the backtrace PR already as well. It's possible you can get that working before I have time to review this one.

tyranron · 2019-11-07T17:18:48Z

@JelteF still WIP. We will remove the WIP: prefix from PR's name once it's ready.

JelteF

I left some preliminary comments, mostly with ideas to better utilize State.

JelteF · 2019-11-07T22:08:48Z

src/error.rs

+where
+    P: Fn(&syn::Field, usize) -> bool,
+{
+    let fields = state.enabled_fields_data();


Let's use unpacking here to make the rest of the code clearer.

JelteF · 2019-11-07T22:24:25Z

src/error.rs

+                info,
+            )?,
+
+            None => process_if_valid_default_field_for_attr(


Let's split this into two loops. First checking all fields for explicit setting of the source and only if that's not done try to infer it. I think the flow is a bit clearer that way and some of the error cases below disappear.

JelteF · 2019-11-07T22:29:25Z

src/error.rs

+}
+
+impl<'a> ParsedField<'a> {
+    fn render_as_struct_field_ident(&self) -> TokenStream {


usage of this function can be replaced by multi_field_data.members[self.index]

JelteF · 2019-11-07T22:39:04Z

tests/error.rs

+    fn named_implicit_source() {
+        #[derive(Default, Debug, Display, Error)]
+        #[display(fmt = "")]
+        struct S<SS, T> {


This type parameter is quite confusing since the above error type is also called SS. Let's change it to E (also for the types below).

JelteF · 2019-11-07T22:41:24Z

tests/error.rs

+
+    #[derive(Debug, Display, Error)]
+    #[display(fmt = "")]
+    enum E<S, T> {


Also here, let's rename this S type parameter such that it doesn't shadow the actual type.

JelteF · 2019-11-07T22:56:53Z

src/error.rs

+        }
+    }
+
+    fn render_as_enum_variant_struct_match_arm_tail(&self, len: usize) -> TokenStream {


This an the next function can be replaced by a call to MultiFieldData.initializer.
Something like this:

let bindings: Vec<_> = (0..len).map(|index| { if index == self.index { quote!(source) } else { quote!(_) } }).collect(); multi_field_data.initializers(&bindings)

I think this also allows you to remove derive_type from ParsedFields

I just added multi_field_data.matcher, now you can do:

multi_field_data.matcher(&[self.index], &[quote!(source)])

ffuugoo · 2019-11-08T10:48:44Z

@JelteF Should have fixed all of your comments now.

ffuugoo · 2019-11-08T10:49:39Z

@tyranron Take a look as well, pls.

tyranron

@ffuugoo

Take a look as well, pls.

Well enough as for me. Remove the WIP: prefix, please.

JelteF · 2019-11-10T14:01:56Z

src/error.rs

+        enum_: vec!["ignore"],
+        struct_: vec!["ignore"],
+        variant: vec!["ignore"],
+        field: vec!["ignore", "source"],


There's no tests at all for the ignore attribute. Let's either add tests or disallow the ignore attribute if it doesn't make sense for error (which I think is the case).

It does not make much sense, but it does not contradict the design as well. Added tests for possible ignore cases.

src/utils.rs

JelteF · 2019-11-10T14:31:46Z

src/error.rs

+            &generics,
+            quote! {
+                where
+                    #(#bounds: ::std::fmt::Debug + ::std::fmt::Display + ::std::error::Error + 'static),*


Are the Debug and Display bounds necessary? Doesn't Error already require those?

Seems so. Tests fail to compile for me if I remove these extra bounds.

src/error.rs

JelteF · 2019-11-10T14:42:22Z

Cargo.toml

+[[test]]
+name = "error"
+path = "tests/error.rs"
+required-features = ["error"]


This should have failed in CI, since the test actually requires the display feature as well for the Display derives. I've updated master with a small change to the testing so that this should now fail.

Let's rename this test to error_full and add display to required-features.
Apart from that let's create a small error test, with a some simple Error derives that have a simple Display implementation written out so that we test that the error feature works.

Removed display dependency for tests/error altogether.

JelteF · 2019-11-10T16:35:10Z

src/parsing.rs

@@ -23,7 +23,7 @@ pub type ParseResult<T> = Result<T, ParseError>;
 impl ::std::fmt::Display for ParseError {
    fn fmt(
        &self,
-        fmt: &mut ::std::fmt::Formatter<'_>,


Changes to this file seem unnecessary. Let's undo those.

JelteF · 2019-11-10T16:40:13Z

src/utils.rs

+        match meta {
+            Meta::List(list) if list.path.is_ident("not") => {
+                if value {
+                    parse_punctuated_nested_meta(


Because this is called recursively it allows #[error(not(not(source))], which is confusing as it's the same as #[error(not(source))]. Let's call another function here, that doesn't allow any Meta::List.

It's not. if value conditional only allows for a single top-level "not" or fails with an error otherwise.

Ah I see, can you add a comment explaining that check then?

… tests

Co-Authored-By: Jelte Fennema <github-tech@jeltef.nl>

ffuugoo · 2019-11-15T12:11:08Z

@JelteF

JelteF

Looks good to me now, but there are some merge conflicts with the latest master. If you solve those we can merge it into the error branch. FYI I'll probably only have time to review the changes for Backtrace somewhere in December.

ffuugoo · 2019-11-18T12:14:53Z

@JelteF

JelteF · 2020-03-28T14:19:46Z

This is now released in version 0.99.5

Implement derive-Error proc-macro

d60a64e

ffuugoo changed the title ~~WIP: Initial derive-Error proc-macro implementation~~ WIP: Initial derive-Error proc-macro implementation (#92) Nov 4, 2019

tyranron requested changes Nov 5, 2019

View reviewed changes

ffuugoo added 3 commits November 7, 2019 21:47

Rework derive-Error to use 'utils::State'

7c2bc77

clippy

13366a7

rustfmt

526b631

JelteF changed the base branch from master to error November 7, 2019 16:16

JelteF requested changes Nov 7, 2019

View reviewed changes

ffuugoo added 5 commits November 8, 2019 20:15

Rework derive-Error to further utilize 'utils::State'

a0a1240

rustfmt

cd49a40

Merge branch 'master' into implement-initial-derive-error-macro

f757eff

rustfmt

5bffb60

Tiny codestyle fix

4e086b2

tyranron added 4 commits November 8, 2019 15:35

Refactor tests

97bd7c9

Correct docs

8c79479

Some code style corrections

66f45a7

Fix fmt

1fcfb84

tyranron approved these changes Nov 8, 2019

View reviewed changes

JelteF requested changes Nov 10, 2019

View reviewed changes

ffuugoo added 2 commits November 11, 2019 15:07

Revert changes unrelated to derive-Error

11df5ce

Remove 'tests/error' dependency on 'Display' feature and add 'ignore'…

c8b6259

… tests

ffuugoo changed the title ~~WIP: Initial derive-Error proc-macro implementation (#92)~~ Initial derive-Error proc-macro implementation (#92) Nov 11, 2019

ffuugoo marked this pull request as ready for review November 11, 2019 07:01

Small clarification in error message text

8234365

Co-Authored-By: Jelte Fennema <github-tech@jeltef.nl>

ffuugoo requested a review from JelteF November 11, 2019 07:03

Add clarification comment

ad3b84d

JelteF approved these changes Nov 17, 2019

View reviewed changes

Merge branch 'master' into implement-initial-derive-error-macro

3d8017f

JelteF changed the title ~~Initial derive-Error proc-macro implementation (#92)~~ Initial derive-Error proc-macro implementation Nov 18, 2019

JelteF merged commit cde058f into JelteF:error Nov 18, 2019

ffuugoo deleted the implement-initial-derive-error-macro branch November 18, 2019 12:26

JelteF pushed a commit that referenced this pull request Mar 28, 2020

Initial derive-Error proc-macro implementation (#103)

f103ff9

Initial derive-Error proc-macro implementation #103

Initial derive-Error proc-macro implementation #103

Conversation

ffuugoo commented Nov 4, 2019 • edited Loading

ffuugoo commented Nov 4, 2019

JelteF commented Nov 4, 2019 • edited Loading

JelteF commented Nov 4, 2019 • edited Loading

tyranron left a comment

Choose a reason for hiding this comment

JelteF commented Nov 6, 2019 • edited Loading

JelteF commented Nov 7, 2019

ffuugoo commented Nov 7, 2019

ffuugoo commented Nov 7, 2019

JelteF commented Nov 7, 2019

JelteF commented Nov 7, 2019

ffuugoo commented Nov 7, 2019 • edited Loading

ffuugoo commented Nov 7, 2019

JelteF commented Nov 7, 2019

ffuugoo commented Nov 7, 2019 • edited Loading

JelteF commented Nov 7, 2019

tyranron commented Nov 7, 2019

JelteF commented Nov 7, 2019

tyranron commented Nov 7, 2019

JelteF 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffuugoo commented Nov 8, 2019

ffuugoo commented Nov 8, 2019

tyranron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffuugoo Nov 11, 2019 • 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

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

Choose a reason for hiding this comment

ffuugoo commented Nov 15, 2019

JelteF left a comment

Choose a reason for hiding this comment

ffuugoo commented Nov 18, 2019

JelteF commented Mar 28, 2020

ffuugoo commented Nov 4, 2019 •

edited

Loading

JelteF commented Nov 4, 2019 •

edited

Loading

JelteF commented Nov 4, 2019 •

edited

Loading

JelteF commented Nov 6, 2019 •

edited

Loading

ffuugoo commented Nov 7, 2019 •

edited

Loading

ffuugoo commented Nov 7, 2019 •

edited

Loading

ffuugoo Nov 11, 2019 •

edited

Loading