Avoid path to macro-generated extern crate in error messages

Opened by David Tolnay at 2019-04-26 22:53:24

#[macro_use]
extern crate serde_derive;
extern crate serde_json;

// Expands to:
//
// const _IMPL_DESERIALIZE_FOR_S: () = {
//     extern crate serde as _serde;
//     impl<'de> _serde::Deserialize<'de> for S { /* ... */ }
// }
#[derive(Deserialize)]
struct S;

fn main() {
    // Requires S: serde::Serialize.
    serde_json::to_string(&S);
}

The message as of rustc 1.24.0-nightly (c284f8807 2017-12-24):

error[E0277]: the trait bound `S: _IMPL_DESERIALIZE_FOR_S::_serde::Serialize` is not satisfied
  --> src/main.rs:16:5
   |
16 |     serde_json::to_string(&S);
   |     ^^^^^^^^^^^^^^^^^^^^^ the trait `_IMPL_DESERIALIZE_FOR_S::_serde::Serialize` is not implemented for `S`
   |
   = note: required by `serde_json::to_string`

In this case it would be more desirable for the error message to refer to serde::Serialize rather than _IMPL_DESERIALIZE_FOR_S::_serde::Serialize. The extern crate means that the user's Cargo.toml includes the serde crate under [dependencies], so showing serde::Serialize as the path seems reasonable.

I understand that serde::Serialize could be ambiguous if the user's crate includes mod serde at the top level. For now we could ignore that case and show serde::Serialize anyway, or treat it as a special case and not show serde::Serialize if they have a mod serde. The special case would go away with https://github.com/rust-lang/rfcs/pull/2126 by showing crate::serde::Serialize.

Fixing this would be valuable in allowing us to reinstate the lint against unused extern crate by addressing the usability issue reported in #44294.

Mentioning @pnkfelix who worked on #46112.

```
use serde::Deserialize;

// Expands to:
//
// const _IMPL_DESERIALIZE_FOR_S: () = {
//     extern crate serde as _serde;
//     impl<'de> _serde::Deserialize<'de> for S { /* ... */ }
// }
#[derive(Deserialize)]
struct S;

fn main() {
    // Requires S: serde::Serialize.
    serde_json::to_string(&S);
}
```
The message as of rustc 1.24.0-nightly (c284f8807 2017-12-24):
```
error[E0277]: the trait bound `S: _IMPL_DESERIALIZE_FOR_S::_serde::Serialize` is not satisfied
  --> src/main.rs:16:5
   |
16 |     serde_json::to_string(&S);
   |     ^^^^^^^^^^^^^^^^^^^^^ the trait `_IMPL_DESERIALIZE_FOR_S::_serde::Serialize` is not implemented for `S`
   |
   = note: required by `serde_json::to_string`
```
In this case it would be more desirable for the error message to refer to serde::Serialize rather than _IMPL_DESERIALIZE_FOR_S::_serde::Serialize. The extern crate means that the user's Cargo.toml includes the serde crate under [dependencies], so showing serde::Serialize as the path seems reasonable.

I understand that serde::Serialize could be ambiguous if the user's crate includes mod serde at the top level. For now we could ignore that case and show serde::Serialize anyway, or treat it as a special case and not show serde::Serialize if they have a mod serde. The special case would go away with https://github.com/rust-lang/rfcs/pull/2126 by showing crate::serde::Serialize.

Fixing this would be valuable in allowing us to reinstate the lint against unused extern crate by addressing the usability issue reported in #44294.

Mentioning @pnkfelix who worked on #46112.
David Tolnay at 2020-06-19 20:33:37
~~That would require us to have a "smart" prioritization system for paths.~~

~~We want to prefer:~~
```
std::option::Option
```
~~over both~~
```
core::option::Option
```
~~and~~
```
#[no_std]
crate hijack {
    pub use core::option::Option;
}
```
~~But also to prefer an "indirect" serde::Serialize over an import through a macro~~
Ariel Ben-Yehuda at 2017-12-25 12:38:38
Disregard the above. rustc can certainly emit "phantom" crate paths if there is a "voldemort re-export" of an item from another crate, aka

crate foo:
```
#[derive(Default)]
pub struct Type;
```
crate bar:
```
extern crate foo;
pub fn my_fn() -> foo::Type { foo::Type::default(); }
```
crate main:
```
extern crate bar;
let () = bar::my_fn(); //~ ERROR mismatched types
                       //~| expected `foo::Type`
                       // ^ this is the case, even if there is no `foo` in the crate `bar`
```
The logic for choosing the path to display a crate from is this: https://github.com/rust-lang/rust/blob/0cd67581e7d048810874903dc589e5b855f47e7d/src/librustc/ty/item_path.rs#L86-L124

That logic could be smarter:
1. It could prefer "phantom" paths to "weird" local paths, for some definition of "weird".
2. If a "phantom" paths collides with a local path, it could plumb the information back somehow to the error message. One common case where phantom paths occur is when crates from different versions are used - e.g. if our main crate had used a different version of foo than bar used, then bar::foo would have been a "phantom path".
Ariel Ben-Yehuda at 2017-12-25 12:52:04
cc @estebank @tirr-c - you might want to think of a good way to solve this
Ariel Ben-Yehuda at 2017-12-25 12:52:44

One strategy I can think of for implementing this (I'm not 100% sure it's a good idea in practice, I haven't tried this):

Add the following structs to ppaux:

pub struct RefPrintContext {
    inner: RefCell<PrintContext>
}

impl RefPrintContext {
    pub fn new(p: PrintContext) -> Self { /* .. */ }
    pub fn into_inner(self) -> PrintContext { self.inner.into_inner() }
    pub fn with<T: Print>(&self, data: T) -> RefPrintContextWith<'a, T> {
        RefPrintContextWith { cx: self, data }
    }
}

pub struct RefPrintContextWith<'a, T: fmt::Print> {
    cx: &'a RefPrintContext,
    data: T
}

impl<'a, T: Print> Display for RefPrintContextWith<'a, T> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        Print::print_display(&self.data, f, &self.cx.borrow_mut())
    }
}

Change the span_err! and span_warn! macros in rustc to create a new PrintContext, turn it to a RefPrintContext, and wrap rpcx.with(arg) around the format args. So span_err!(sess, span, E0222, "{}", x) becomes
```
    let rpcx = RefPrintContext::new(PrintContext::new());
    span_err!(sess, span, E0222, "{}", rpcx.with(&x));
```
Yes, this will mean that "smart" formatting taking extra integer arguments will not be possible. I don't believe anyone is using it in span_bug! etc.

This will require adding Print impls to "small" types (e.g. integers).

Remove the Display impls for Print types and make everyone use cx.wrap.

Then you could pass the PrintContext through item_path and move the thread-locals in item_path to the PrintContext.

Afterward, you could allow a PrintContext to keep a reference to a Diagnostic, and have it register notes (make sure you replace all the old uses of PrintContext::new), so that we could have this error:

If serde is accessible in this crate through a cargo dependency and there is no name collision - I think we should use the [serde] syntax even while it is not legal, as it is better than just serde

error[E0277]: the trait bound `S : [serde]::Serialize` is not satisfied
  --> src/main.rs:16:5
   |
16 |     serde_json::to_string(&S);
   |     ^^^^^^^^^^^^^^^^^^^^^ the trait `[serde]::Serialize` is not implemented for `S`
   |
   = note: required by `serde_json::to_string`

If it is not, we could have this error:

error[E0277]: the trait bound `S : [serde#1]::Serialize` is not satisfied
  --> src/main.rs:16:5
   |
16 |
   |     ^^^^^^^^^^^^^^^^^^^^^ the trait `[serde#1]::Serialize` is not implemented for `S`
   |
   = note: required by `serde_json::to_string`
   = note: `[serde#1]` is used by the crate `hyper`, and is different from `[serde]` in this crate

Ariel Ben-Yehuda at 2017-12-25 13:35:11

Still seeing the same unhelpful path in 2018 edition as of <kbd>rustc 1.35.0-nightly (3eb4890df 2019-03-19)</kbd>.

use serde::Deserialize;

#[derive(Deserialize)]
struct S;

fn main() {
    serde_json::to_string(&S);
}

error[E0277]: the trait bound `S: _IMPL_DESERIALIZE_FOR_S::_serde::Serialize` is not satisfied
 --> src/main.rs:7:5
  |
7 |     serde_json::to_string(&S);
  |     ^^^^^^^^^^^^^^^^^^^^^ the trait `_IMPL_DESERIALIZE_FOR_S::_serde::Serialize` is not implemented for `S`
  |
  = note: required by `serde_json::ser::to_string`

David Tolnay at 2019-03-20 22:58:54

@dtolnay you could use rustc_on_unimplemented to annotate serde::Deserialize and serde::Serialize when building in nightly where you can rewrite the E0277 errors to your own needs.

Fixing this in a general way will be hard to accomplish in the short term.
Esteban Kuber at 2019-04-26 22:53:08