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

Attributes containing keywords can't be parsed with 0.20 #238

Open
Florob opened this issue May 25, 2023 · 8 comments
Open

Attributes containing keywords can't be parsed with 0.20 #238

Florob opened this issue May 25, 2023 · 8 comments
Labels

Comments

@Florob
Copy link

Florob commented May 25, 2023

The code below worked in 0.14.2. It uses type inside a derive macro attribute.

As of 0.20 this yields:

Error { kind: Custom("expected identifier, found keyword `type`"), locations: ["bar"], span: Some(Span) }
use darling::{ast, FromDeriveInput, FromField, FromMeta};
use quote::quote;
use syn::{parse_quote, DeriveInput};

#[derive(Clone, Debug, FromMeta)]
#[darling(rename_all = "snake_case")]
enum Type {
    A,
    B,
}

#[derive(Clone, Debug, FromDeriveInput)]
#[darling(attributes(myderive), supports(struct_named))]
struct MyDeriveReceiver {
    data: ast::Data<(), MyDeriveFieldReceiver>,
}

#[derive(Clone, Debug, FromField)]
#[darling(attributes(myderive))]
struct MyDeriveFieldReceiver {
    #[darling(rename = "type")]
    my_type: Option<Type>,
}

fn main() {
    let input = parse_quote! {
        #[derive(MyDerive)]
        struct Foo {
            #[myderive(type = "a")]
            bar: (),
        }
    };
    let receiver = MyDeriveReceiver::from_derive_input(&input).unwrap();
}
@TedDriggs
Copy link
Owner

Attempting to repro this, I discovered something interesting. This errors:

let _attr: syn::Attribute = syn::parse_quote!(#[type = false]);

But this doesn't:

let _attr: syn::Attribute = syn::parse_quote!(#[hello(type = false)]);

It seems like it may matter to syn whether the keyword is the outermost item, or is inside a Meta wrapper.

Alternatively, the issue is in NestedMeta, which does some custom parsing. That seems to be looking for an identifier, so I it's possible that needs to allow for keywords instead. @jonasbb is that something you'd be able to take a look at?

@TedDriggs
Copy link
Owner

I've filed dtolnay/syn#1458 to request that syn handle this scenario without errors.

@jonasbb
Copy link
Contributor

jonasbb commented May 25, 2023

I can take another look next week, but it looks a bit like dtolnay/syn#1414
Basically, type = false is not a valid Meta syntax in Rust and syn. I guess the DeriveInput gets recursively parsed into a Meta or NestedMeta somewhere. No chance of getting any support from syn, because of its maintainer.
This is how I worked around that for serde_with: jonasbb/serde_with@c22f165 (#578), replace as with r#as such that it is valid Meta.

@TedDriggs
Copy link
Owner

As noted in #1414 it seems that keyword identifiers are fine as long as they're inside the List portion; is there a way that NestedMeta could reimplement Meta parsing to preserve that behavior?

@jonasbb
Copy link
Contributor

jonasbb commented May 25, 2023

NestedMeta works.

let nm: darling::ast::NestedMeta = syn::parse_quote!(myderive(type = "a"));

You just cannot expect that the inside is a well-formed meta again. Probably from_derive_input assumes that (recursive parsing). darling has no from_tokenstream for directly parsing. syn has no type supporting keywords.
Dropping Meta and doing something separate from syn would work.

@TedDriggs
Copy link
Owner

TedDriggs commented May 25, 2023

It's possible to hand-construct a syn::Meta that has an Ident in its path field which is a keyword, right? The issue is that attempting to construct it using Parse fails?

If so, then would a darling-local copy of the Meta parsing code which used a different path parsing function work?

@jonasbb
Copy link
Contributor

jonasbb commented May 29, 2023

It's possible to hand-construct a syn::Meta that has an Ident in its path field which is a keyword, right?

I am not sure if that is true, but yes if it is, then copying and adapting the Meta parsing code could work. I would be a bit wary about it. dtolnay likes breaking code that works and I feel that using Meta in such a form is something that could break at any point.

Instead of using Meta it might work to rely more on syn::parse::Parse to directly process the TokenStream that is embedded in a MetaList.

@wcampbell0x2a
Copy link

Is there a current hack to get around this issue?

github-merge-queue bot pushed a commit to apollographql/subgraph-template-rust-async-graphql that referenced this issue Aug 29, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [async-graphql](https://togithub.com/async-graphql/async-graphql) |
dependencies | major | `5.0.10` -> `6.0.4` |
| [async-graphql-axum](https://togithub.com/async-graphql/async-graphql)
| dependencies | major | `5.0.10` -> `6.0.4` |

---

### Release Notes

<details>
<summary>async-graphql/async-graphql (async-graphql)</summary>

###
[`v6.0.4`](https://togithub.com/async-graphql/async-graphql/blob/HEAD/CHANGELOG.md#604-2023-08-18)

- Parse "repeatable" in directive definitions.
[#&#8203;1336](https://togithub.com/async-graphql/async-graphql/pull/1336)
- add support `multipart/mixed` request.
[#&#8203;1348](https://togithub.com/async-graphql/async-graphql/issues/1348)
-   async-graphql-actix-web: add `GraphQL` handler.
-   async-graphql-axum: add `GraphQL` service.

###
[`v6.0.3`](https://togithub.com/async-graphql/async-graphql/blob/HEAD/CHANGELOG.md#603-2023-08-15)

- dynamic: fix the error that some methods of `XXXAccessor` return
reference lifetimes that are smaller than expected.
- dynamic: no longer throws an error if the Query object does not
contain any fields but the schema contains entities.
- chore: make accessors public and reexport indexmap
[#&#8203;1329](https://togithub.com/async-graphql/async-graphql/pull/1329)
- feat: added `OutputType` implementation for `std::sync::Weak`
[#&#8203;1334](https://togithub.com/async-graphql/async-graphql/pull/1334)

###
[`v6.0.1`](https://togithub.com/async-graphql/async-graphql/blob/HEAD/CHANGELOG.md#601-2023-08-02)

-   dynamic: remove `TypeRefInnner`
-   update MSRV to `1.67.0`

###
[`v6.0.0`](https://togithub.com/async-graphql/async-graphql/blob/HEAD/CHANGELOG.md#600-2023-07-29)

-   Bump `syn` from `1.0` to `2.0`
-   Bump `darling` from `0.14` to `0.20`
-   Bump `indexmap` from `1.6.2` to `2`
- Attributes `guard`, `process_with`, `complexity` support expression or
string as value
[#&#8203;1295](https://togithub.com/async-graphql/async-graphql/issues/1295)
- Schema (type) level directive support with optional support of
federation composeDirective
[#&#8203;1308](https://togithub.com/async-graphql/async-graphql/pull/1308)
- Add support for generic structs derriving InputObject and SimpleObject
[#&#8203;1313](https://togithub.com/async-graphql/async-graphql/pull/1313)
- chore: trim up some unnecessary code
[#&#8203;1324](https://togithub.com/async-graphql/async-graphql/pull/1324)
- Adds `Dataloader::get_cached_values` method to the dataloader cache so
that callers can access the contents of the cache without knowing the
keys.
[#&#8203;1326](https://togithub.com/async-graphql/async-graphql/pull/1326)

#### Breaking Changes

- Since `syn 2.0` no longer supports keywords as meta path, rename the
parameter used to specify interface field types from `type` to `ty`.


[dtolnay/syn#1458

[TedDriggs/darling#238

```rust

#[derive(Interface)]
#[graphql(field(name = "id", ty = "&i32"))] // rename from type to ty
enum Node {
    MyObj(MyObj),
}
```

- Change the parameter `location` of the macro `Directive` to
*PascalCase*

```rust
// #[Directive(location = "field")]

#[Directive(location = "Field")]
pub fn lowercase() -> impl CustomDirective {
    LowercaseDirective
}
```

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these
updates again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/apollographql/subgraph-template-rust-async-graphql).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi42OC4xIiwidXBkYXRlZEluVmVyIjoiMzYuNjguMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants