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

subscriber: re-export prelude traits as _ #1850

Open
wants to merge 1 commit into
base: v0.1.x
Choose a base branch
from
Open

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Jan 21, 2022

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

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 hawkw requested a review from a team as a code owner January 21, 2022 17:17
Copy link
Member

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved pending compile errors

@davidbarsky
Copy link
Member

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

@hawkw
Copy link
Member Author

hawkw commented Jan 21, 2022

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
Copy link
Member Author

hawkw commented Jan 21, 2022

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
Copy link
Member Author

hawkw commented Jan 21, 2022

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

@davidbarsky
Copy link
Member

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

@hawkw
Copy link
Member Author

hawkw commented Jan 21, 2022

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. :/

@tobz
Copy link
Member

tobz commented Jan 21, 2022

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.

@hawkw hawkw added this to the tracing-subscriber 0.4 milestone Jan 21, 2022
@hawkw hawkw added the meta/breaking This is a breaking change, and should wait until the next breaking release. label Jan 21, 2022
@lilyball
Copy link
Contributor

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
Copy link
Contributor

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

@hawkw
Copy link
Member Author

hawkw commented Jan 24, 2022

@lilyball:
Unfortunately, I don't believe re-exports can be deprecated: rust-lang/rust#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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta/breaking This is a breaking change, and should wait until the next breaking release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants