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.

Sign up for GitHub

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

Jump to bottom

use fully qualified names in macros for items exported from std prelude #2621

Merged
merged 5 commits into from Jul 11, 2023
Merged

use fully qualified names in macros for items exported from std prelude #2621

merged 5 commits into from Jul 11, 2023

Conversation

hlbarber
Copy link
Contributor

Motivation

Currently, in many places, macros do not use fully qualified names for items exported from the prelude. This means that naming collisions (struct Some) or the removal of the std library prelude will cause compilation errors.

Solution

  • Identify and use fully qualified names in macros were we previously assumed the Rust std prelude. We use ::core rather than ::std.
  • Add no_implicit_prelude to tracing/tests/macros.rs. I'm unsure if this is giving us good coverage - can we improve on this approach? I'm not confident I've caught everything.

@hlbarber hlbarber mentioned this pull request Jun 17, 2023
Closed
@hlbarber hlbarber changed the title Use fully qualified names in macros for items exported from standard library prelude use fully qualified names in macros for items exported from std prelude Jun 18, 2023
hawkw
hawkw approved these changes Jun 19, 2023
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

I notice we've applied the change to both macros that are publicly exported and to some internal macros that are used only to generate code inside various tracing crates. I don't actually think it's necessary to avoid the use of prelude imports in macros that are only used locally within the crate. I'm fine with changing them as well if it solves a problem, but I wanted to ask about the motivation for that — AFAIK, because those macros are only ever used within a tracing crate where the prelude is imported and prelude names are not shadowed, there's no reason to use the fully qualified names there?

tracing-subscriber/src/filter/subscriber_filters/mod.rs Outdated
@@ -475,7 +475,7 @@ macro_rules! filter_impl_body {
}

#[inline]
fn max_level_hint(&self) -> Option<LevelFilter> {
fn max_level_hint(&self) -> ::core::option::Option<LevelFilter> {
Copy link
Member

Choose a reason for hiding this comment

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

this macro is only used internally; it's not publicly re-exported. i think it would be fine to continue using the prelude, or add a module-level import here?

tracing-core/src/field.rs Outdated
@@ -318,40 +318,40 @@ macro_rules! impl_values {

macro_rules! ty_to_nonzero {
Copy link
Member

Choose a reason for hiding this comment

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

this macro is only used internally; it's not publicly re-exported. i think it would be fine to continue using the prelude, or add a module-level import here?

tracing-subscriber/src/subscribe/mod.rs Outdated
@@ -1723,13 +1723,13 @@ macro_rules! subscriber_impl_body {
}

#[inline]
fn max_level_hint(&self) -> Option<LevelFilter> {
fn max_level_hint(&self) -> ::core::option::Option<LevelFilter> {
Copy link
Member

Choose a reason for hiding this comment

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

this macro is only used internally; it's not publicly re-exported. i think it would be fine to continue using the prelude, or add a module-level import here?

@hlbarber
Copy link
Contributor Author

I don't actually think it's necessary to avoid the use of prelude imports in macros that are only used locally within the crate. I'm fine with changing them as well if it solves a problem, but I wanted to ask about the motivation for that

It doesn't solve a problem, they just got caught in the net I threw while changing the rest. I've reverted them here.

@hawkw hawkw enabled auto-merge (squash) July 11, 2023 19:25
@hawkw
Copy link
Member

hawkw commented Jul 11, 2023

Hi @hlbarber, thanks again for working on this --- I accidentally let this PR drop off my radar a bit, but it looks good to me and I'm going to go ahead and merge it now. Thank you!

@hawkw hawkw merged commit 27f688e into tokio-rs:master Jul 11, 2023
54 of 56 checks passed
@hlbarber hlbarber deleted the fully-qualified-some branch July 11, 2023 19:40
davidbarsky pushed a commit that referenced this pull request Sep 26, 2023
@hlbarber @davidbarsky
tracing: use fully qualified names in macros for items exported from …
df581c1 
…std prelude (#2621)

Currently, in many places, macros do not use fully qualified names for
items exported from the prelude. This means that naming collisions
(`struct Some`) or the removal of the std library prelude will cause
compilation errors.

- Identify and use fully qualified names in macros were we previously
  assumed the Rust std prelude. We use `::core` rather than `::std`.
- Add
  [`no_implicit_prelude`](https://doc.rust-lang.org/reference/names/preludes.html#the-no_implicit_prelude-attribute)
  to `tracing/tests/macros.rs`. I'm unsure if this is giving us good
  coverage - can we improve on this approach? I'm not confident I've
  caught everything.
@davidbarsky davidbarsky mentioned this pull request Sep 26, 2023
Merged
davidbarsky pushed a commit that referenced this pull request Sep 27, 2023
@hlbarber @davidbarsky
tracing: use fully qualified names in macros for items exported from …
6182d7b 
…std prelude (#2621)

Currently, in many places, macros do not use fully qualified names for
items exported from the prelude. This means that naming collisions
(`struct Some`) or the removal of the std library prelude will cause
compilation errors.

- Identify and use fully qualified names in macros were we previously
  assumed the Rust std prelude. We use `::core` rather than `::std`.
- Add
  [`no_implicit_prelude`](https://doc.rust-lang.org/reference/names/preludes.html#the-no_implicit_prelude-attribute)
  to `tracing/tests/macros.rs`. I'm unsure if this is giving us good
  coverage - can we improve on this approach? I'm not confident I've
  caught everything.
davidbarsky pushed a commit that referenced this pull request Sep 27, 2023
@hlbarber @davidbarsky
tracing: use fully qualified names in macros for items exported from …
733d3ea 
…std prelude (#2621)

Currently, in many places, macros do not use fully qualified names for
items exported from the prelude. This means that naming collisions
(`struct Some`) or the removal of the std library prelude will cause
compilation errors.

- Identify and use fully qualified names in macros were we previously
  assumed the Rust std prelude. We use `::core` rather than `::std`.
- Add
  [`no_implicit_prelude`](https://doc.rust-lang.org/reference/names/preludes.html#the-no_implicit_prelude-attribute)
  to `tracing/tests/macros.rs`. I'm unsure if this is giving us good
  coverage - can we improve on this approach? I'm not confident I've
  caught everything.
davidbarsky pushed a commit that referenced this pull request Sep 27, 2023
@hlbarber @davidbarsky
tracing: use fully qualified names in macros for items exported from …
0b31599 
…std prelude (#2621)

Currently, in many places, macros do not use fully qualified names for
items exported from the prelude. This means that naming collisions
(`struct Some`) or the removal of the std library prelude will cause
compilation errors.

- Identify and use fully qualified names in macros were we previously
  assumed the Rust std prelude. We use `::core` rather than `::std`.
- Add
  [`no_implicit_prelude`](https://doc.rust-lang.org/reference/names/preludes.html#the-no_implicit_prelude-attribute)
  to `tracing/tests/macros.rs`. I'm unsure if this is giving us good
  coverage - can we improve on this approach? I'm not confident I've
  caught everything.
hawkw pushed a commit that referenced this pull request Oct 1, 2023
@hlbarber @hawkw
tracing: use fully qualified names in macros for items exported from …
111ad87 
…std prelude (#2621)

Currently, in many places, macros do not use fully qualified names for
items exported from the prelude. This means that naming collisions
(`struct Some`) or the removal of the std library prelude will cause
compilation errors.

- Identify and use fully qualified names in macros were we previously
  assumed the Rust std prelude. We use `::core` rather than `::std`.
- Add
  [`no_implicit_prelude`](https://doc.rust-lang.org/reference/names/preludes.html#the-no_implicit_prelude-attribute)
  to `tracing/tests/macros.rs`. I'm unsure if this is giving us good
  coverage - can we improve on this approach? I'm not confident I've
  caught everything.
hawkw pushed a commit that referenced this pull request Oct 13, 2023
@davidbarsky @hawkw
chore: prepare tracing 0.1.39 (#2755)
4b99457 
# 0.1.39 (October 12, 2023)

This release adds several additional features to the `tracing` macros.
In addition, it updates the `tracing-core` dependency to
[v0.1.32][core-0.1.32] and the `tracing-attributes` dependency to
[v0.1.27][attrs-0.1.27].

### Added

- Allow constant field names in macros ([#2617])
- Allow setting event names in macros ([#2699])
- **core**: Allow `ValueSet`s of any length ([#2508])

### Changed

- `tracing-attributes`: updated to [0.1.27][attrs-0.1.27]
- `tracing-core`: updated to [0.1.32][core-0.1.32]
- **attributes**: Bump minimum version of proc-macro2 to 1.0.60
  ([#2732])
-  **attributes**: Generate less dead code for async block return type
   hint ([#2709])

### Fixed

- Use fully qualified names in macros for items exported from std
  prelude ([#2621], [#2757])
- **attributes**: Allow [`clippy::let_with_type_underscore`] in
  macro-generated code ([#2609])
- **attributes**: Allow `unknown_lints` in macro-generated code
  ([#2626])
- **attributes**: Fix a compilation error in `#[instrument]` when the
  `"log"` feature is enabled ([#2599])

### Documented

- Add `axum-insights` to relevant crates. ([#2713])
- Fix link to RAI pattern crate documentation ([#2612])
- Fix docs typos and warnings ([#2581])
- Add `clippy-tracing` to related crates ([#2628])
- Add `tracing-cloudwatch` to related crates ([#2667])
- Fix deadlink to `tracing-etw` repo ([#2602])

[#2617]: #2617
[#2699]: #2699
[#2508]: #2508
[#2621]: #2621
[#2713]: #2713
[#2581]: #2581
[#2628]: #2628
[#2667]: #2667
[#2602]: #2602
[#2626]: #2626
[#2757]: #2757
[#2732]: #2732
[#2709]: #2709
[#2599]: #2599
[`let_with_type_underscore`]: http://rust-lang.github.io/rust-clippy/rust-1.70.0/index.html#let_with_type_underscore
[attrs-0.1.27]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.27
[core-0.1.32]:
    https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkw hawkw hawkw approved these changes

@carllerche carllerche Awaiting requested review from carllerche carllerche is a code owner

@davidbarsky davidbarsky Awaiting requested review from davidbarsky davidbarsky is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@hlbarber @hawkw