tracing icon indicating copy to clipboard operation
tracing copied to clipboard

subscriber: re-export prelude traits `as _`

Open hawkw opened this issue 3 years ago • 10 comments

Currently, the tracing-subscriber prelude re-exports some traits as __tracing_subscriber_<TRAIT NAME> to avoid namespace conflicts. This is because those traits were added to the prelude before use Trait as _ was available in stable Rust. In some cases, this can apparently result in weird error messages (see #1847).

This commit changes the prelude to re-export everything as _. In theory, this is technically a breaking change in case anyone is actually referring to traits as the re-exported names in the prelude. However, I don't think it's likely that anyone's actually doing this, and they shouldn't be considered stable API, so I don't think this needs to wait for a breaking release. If anyone disagrees, I'm open to being convinced otherwise.

Fixes #1847

hawkw avatar Jan 21 '22 17:01 hawkw

I think that Vector is the main public user of the re-exported prelude traits (see this search, cc: @tobz).

davidbarsky avatar Jan 21 '22 17:01 davidbarsky

I think that Vector is the main public user of the re-exported prelude traits (see this search, cc: @tobz).

I haven't gotten into the GitHub code search beta yet (just signed up), can you share a link that I can actually view?

hawkw avatar Jan 21 '22 17:01 hawkw

If anyone is actually using the prelude types by name, though...

  1. why are they doing that???
  2. they shouldn't be doing that
  3. this is a breaking change and we may not want to make it in a patch release

hawkw avatar Jan 21 '22 17:01 hawkw

The compiler errors look like the time dependency in tracing-appender broke MSRV.

hawkw avatar Jan 21 '22 17:01 hawkw

I haven't gotten into the GitHub code search beta yet (just signed up), can you share a link that I can actually view?

Ah, sorry about that. Here's a set of uses on the non-beta code search.

why are they doing that???

I think that rust-analyzer automatically suggests importing those re-exported prelude traits and isn't able to see through to the actual trait. I've had that happen a few times.

they shouldn't be doing that

i agree

davidbarsky avatar Jan 21 '22 17:01 davidbarsky

Hmm, okay, the number of results in that search is making me significantly less comfortable about changing this in a non-breaking release. I think it might be better off waiting for 0.4.

That's really disappointing. :/

hawkw avatar Jan 21 '22 17:01 hawkw

I can tell you that I almost exclusively use VS Code/Rust Analyzer's feature to automatically add the import statement for a given type, so I guess Rust Analyzer is finding matching types and because of the __ prefix, it's sorting them lexicographically and these ones are coming up first? (EDIT: woops, David already covered this so this is more of a +1 to his assessment)

Might also be worth reaching out to the RA team since it does seem like a weird behavior for it to suggest them when they wouldn't otherwise come up in normal API documentation, etc.

tobz avatar Jan 21 '22 18:01 tobz

Maybe you could do something like

pub use crate::layer::{Layer as _, SubscriberExt as _};

// Remove in 0.4
#[doc(hidden)]
pub use crate::layer::{
    Layer as __tracing_subscriber_Layer, SubscriberExt as __tracing_subscriber_SubscriberExt,
};

I don't know if this will fix error messages, but it will at least get rid of the weirdness in the published docs.

lilyball avatar Jan 21 '22 23:01 lilyball

Ooh, can you deprecate re-exports? That might be nice too.

lilyball avatar Jan 21 '22 23:01 lilyball

@lilyball: Unfortunately, I don't believe re-exports can be deprecated: https://github.com/rust-lang/rust/issues/30827

It might be worth using the approach in your first comment, though, if that fixes the weird error messages. It would be nicer if we could emit deprecation warnings when rust-analyzer imports a type via its re-exported name, but that doesn't appear to be possible today...

hawkw avatar Jan 24 '22 22:01 hawkw