Missing fixes for 2018

[SimonSapin]

https://blog.rust-lang.org/2018/10/30/help-test-rust-2018.html

The first step is to run cargo fix:

$ cargo fix --edition

This will check your code, and automatically fix any issues that it can. cargo fix is still pretty new, and so it can’t always fix your code automatically. If cargo fix can’t fix something, it will print the warning that it cannot fix to the console. If you see one of these warnings, you’ll have to update your code manually.

Indeed, running (roughly) this command with nightly-2018-11-01 prints some warnings. So many warnings that just transferring them through SSH saturates the 100 Mbps link from my build server.

$ touch **/lib.rs
$ cargo fix --edition --allow-dirty --manifest-path ports/servo/Cargo.toml > /tmp/fix.log 2>&1
$ ls -sh /tmp/fix.log 
96M /tmp/fix.log
$ wc --lines /tmp/fix.log
1450907 /tmp/fix.log
$ grep '^warning:' /tmp/fix.log -c          
160015
$  grep '^warning:' /tmp/fix.log|sort|uniq -c 
 159696 warning: absolute paths must start with `self`, `super`, `crate`, or an external crate name in the 2018 edition
      9 warning: anonymous parameters are deprecated and will be removed in the next edition.
      8 warning: `async` is a keyword in the 2018 edition
    302 warning: `try` is a keyword in the 2018 edition
$ grep 'help: use `crate`:' /tmp/fix.log -c   
157968

To be fair these numbers are inflated because a lot of the relevant code is generated. But still, I hope this shows that the absolute-path warnings are extremely common. I feel it is essential that rustfix can fix them automatically for the edition migration path to be viable.

These absolute-path warnings seem to be mostly in one of three cases:

1. An use statement for an item in the same crate. The warning message suggests adding crate:: and even shows what the new import should look like. So it seems like we’re most of the way there to apply this fix automatically.

2. An use statement for an item in an external crate, but that crate has been renamed with for example extern crate mozjs as js;. In this case a path starting with js:: could be considered absolute and referring to an external crate. (Though this is a language change rather than a rustfix change.)

3. An #[derive(…)] attribute with a derive defined in a proc-macro crate. (“Built-in” derives like #[derive(Clone)] are not affected.) The correct fix is probably to add use crate_name::DeriveName; near the start of the module? Again, automatically.

[SimonSapin]

It turns out that the first case is actually implemented, but I’m also hitting https://github.com/rust-lang-nursery/rustfix/issues/150.

[alexcrichton]

Can you check the top of the output? I think basically everything should be automatically fixed, but if cargo fix hits a bug in one way or another it won't fix anything because some fix along the way causes the crate to stop compiling

(or is this rust-lang/cargo#13022?, I haven't read that yet)

[alexcrichton]

@SimonSapin can you try this out again once rust-lang/cargo#13022 is fixed and see how many issues still remain?

[SimonSapin]

So, on further discoveries, for the three respective cases in the issue description:

1. This actually fixed automatically, but only in crates that cargo fix decides to fix. I assumed that making this fix was not supported but that was only a symptom of https://github.com/rust-lang-nursery/rustfix/issues/150

2. Cargo can achieve an equivalent result (on Nightly) with https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#rename-dependency

3. An example diagnostic for this was:

   warning: absolute paths must start with `self`, `super`, `crate`, or an external crate name in the 2018 edition        
      --> components/style/values/generics/image.rs:170:69                                                                
       |                                                                                                                  
   170 | #[derive(Clone, Debug, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToComputedValue, ToCss)]                     
       |                                                                     ^^^^^^^^^^^^^^^                              
       |                                                                                                                  
       = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in the 2018 edition!
       = note: for more information, see issue #53130 <https://github.com/rust-lang/rust/issues/53130>                    

   In my mind there was no question at all that the "absolute path" in question was ToComputedValue, the name of the proc macro derive. I assumed that #[macro_use] had lost its effect in Rust 2018, and that importing derives with use was now mandatory.

   Eddyb pointed out on IRC that this warning comes in fact from the expansion of the proc macro. There a two problems here:

   • There is no indication that this is the case
   • Even once that is known, there is no hint as to what part of the expansion is problematic, not even which path needs to be made absolute.

[petrochenkov]

ToComputedValue

Macros defined by 2015 edition crates are supposed to "just work" on 2018 edition with help edition hygiene, which is not completely implemented currently.
rust-lang/rust#50999 tried to hygienize imports, but never landed.
That's pretty unfortunate because code working cross-edition (including macros) were one of the main edition promises.

[alexcrichton]

Before we jump to conclusions about whether or not macros work in cross-editions situations, can the supposed bug be reduced to a digestable input? I've found that 90% of the time the bug isn't what you expect, but it's something totally random in the compiler.

[ehuss]

I'm going to close, as I don't think this has a particularly actionable reproduction, and is likely better tracked in rust-lang/rust#55592 or some other related issue.