macros: improve overall macro hygiene#7997
Conversation
| $crate::macros::support::Poll::Pending | ||
| } else { | ||
| $crate::macros::support::Poll::Ready(Ok(($({ | ||
| $crate::macros::support::Poll::Ready(::std::result::Result::Ok(($({ |
There was a problem hiding this comment.
Can you re-export it from $crate::macros::support instead? That's more reliable than using ::std.
c5922db to
2c22fc1
Compare
There was a problem hiding this comment.
Let's fix this one too, while we're at it.
There was a problem hiding this comment.
There are more in the other modules:
- https://github.com/dybucc/tokio/blob/2c22fc1973621893f07c43ebb5f49b8e03fabe1e/tokio/src/macros/addr_of.rs#L8
- https://github.com/dybucc/tokio/blob/2c22fc1973621893f07c43ebb5f49b8e03fabe1e/tokio/src/macros/select.rs#L702
- https://github.com/dybucc/tokio/blob/2c22fc1973621893f07c43ebb5f49b8e03fabe1e/tokio/src/macros/support.rs#L16
- https://github.com/dybucc/tokio/blob/2c22fc1973621893f07c43ebb5f49b8e03fabe1e/tokio/src/macros/support.rs#L24
- https://github.com/dybucc/tokio/blob/2c22fc1973621893f07c43ebb5f49b8e03fabe1e/tokio/src/macros/support.rs#L25
- https://github.com/dybucc/tokio/blob/2c22fc1973621893f07c43ebb5f49b8e03fabe1e/tokio/src/macros/trace.rs#L15-L21
I think the one in select.rs is intentionally left because it breaks some axum related crate.
There was a problem hiding this comment.
The improvements in select.rs for 1.50.0 broke the build of Apache DataFusion, but it was very easy to fix - apache/datafusion#21000
I am in favour to improve the last entry in select.rs too!
There was a problem hiding this comment.
There are more in the other modules:
* https://github.com/dybucc/tokio/blob/2c22fc1973621893f07c43ebb5f49b8e03fabe1e/tokio/src/macros/addr_of.rs#L8 * https://github.com/dybucc/tokio/blob/2c22fc1973621893f07c43ebb5f49b8e03fabe1e/tokio/src/macros/select.rs#L702 * https://github.com/dybucc/tokio/blob/2c22fc1973621893f07c43ebb5f49b8e03fabe1e/tokio/src/macros/support.rs#L16 * https://github.com/dybucc/tokio/blob/2c22fc1973621893f07c43ebb5f49b8e03fabe1e/tokio/src/macros/support.rs#L24 * https://github.com/dybucc/tokio/blob/2c22fc1973621893f07c43ebb5f49b8e03fabe1e/tokio/src/macros/support.rs#L25 * https://github.com/dybucc/tokio/blob/2c22fc1973621893f07c43ebb5f49b8e03fabe1e/tokio/src/macros/trace.rs#L15-L21I think the one in select.rs is intentionally left because it breaks some axum related crate.
@martin-g Done. Still, is the list item concerning addr_of really meant to use absolute paths? That one is part of a macro matcher and not a macro invocation, so it wouldn't make a lot of sense to force callers to use absolute paths, right?
I'm also not so sure about the support.rs ones, so I've not made changes there either, but I can't make a case against not using absolute paths/$crate-relative paths in this instance.
Multiple macro expansions were previously assumming the existence of some standard library symbols to both be in-scope and referring to the right symbol, and not only having the same identifier. This has now been refactored into using reexports from the `support` module under the `macros` module. Some other macro invocations were also using absolute standard library paths instead of calling into the afore mentioned module through the special `$crate` macro. This has been changed, thus also reexporting some other items in `support`.
2c22fc1 to
53277b2
Compare
try_join macro hygiene
martin-g
left a comment
There was a problem hiding this comment.
LGTM!
The fixed Pin/Ok/Err may break some users (e.g. hyper - #7929 (comment)) but IMO it is OK.
Darksonn
left a comment
There was a problem hiding this comment.
We have to keep imports to avoid breaking users, but changing the actual uses to full paths sounds good to me.
Motivation
Multiple macro expansions were previously assuming the existence of some
standard library symbols to both be in-scope and referring to the right symbol,
and not only having the same identifier.
Solution
This has now been refactored into using reexports from the
supportmoduleunder the
macrosmodule. Some other macro invocations were also using absolutestandard library paths instead of calling into the afore mentioned module
through the special
$cratemetavariable. This has been changed, thus alsoreexporting some other items in
support.