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

Diagnostic suggestions sometimes incorrectly point to external crates #88514

Open
ehuss opened this issue Aug 30, 2021 · 2 comments
Open

Diagnostic suggestions sometimes incorrectly point to external crates #88514

ehuss opened this issue Aug 30, 2021 · 2 comments
Labels
A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code.

Comments

@ehuss
Copy link
Contributor

ehuss commented Aug 30, 2021

In some situations, rustc may provide a diagnostic suggestion in an external crate, which in general it shouldn't do. These external crates may be in cargo's registry cache, which the user should not be modifying. For example, cargo fix may stomp on changes outside of the package (which is a separate issue rust-lang/cargo#9857). Usually rustc is good about avoiding this, so I'm not sure what is wrong in these situations.

In general, I expect rustc to not provide suggestions to modify dependencies.

The crater run found several instances of this, each for different underlying reasons (all dealing with macros).

Example 1 — unused tt

Example dependency:

// bar.rs
#[macro_export]
macro_rules! m {
    ($i:tt) => {
        $crate::m!(foo $i);
    };
    (foo $i:tt) => {
        let $i = 1;
    }
}

Example local crate:

// foo.rs
pub fn foo() {
    bar::m!(abc);
}

Resulting suggestion:

warning: unused variable: `abc`
 --> /Users/eric/Temp/crater-2021/foo/bar/src/lib.rs:7:13
  |
7 |         let $i = 1;
  |             ^^ help: if this is intentional, prefix it with an underscore: `_abc`
  |
  = note: `#[warn(unused_variables)]` on by default

A key point of this example is that the macro uses tt instead of ident. ident will not issue an unused warning.

Found in the 2021 crater run for:

Example 2 — Weird $body suggestion

The following makes a suggestion to remove unnecessary braces, but the suggestion doesn't actually remove any braces.

// Using cpython 0.2.1
use cpython::*;

fn hello(py: Python) -> PyResult<PyString> {
    Ok(PyString::new(py, "Rust says: Hello world"))
}

py_module_initializer!(example, initexample, PyInit_example, |py, m| {
    m.add(py, "hello", py_fn!(py, hello()))?;
    Ok(())
});

Suggestion:

warning: unnecessary braces around block return value
   --> /Users/eric/.cargo/registry/src/github.com-1ecc6299db9ec823/cpython-0.2.1/src/argparse.rs:386:52
    |
386 |     ( $py:expr, $iter:expr, $body:block, [] ) => { $body };
    |                                                    ^^^^^ help: remove these braces
    |
    = note: `#[warn(unused_braces)]` on by default

warning: `foo` (lib) generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s

Found in the 2021 crater run for:

Example 3 — Hidden JSON suggestion

In this example, the human-readable text doesn't mention the dependency, but the JSON includes a MachineApplicable suggestion.

// diesel 1.4.7
#[macro_use]
extern crate diesel;
use diesel::table;

table! {
    use diesel::sql_types::*;
    use std::fs;

    users {
        id -> Integer,
        name -> VarChar,
        favorite_color -> Nullable<VarChar>,
    }
}

This emits two duplicate diagnostics in human form:

warning: unused import: `std::fs`
 --> src/lib.rs:7:9
  |
7 |     use std::fs;
  |         ^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: unused import: `std::fs`
 --> src/lib.rs:7:9
  |
7 |     use std::fs;
  |         ^^^^^^^

warning: `foo` (lib) generated 2 warnings

Buried in the JSON output is a machine-applicable change which modifies diesel:

{
    "byte_end": 9126,
    "byte_start": 9108,
    "column_end": 55,
    "column_start": 37,
    "expansion":
    {},
    "file_name": "/Users/eric/.cargo/registry/src/github.com-1ecc6299db9ec823/diesel-1.4.7/src/macros/mod.rs",
    "is_primary": true,
    "label": null,
    "line_end": 299,
    "line_start": 299,
    "suggested_replacement": "",
    "suggestion_applicability": "MachineApplicable",
    "text":
    [
        {
            "highlight_end": 55,
            "highlight_start": 37,
            "text": "            imports = [$($imports)* use $($import)::+;],"
        }
    ]
}

Found in the 2021 crater run for:

Meta

rustc --version --verbose:

rustc 1.56.0-nightly (5eacec9ec 2021-08-28)
binary: rustc
commit-hash: 5eacec9ec7e112a0de1011519a57c45586d58414
commit-date: 2021-08-28
host: x86_64-apple-darwin
release: 1.56.0-nightly
LLVM version: 13.0.0
@ehuss ehuss added C-bug Category: This is a bug. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. labels Aug 30, 2021
@weihanglo
Copy link
Member

weihanglo commented Sep 25, 2021

The regression of "Example 1" was introduced between 1.51.0 and 1.52.0. The bisect report gave me three rollup PRs. The bug can be reproduced after this rollup and the suspect may be #82655.

I am not familiar with this part, so if anyone want to address it, just take it. (Or even better mentor me 😆, thanks)

Report from cargo-bisect-rustc

Script for cargo-bisect-rustc

#!/usr/bin/env bash
# Run Example 1
cargo clean
cargo fix 2>&1 | grep "Fixed"
if [[ $? -gt 0 ]]; then
    exit 0
else
    exit 1
fi

Report from cargo-bisect-rustc

********************************************************************************
Regression in nightly-2021-03-03
********************************************************************************

fetching https://static.rust-lang.org/dist/2021-03-02/channel-rust-nightly-git-commit-hash.txt
nightly manifest 2021-03-02: 40 B / 40 B [=======================================================================================] 100.00 % 692.44 KB/s converted 2021-03-02 to 4f20caa6258d4c74ce6b316fd347e3efe81cf557
fetching https://static.rust-lang.org/dist/2021-03-03/channel-rust-nightly-git-commit-hash.txt
nightly manifest 2021-03-03: 40 B / 40 B [=======================================================================================] 100.00 % 893.43 KB/s converted 2021-03-03 to 35dbef235048f9a2939dc20effe083ca483c37ff
looking for regression commit between 2021-03-02 and 2021-03-03
opening existing repository at "/Users/weihanglo/wd/contrib/rust"
refreshing repository
fetching (via local git) commits from 4f20caa6258d4c74ce6b316fd347e3efe81cf557 to 35dbef235048f9a2939dc20effe083ca483c37ff
opening existing repository at "/Users/weihanglo/wd/contrib/rust"
refreshing repository
looking up first commit
looking up second commit
checking that commits are by bors and thus have ci artifacts...
finding bors merge commits
found 6 bors merge commits in the specified range
  commit[0] 2021-03-01UTC: Auto merge of #82663 - jyn514:rollup-xh3cb0c, r=jyn514
  commit[1] 2021-03-02UTC: Auto merge of #82688 - GuillaumeGomez:rollup-b754t11, r=GuillaumeGomez
  commit[2] 2021-03-02UTC: Auto merge of #82634 - osa1:osa1/remove_old_fixme, r=Mark-Simulacrum
  commit[3] 2021-03-02UTC: Auto merge of #82698 - JohnTitor:rollup-htd533c, r=JohnTitor
  commit[4] 2021-03-02UTC: Auto merge of #82043 - tmiasko:may-have-side-effect, r=kennytm
  commit[5] 2021-03-02UTC: Auto merge of #82562 - llogiq:one-up-82248, r=oli-obk
ERROR: no commits between 4f20caa6258d4c74ce6b316fd347e3efe81cf557 and 35dbef235048f9a2939dc20effe083ca483c37ff within last 167 days
-----------------

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 16, 2023
Do not provide suggestions when the spans come from expanded code that doesn't point at user code

Hide invalid proc-macro suggestions and track spans
coming from proc-macros pointing at attribute.

Effectively, unless the proc-macro keeps user spans,
suggestions will not be produced for the code they
produce.

r? `@ghost`

Fix rust-lang#107113, fix rust-lang#107976, fix rust-lang#107977, fix rust-lang#108748, fix rust-lang#106720, fix rust-lang#90557.

Could potentially address rust-lang#50141, rust-lang#67373, rust-lang#55146, rust-lang#78862, rust-lang#74043, rust-lang#88514, rust-lang#83320, rust-lang#91520, rust-lang#104071. CC rust-lang#50122, rust-lang#76360.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 17, 2023
Do not provide suggestions when the spans come from expanded code that doesn't point at user code

Hide invalid proc-macro suggestions and track spans
coming from proc-macros pointing at attribute.

Effectively, unless the proc-macro keeps user spans,
suggestions will not be produced for the code they
produce.

r? ``@ghost``

Fix rust-lang#107113, fix rust-lang#107976, fix rust-lang#107977, fix rust-lang#108748, fix rust-lang#106720, fix rust-lang#90557.

Could potentially address rust-lang#50141, rust-lang#67373, rust-lang#55146, rust-lang#78862, rust-lang#74043, rust-lang#88514, rust-lang#83320, rust-lang#91520, rust-lang#104071. CC rust-lang#50122, rust-lang#76360.
@ehuss
Copy link
Contributor Author

ehuss commented Apr 8, 2024

This example attempts to make modifications to the standard library (if you have the source component installed).

pub fn bug_report<W: std::fmt::Write>(w: &mut W) -> std::fmt::Result {
    if true {
        writeln!(w, "`;?` here ->")?;
    } else {
        writeln!(w, "but not here")
    }
    Ok(())
}

JSON:

SEE JSON
{
    "$message_type": "diagnostic",
    "message": "mismatched types",
    "code":
    {
        "code": "E0308",
        "explanation": "Expected type did not match the received type.\n\nErroneous code examples:\n\n```compile_fail,E0308\nfn plus_one(x: i32) -> i32 {\n    x + 1\n}\n\nplus_one(\"Not a number\");\n//       ^^^^^^^^^^^^^^ expected `i32`, found `&str`\n\nif \"Not a bool\" {\n// ^^^^^^^^^^^^ expected `bool`, found `&str`\n}\n\nlet x: f32 = \"Not a float\";\n//     ---   ^^^^^^^^^^^^^ expected `f32`, found `&str`\n//     |\n//     expected due to this\n```\n\nThis error occurs when an expression was used in a place where the compiler\nexpected an expression of a different type. It can occur in several cases, the\nmost common being when calling a function and passing an argument which has a\ndifferent type than the matching type in the function declaration.\n"
    },
    "level": "error",
    "spans":
    [
        {
            "file_name": "/Users/eric/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/macros/mod.rs",
            "byte_start": 23568,
            "byte_end": 23617,
            "line_start": 670,
            "line_end": 670,
            "column_start": 9,
            "column_end": 58,
            "is_primary": true,
            "text":
            [
                {
                    "text": "        $dst.write_fmt($crate::format_args_nl!($($arg)*))",
                    "highlight_start": 9,
                    "highlight_end": 58
                }
            ],
            "label": "expected `()`, found `Result<(), Error>`",
            "suggested_replacement": null,
            "suggestion_applicability": null,
            "expansion":
            {
                "span":
                {
                    "file_name": "lib.rs",
                    "byte_start": 144,
                    "byte_end": 171,
                    "line_start": 5,
                    "line_end": 5,
                    "column_start": 9,
                    "column_end": 36,
                    "is_primary": false,
                    "text":
                    [
                        {
                            "text": "        writeln!(w, \"but not here\")",
                            "highlight_start": 9,
                            "highlight_end": 36
                        }
                    ],
                    "label": null,
                    "suggested_replacement": null,
                    "suggestion_applicability": null,
                    "expansion": null
                },
                "macro_decl_name": "writeln!",
                "def_site_span":
                {
                    "file_name": "/Users/eric/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/macros/mod.rs",
                    "byte_start": 23434,
                    "byte_end": 23454,
                    "line_start": 665,
                    "line_end": 665,
                    "column_start": 1,
                    "column_end": 21,
                    "is_primary": false,
                    "text":
                    [
                        {
                            "text": "macro_rules! writeln {",
                            "highlight_start": 1,
                            "highlight_end": 21
                        }
                    ],
                    "label": null,
                    "suggested_replacement": null,
                    "suggestion_applicability": null,
                    "expansion": null
                }
            }
        },
        {
            "file_name": "lib.rs",
            "byte_start": 75,
            "byte_end": 177,
            "line_start": 2,
            "line_end": 6,
            "column_start": 5,
            "column_end": 6,
            "is_primary": false,
            "text":
            [
                {
                    "text": "    if true {",
                    "highlight_start": 5,
                    "highlight_end": 14
                },
                {
                    "text": "        writeln!(w, \"`;?` here ->\")?;",
                    "highlight_start": 1,
                    "highlight_end": 38
                },
                {
                    "text": "    } else {",
                    "highlight_start": 1,
                    "highlight_end": 13
                },
                {
                    "text": "        writeln!(w, \"but not here\")",
                    "highlight_start": 1,
                    "highlight_end": 36
                },
                {
                    "text": "    }",
                    "highlight_start": 1,
                    "highlight_end": 6
                }
            ],
            "label": "expected this to be `()`",
            "suggested_replacement": null,
            "suggestion_applicability": null,
            "expansion": null
        }
    ],
    "children":
    [
        {
            "message": "expected unit type `()`\n        found enum `Result<(), std::fmt::Error>`",
            "code": null,
            "level": "note",
            "spans":
            [],
            "children":
            [],
            "rendered": null
        },
        {
            "message": "consider using a semicolon here",
            "code": null,
            "level": "help",
            "spans":
            [
                {
                    "file_name": "lib.rs",
                    "byte_start": 177,
                    "byte_end": 177,
                    "line_start": 6,
                    "line_end": 6,
                    "column_start": 6,
                    "column_end": 6,
                    "is_primary": true,
                    "text":
                    [
                        {
                            "text": "    }",
                            "highlight_start": 6,
                            "highlight_end": 6
                        }
                    ],
                    "label": null,
                    "suggested_replacement": ";",
                    "suggestion_applicability": "MaybeIncorrect",
                    "expansion": null
                }
            ],
            "children":
            [],
            "rendered": null
        },
        {
            "message": "you might have meant to return this value",
            "code": null,
            "level": "help",
            "spans":
            [
                {
                    "file_name": "lib.rs",
                    "byte_start": 144,
                    "byte_end": 144,
                    "line_start": 5,
                    "line_end": 5,
                    "column_start": 9,
                    "column_end": 9,
                    "is_primary": true,
                    "text":
                    [
                        {
                            "text": "        writeln!(w, \"but not here\")",
                            "highlight_start": 9,
                            "highlight_end": 9
                        }
                    ],
                    "label": null,
                    "suggested_replacement": "return ",
                    "suggestion_applicability": "MaybeIncorrect",
                    "expansion": null
                },
                {
                    "file_name": "lib.rs",
                    "byte_start": 171,
                    "byte_end": 171,
                    "line_start": 5,
                    "line_end": 5,
                    "column_start": 36,
                    "column_end": 36,
                    "is_primary": true,
                    "text":
                    [
                        {
                            "text": "        writeln!(w, \"but not here\")",
                            "highlight_start": 36,
                            "highlight_end": 36
                        }
                    ],
                    "label": null,
                    "suggested_replacement": ";",
                    "suggestion_applicability": "MaybeIncorrect",
                    "expansion": null
                }
            ],
            "children":
            [],
            "rendered": null
        },
        {
            "message": "use the `?` operator to extract the `Result<(), std::fmt::Error>` value, propagating a `Result::Err` value to the caller",
            "code": null,
            "level": "help",
            "spans":
            [
                {
                    "file_name": "/Users/eric/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/macros/mod.rs",
                    "byte_start": 23617,
                    "byte_end": 23617,
                    "line_start": 670,
                    "line_end": 670,
                    "column_start": 58,
                    "column_end": 58,
                    "is_primary": true,
                    "text":
                    [
                        {
                            "text": "        $dst.write_fmt($crate::format_args_nl!($($arg)*))",
                            "highlight_start": 58,
                            "highlight_end": 58
                        }
                    ],
                    "label": null,
                    "suggested_replacement": "?",
                    "suggestion_applicability": "HasPlaceholders",
                    "expansion":
                    {
                        "span":
                        {
                            "file_name": "lib.rs",
                            "byte_start": 144,
                            "byte_end": 171,
                            "line_start": 5,
                            "line_end": 5,
                            "column_start": 9,
                            "column_end": 36,
                            "is_primary": false,
                            "text":
                            [
                                {
                                    "text": "        writeln!(w, \"but not here\")",
                                    "highlight_start": 9,
                                    "highlight_end": 36
                                }
                            ],
                            "label": null,
                            "suggested_replacement": null,
                            "suggestion_applicability": null,
                            "expansion": null
                        },
                        "macro_decl_name": "writeln!",
                        "def_site_span":
                        {
                            "file_name": "/Users/eric/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/macros/mod.rs",
                            "byte_start": 23434,
                            "byte_end": 23454,
                            "line_start": 665,
                            "line_end": 665,
                            "column_start": 1,
                            "column_end": 21,
                            "is_primary": false,
                            "text":
                            [
                                {
                                    "text": "macro_rules! writeln {",
                                    "highlight_start": 1,
                                    "highlight_end": 21
                                }
                            ],
                            "label": null,
                            "suggested_replacement": null,
                            "suggestion_applicability": null,
                            "expansion": null
                        }
                    }
                }
            ],
            "children":
            [],
            "rendered": null
        }
    ],
    "rendered": "error[E0308]: mismatched types\n --> lib.rs:5:9\n  |\n2 | /     if true {\n3 | |         writeln!(w, \"`;?` here ->\")?;\n4 | |     } else {\n5 | |         writeln!(w, \"but not here\")\n  | |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found `Result<(), Error>`\n6 | |     }\n  | |_____- expected this to be `()`\n  |\n  = note: expected unit type `()`\n                  found enum `Result<(), std::fmt::Error>`\n  = note: this error originates in the macro `writeln` (in Nightly builds, run with -Z macro-backtrace for more info)\nhelp: consider using a semicolon here\n  |\n6 |     };\n  |      +\nhelp: you might have meant to return this value\n  |\n5 |         return writeln!(w, \"but not here\");\n  |         ++++++                            +\nhelp: use the `?` operator to extract the `Result<(), std::fmt::Error>` value, propagating a `Result::Err` value to the caller\n --> /Users/eric/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/macros/mod.rs:670:58\n  |\n67|         $dst.write_fmt($crate::format_args_nl!($($arg)*))?\n  |                                                          +\n\n"
}

Suggests to edit lib/rustlib/src/rust/library/core/src/macros/mod.rs

rustc 1.79.0-nightly (88c2f4f5f 2024-04-02)
binary: rustc
commit-hash: 88c2f4f5f50ace5ddc7655ea311435104d3659bd
commit-date: 2024-04-02
host: aarch64-apple-darwin
release: 1.79.0-nightly
LLVM version: 18.1.2

weihanglo added a commit to weihanglo/cargo that referenced this issue Apr 23, 2024
The suggestion is slightly tweaked since the original
diagnostic was rejected due to multiple files were involved.

For the original diagnostic JSON, see
rust-lang/rust#88514 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code.
Projects
None yet
Development

No branches or pull requests

2 participants