`cargo fix` can fail if `cfg(test)` gated code causes warnings without `test`

[Volker-Weissmann]

My project runs cargo test with warnings, but successful, and if I run cargo fix, cargo test reports a build error

$ git clone https://gitlab.com/volkerweissmann/math_dsl_macro
$ cd math_dsl_macro
$ git checkout c19efa4fd2549185287c851ea9348a18f5eb461a
$ cargo test
  warnings but everything passes
$ cargo fix
$ cargo test
 compile error

Correct me if I'm wrong, but I don't think this is intended to ever happen. If it is intended, you can close this issue, if it's not intended, it's a bug

[Manishearth]

Can you paste the output you see from cargo fix?

[Volker-Weissmann]

[volker@battle math_dsl_macro]$ cargo fix
    Checking syn v1.0.73
    Checking nom v6.1.2
    Checking lalrpop-util v0.19.6
    Checking pest_consume v1.0.6
    Checking math_dsl_macro v0.1.0 (/home/volker/math_dsl_macro)
       Fixed src/codegen.rs (8 fixes)
       Fixed src/pest_parse.rs (3 fixes)
warning: unused `#[macro_use]` import
  --> src/lib.rs:11:1
   |
11 | #[macro_use] extern crate quote;
   | ^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused macro definition
 --> src/ast.rs:3:1
  |
3 | / macro_rules! hashmap {
4 | |     ($( $key: expr => $val: expr ),*) => {{
5 | |          let mut map = ::std::collections::HashMap::new();
6 | |          $( map.insert($key, $val); )*
7 | |          map
8 | |     }}
9 | | }
  | |_^
  |
  = note: `#[warn(unused_macros)]` on by default

warning: unused import: `hashmap`
  --> src/ast.rs:10:16
   |
10 | pub(crate) use hashmap;
   |                ^^^^^^^

warning: function is never used: `subseval`
  --> src/ast.rs:31:8
   |
31 | pub fn subseval(term: Term, vals: &HashMap<Symbol, f64>) -> f64 {
   |        ^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

warning: associated function is never used: `new`
   --> /home/volker/math_dsl_macro/target/debug/build/math_dsl_macro-d36924612dd3ded5/out/latex.rs:226:16
    |
226 |         pub fn new() -> TermParser {
    |                ^^^

warning: function is never used: `new_builder`
   --> /home/volker/math_dsl_macro/target/debug/build/math_dsl_macro-d36924612dd3ded5/out/latex.rs:382:12
    |
382 |     pub fn new_builder() -> __lalrpop_util::lexer::MatcherBuilder {
    |            ^^^^^^^^^^^

warning: function is never used: `from_hex`
  --> src/nom_parse.rs:15:4
   |
15 | fn from_hex(input: &str) -> Result<u8, std::num::ParseIntError> {
   |    ^^^^^^^^

warning: function is never used: `is_hex_digit`
  --> src/nom_parse.rs:19:4
   |
19 | fn is_hex_digit(c: char) -> bool {
   |    ^^^^^^^^^^^^

warning: function is never used: `hex_primary`
  --> src/nom_parse.rs:23:4
   |
23 | fn hex_primary(input: &str) -> IResult<&str, u8> {
   |    ^^^^^^^^^^^

warning: function is never used: `hex_color`
  --> src/nom_parse.rs:27:4
   |
27 | fn hex_color(input: &str) -> IResult<&str, Color> {
   |    ^^^^^^^^^

warning: unused attribute
 --> src/pest_parse.rs:2:1
  |
2 | #[macro_use]
  | ^^^^^^^^^^^^
  |
  = note: `#[warn(unused_attributes)]` on by default

warning: 11 warnings emitted

error: cannot find macro `hashmap` in this scope
   --> src/pest_parse.rs:189:18
    |
189 |                 &hashmap![Symbol::str("a") => 12.]
    |                  ^^^^^^^
    |
    = note: consider importing this macro:
            crate::ast::hashmap
    = help: have you added the `#[macro_use]` on the module/import?

error: cannot find macro `hashmap` in this scope
   --> src/pest_parse.rs:182:18
    |
182 |                 &hashmap![Symbol::str("a") => 12., Symbol::str("b") => 23., Symbol::str("c") => 34.]
    |                  ^^^^^^^
    |
    = note: consider importing this macro:
            crate::ast::hashmap
    = help: have you added the `#[macro_use]` on the module/import?

error: cannot find macro `hashmap` in this scope
   --> src/pest_parse.rs:175:18
    |
175 |                 &hashmap![Symbol::str("a") => 12., Symbol::str("b") => 23.]
    |                  ^^^^^^^
    |
    = note: consider importing this macro:
            crate::ast::hashmap
    = help: have you added the `#[macro_use]` on the module/import?

error: cannot find macro `hashmap` in this scope
  --> src/codegen.rs:81:16
   |
81 |     let vars = hashmap![
   |                ^^^^^^^
   |
   = note: consider importing this macro:
           crate::ast::hashmap
   = help: have you added the `#[macro_use]` on the module/import?

error[E0433]: failed to resolve: use of undeclared crate or module `pest_parse`
  --> src/codegen.rs:80:16
   |
80 |     let term = pest_parse::parse_term(r"(2\pi \sin\psi)\sin(\psi+\theta)/R^2 - \frac\pi 3 (1-\cos\psi)^2(2+\cos\psi) + \pi \sin^2\psi y/R...
   |                ^^^^^^^^^^ use of undeclared crate or module `pest_parse`

error[E0425]: cannot find function `subseval` in this scope
   --> src/pest_parse.rs:173:13
    |
173 |             subseval(
    |             ^^^^^^^^ not found in this scope
    |
help: consider importing this function
    |
168 |     use crate::ast::subseval;
    |

error[E0425]: cannot find function `subseval` in this scope
   --> src/pest_parse.rs:180:13
    |
180 |             subseval(
    |             ^^^^^^^^ not found in this scope
    |
help: consider importing this function
    |
168 |     use crate::ast::subseval;
    |

error[E0425]: cannot find function `subseval` in this scope
   --> src/pest_parse.rs:187:13
    |
187 |             subseval(
    |             ^^^^^^^^ not found in this scope
    |
help: consider importing this function
    |
168 |     use crate::ast::subseval;
    |

warning: unused `#[macro_use]` import
  --> src/lib.rs:11:1
   |
11 | #[macro_use] extern crate quote;
   | ^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused macro definition
 --> src/ast.rs:3:1
  |
3 | / macro_rules! hashmap {
4 | |     ($( $key: expr => $val: expr ),*) => {{
5 | |          let mut map = ::std::collections::HashMap::new();
6 | |          $( map.insert($key, $val); )*
7 | |          map
8 | |     }}
9 | | }
  | |_^
  |
  = note: `#[warn(unused_macros)]` on by default

warning: unused import: `hashmap`
  --> src/ast.rs:10:16
   |
10 | pub(crate) use hashmap;
   |                ^^^^^^^

error[E0599]: no method named `to_token_stream` found for enum `syn::Expr` in the current scope
  --> src/codegen.rs:87:27
   |
87 |     dbg!(ast_to_syn(term).to_token_stream());
   |                           ^^^^^^^^^^^^^^^ method not found in `syn::Expr`
   | 
  ::: /home/volker/.cargo/registry/src/github.com-1ecc6299db9ec823/quote-1.0.9/src/to_tokens.rs:61:8
   |
61 |     fn to_token_stream(&self) -> TokenStream {
   |        --------------- the method is available for `syn::Expr` here
   |
   = help: items from traits can only be used if the trait is in scope
   = note: the following trait is implemented but not in scope; perhaps add a `use` for it:
           `use crate::quote::ToTokens;`

error: aborting due to 9 previous errors; 3 warnings emitted

Some errors have detailed explanations: E0425, E0433, E0599.
For more information about an error, try `rustc --explain E0425`.
error: could not compile `math_dsl_macro`

To learn more, run the command again with --verbose.
[volker@battle math_dsl_macro]$ 

[ehuss]

I see what is happening.

cargo fix attempts to first fix with cfg(test) as false. Since there are several imports that are only being used by some tests, they get removed (because they are unused). Then, when it tries to fix with cfg(test) as true, those tests fail to compile because the imports have been removed.

One workaround is to use cargo fix --tests, which ensures it only fixes with cfg(test) enabled.

This is a general hazard of conditional compilation. The compiler can only compile for one specific configuration at a time. If there is a mix of incompatible code between configurations, fixing for one may break another.

I'm transferring to the cargo repo, since I think this could maybe be resolved by prioritizing tests first. That might have the opposite problem (like a cfg(not(test)) expression breaking in the other direction), but I suspect that is less likely. (This repo just contains the library which applies text transformations, it doesn't really do much.)

[Volker-Weissmann]

Seems right.
Maybe --tests should be enabled by default.

[ehuss]

Here is a test in Cargo's testsuite form:

#[cargo_test]
fn fix_breaks_test() {
    // Checks what happens when a cfg-test-only fix interacts with non-test code.
    let p = project()
        .file("Cargo.toml", &basic_manifest("foo", "0.1.0"))
        .file(
            "src/lib.rs",
            r#"
                use std::path::Path;

                #[test]
                fn mytest() {
                    let _x = Path::new("foo");
                }
            "#,
        )
        .build();

    p.cargo("fix --allow-no-vcs").run();
}

This will randomly fail depending on the order the targets get processed.

[kraktus]

Same issue, the best fix would be for cargo fix to move imports in the cfg(test) gated code, not sure how feasible it is.