Cargo fix generates broken code for type names passed to derives

[sgrif]

When a type is given to a proc macro as a string, such as in https://github.com/diesel-rs/diesel/blob/6e2d467d131030dc0bbce4e4801c7bee7bcbf0dd/diesel/src/type_impls/primitives.rs#L20, cargo fix --edition will incorrectly generate crate::"::old_type::Path". If there are nested types inside, such as https://github.com/diesel-rs/diesel/blob/6e2d467d131030dc0bbce4e4801c7bee7bcbf0dd/diesel/src/type_impls/primitives.rs#L42, they won't be touched at all.

[ehuss]

Moved to rust-lang/rust, as the fix suggestions are generated by rustc.

It might help to include a minimal reproduction, along with details about what the before and after looks like. Diesel is quite large, and it can be difficult to untangle what is going on from what you linked.

[sgrif]

Thanks, @ehuss! A minimal reproduction would be this:

#[derive(diesel::AsExpression)]
#[sql_type = "::Bar"]
struct Foo;

struct Bar;

cargo fix --edition will turn that into:

#[derive(diesel::AsExpression)]
#[sql_type = crate::"::Bar"]
struct Foo;

struct Bar;

and it can be difficult to untangle what is going on from what you linked.

I suspect the problem here is that we are parsing the given string as a type (since that's the only type that could be used to reference a type in a meta attribute), keeping the span of the original string. It appears as though it's just naively putting crate:: in front of whatever the span points to. There could be some special casing here, but the problem is worse when generics are involved.

#[derive(diesel::AsExpression)]
#[sql_type = "Option<::Bar>"]
struct Foo;

struct Bar;

gets the same treatment:

#[derive(diesel::AsExpression)]
#[sql_type = crate::"Option<::Bar>"]
struct Foo;

struct Bar;

Since the APIs on Span for us to only point at the relevant characters don't exist, cargo fix should probably be bailing if the tokens pointed to aren't a valid type.

[ehuss]

I was thinking of something more along the lines of not using diesel, since there's a lot of machinery there. Something like this:

// A proc-macro crate named "pmf".
extern crate proc_macro;
extern crate syn;
extern crate quote;
use proc_macro::TokenStream;
use quote::quote;

#[proc_macro_derive(Foo, attributes(helper))]
pub fn foo(tokens: TokenStream) -> TokenStream {
    let s = syn::parse_macro_input!(tokens as syn::DeriveInput);
    let helper = s.attrs.iter().filter(|attr| attr.path.is_ident("helper")).next().unwrap();
    let name = match helper.parse_meta().unwrap() {
        syn::Meta::NameValue(v) => v.lit,
        _ => panic!("expected helper=lit"),
    };
    let name_str = match name {
        syn::Lit::Str(s) => s,
        _ => panic!("expected string"),
    };
    let ty: syn::Type = name_str.parse().unwrap();  // KEY POINT HERE
    let ts = quote!{
        impl Foo for #ty {}
    };
    TokenStream::from(ts)
}

// A 2015-edition crate using the proc-macro "pmf".
#![warn(absolute_paths_not_starting_with_crate)]
extern crate pmf;

pub trait Foo {}

#[derive(pmf::Foo)]
#[helper = "::Bar"]
pub struct S;

pub struct Bar;

Results in:

warning: absolute paths must start with `self`, `super`, `crate`, or an external crate name in the 2018 edition
 --> tests/t1.rs:7:12
  |
7 | #[helper = "::Bar"]
  |            ^^^^^^^ help: use `crate`: `crate::"::Bar"`
  |

I think the key part here is that diesel is using syn to convert a token from a string to a Type, which is probably confusing rustc. I don't fully understand edition hygiene, so I'll leave it up to someone else to provide more detail. This seems like an unusual circumstance, and I really don't know how rustc should handle it.

[ehuss]

should probably be bailing if the tokens pointed to aren't a valid type.

That's certainly an option, but the code will still be broken (because the string "::Bar" is still the wrong path in 2018), so cargo fix --edition would still fail.

[sgrif]

Sure, but IMO leaving broken code is preferable to generating even more broken code

[sgrif]

I can try to produce a more minimal repro if you'd like -- I opted not to upon reporting because it seems the root cause is pretty clear just from the behavior alone. (Granted it seems like your last comment has it, thank you!)