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

format! .into() suggestion deletes the format macro #110017

Closed
ehuss opened this issue Apr 6, 2023 · 8 comments · Fixed by #112043
Closed

format! .into() suggestion deletes the format macro #110017

ehuss opened this issue Apr 6, 2023 · 8 comments · Fixed by #112043
Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ehuss
Copy link
Contributor

ehuss commented Apr 6, 2023

Code

pub fn foo(x: &str) -> Result<(), Box<dyn std::error::Error>> {
    Err(format!("error: {x}"))
}

Current output

Checking foo v0.1.0 (/Users/eric/Temp/foo)
error[E0308]: mismatched types
 --> src/lib.rs:2:9
  |
2 |     Err(format!("error: {x}"))
  |         ^^^^^^^^^^^^^^^^^^^^^ expected `Box<dyn Error>`, found `String`
  |
  = note: expected struct `Box<dyn std::error::Error>`
             found struct `String`
  = note: this error originates in the macro `format` (in Nightly builds, run with -Z macro-backtrace for more info)
help: call `Into::into` on this expression to convert `String` into `Box<dyn std::error::Error>`
 --> /Users/eric/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/macros.rs:121:12
  |
12|         res.into()
  |            +++++++

For more information about this error, try `rustc --explain E0308`.
error: could not compile `foo` (lib) due to previous error

Desired output

Checking foo v0.1.0 (/Users/eric/Temp/foo)
error[E0308]: mismatched types
 --> src/lib.rs:2:9
  |
2 |     Err(format!("error: {x}"))
  |         ^^^^^^^^^^^^^^^^^^^^^ expected `Box<dyn Error>`, found `String`
  |
  = note: expected struct `Box<dyn std::error::Error>`
             found struct `String`
  = note: this error originates in the macro `format` (in Nightly builds, run with -Z macro-backtrace for more info)
help: call `Into::into` on this expression to convert `String` into `Box<dyn std::error::Error>`
 --> /Users/eric/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/macros.rs:121:12
  |
12|          Err(format!("error: {x}").into())
  |                                   +++++++

For more information about this error, try `rustc --explain E0308`.
error: could not compile `foo` (lib) due to previous error

Rationale and extra context

The suggestion converts the code to:

Err(.into())

which is invalid.

It should suggest:

Err(format!("error: {x}").into())

The suggestion works correctly for non-macro string values.

This suggestion was introduced in #102496.

Other cases

No response

Anything else?

rustc 1.70.0-nightly (cf7ada217 2023-04-03)
binary: rustc
commit-hash: cf7ada217c8ac63367b184afd9fffaff30f6ed44
commit-date: 2023-04-03
host: aarch64-apple-darwin
release: 1.70.0-nightly
LLVM version: 16.0.0
@ehuss ehuss added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. labels Apr 6, 2023
@mu001999
Copy link
Contributor

mu001999 commented Apr 7, 2023

It cannot be reproduced on play.

@Ezrashaw
Copy link
Contributor

Ezrashaw commented Apr 7, 2023

I'm able to reproduce it locally. It seems to be suggesting a change in libstd:

help: call `Into::into` on this expression to convert `String` into `Box<dyn std::error::Error>`
 --> /Users/eric/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/macros.rs:121:12

(note the path)

I assume that this is due to macro expansion. It might not be working on the playground because the playground doesn't have the rust-src component installed but that's a total guess.

@ehuss
Copy link
Contributor Author

ehuss commented Apr 7, 2023

The presence of rust-src only affects the human-readable rendering. The important part to me is the JSON output which has "suggested_replacement": ".into()" even without rust-src.

Ideally it would be something closer to what it looks like with Err(String::new()):

    {
      "children": [],
      "code": null,
      "level": "help",
      "message": "call `Into::into` on this expression to convert `String` into `Box<dyn std::error::Error>`",
      "rendered": null,
      "spans": [
        {
          "byte_end": 85,
          "byte_start": 85,
          "column_end": 22,
          "column_start": 22,
          "expansion": null,
          "file_name": "src/lib.rs",
          "is_primary": true,
          "label": null,
          "line_end": 2,
          "line_start": 2,
          "suggested_replacement": ".into()",
          "suggestion_applicability": "MaybeIncorrect",
          "text": [
            {
              "highlight_end": 22,
              "highlight_start": 22,
              "text": "    Err(String::new())"
            }
          ]
        }
      ]
    }

@mu001999
Copy link
Contributor

mu001999 commented Apr 7, 2023

It seems to affect all macro expansions (play):

macro_rules! s {
    ($e: expr) => {
        String::from($e)
    }
}

pub fn foo(x: &str) -> Result<(), Box<dyn std::error::Error>> {
    Err(s!(x))
}

@Ezrashaw
Copy link
Contributor

Ezrashaw commented Apr 7, 2023

Is the solution to just disable this suggestion for macro-expanded code?

As an aside, perhaps the playground should get rust-src, especially because of its prevalence in sharing code which might need the additional context.

@fmease
Copy link
Member

fmease commented Apr 7, 2023

Is the solution to just disable this suggestion for macro-expanded code?

I think it's possible to achieve the desired output found in the description, although I'm not sure how exactly.
Typically, you would use find_ancestor_inside on the span to peel away the layers of macro invocations, though for
that you would need to provide a parent to mark the end of the chain. Span::source_callsite doesn't need such a stopper but here its use would be incorrect if the macro call itself (e.g. Err(s!(x))) was inside of a macro expansion.

Since we don't have a parent (afaiaa), earlier I briefly tried to create a parent for find_ancestor_inside via tcx.hir().span_with_body(tcx.hir().parent_id(expr.hir_id)) I think but that didn't work. Maybe tcx.hir().get_enclosing_scope(/*???*/) could work, not sure. This approach is a catch-22 I believe.

@Ezrashaw
Copy link
Contributor

Ezrashaw commented Apr 7, 2023

Yeah, would that be the desired solution though? Can this be generalized to all the other type mismatch suggestions?

@fmease
Copy link
Member

fmease commented Apr 7, 2023

That's a good question. It was mostly an experiment to see if this specific issue can be solved with find_ancestor_inside.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants