Avoid path to macro-generated extern crate in error messages

[dtolnay]

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 (c284f88 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 rust-lang/rfcs#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.

[arielb1]

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

[arielb1]

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:

rust/src/librustc/ty/item_path.rs

Lines 86 to 124 in 0cd6758

	pub fn push_krate_path<T>(self, buffer: &mut T, cnum: CrateNum)
	where T: ItemPathBuffer
	{
	match *buffer.root_mode() {
	RootMode::Local => {
	// In local mode, when we encounter a crate other than
	// LOCAL_CRATE, execution proceeds in one of two ways:
	//
	// 1. for a direct dependency, where user added an
	// `extern crate` manually, we put the `extern
	// crate` as the parent. So you wind up with
	// something relative to the current crate.
	// 2. for an indirect crate, where there is no extern
	// crate, we just prepend the crate name.
	//
	// Returns `None` for the local crate.
	if cnum != LOCAL_CRATE {
	let opt_extern_crate = self.extern_crate(cnum.as_def_id());
	let opt_extern_crate = opt_extern_crate.and_then(|extern_crate| {
	if extern_crate.direct {
	Some(extern_crate.def_id)
	} else {
	None
	}
	});
	if let Some(extern_crate_def_id) = opt_extern_crate {
	self.push_item_path(buffer, extern_crate_def_id);
	} else {
	buffer.push(&self.crate_name(cnum).as_str());
	}
	}
	}
	RootMode::Absolute => {
	// In absolute mode, just write the crate name
	// unconditionally.
	buffer.push(&self.original_crate_name(cnum).as_str());
	}
	}
	}

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".

[arielb1]

cc @estebank @tirr-c - you might want to think of a good way to solve this

[arielb1]

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):

1. 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())
       }
   }

2. 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).

3. 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

[dtolnay]

Still seeing the same unhelpful path in 2018 edition as of rustc 1.35.0-nightly (3eb4890 2019-03-19).

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`

[estebank]

@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.