-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Make processing of entrypoints more efficient #4513
Conversation
Nice! cc @taiki-e would love your review of this. |
minrust constraint I didn't foresee. Not a big deal so will fix: https://github.com/tokio-rs/tokio/runs/5235296039?check_suite_focus=true |
To resolve the lint failure in https://github.com/tokio-rs/tokio/runs/5236195058?check_suite_focus=true I believe we'd have to do something similar as in #4176 which requires a bit more effort! I'll take a look at later so marked as draft for now. The
And this version doesn't support the required minimal. Not sure what to do about this right now! |
Hm. So after pondering the implementation for a bit I have a couple of thoughts. What is desirable to me would be to perform the following transformation rather than what we're doing right now: #[tokio::main]
async fn main() <ret> <block> Into: fn main() <ret> {
async fn main() <ret> <block>
<runtime>.block_on(main())
} Now this is easy to do and it catches essentially all forms of diagnostics emitted by the rust compiler, it does however cause a regression which was initially documented by @taiki-e in #3766 (comment) where this solution is also suggested and discarded because it isn't backwards compatible.
So there's a couple of thoughts I have about this:
If I could go back, I'd probably argue for not supporting this use and make any arguments specified on functions with the So my suggestion would be to consider introducing this as a breaking change. One which does produce a clear error message that other use constitutes a misuse of the attribute. One might even consider it a bug that is patched, and the scope of the breakage seems limited because frankly I've never seen this use outside of test cases in Tokio. If I missed something important of why such an expansion above wouldn't be appropriate, please tell me! That means, uses like these would not be permitted (because they have an argument): trait Foo {
#[tokio::main]
async fn foo(self) {
}
}
#[tokio::main]
async fn foo(v: u32) {
} Examples of improved diagnosticsThe following are a few examples where we currently do not provide great diagnostics with Non-trivial control flow in blocksThe following is not caught, because the return is inside of a nested expression. We only perform return-level diagnostics at the root level of the function: https://github.com/tokio-rs/tokio/blob/master/tokio-macros/src/entry.rs#L342 We can't catch many other use cases without processing the entirety of the function body, so most forms of control flow simply do not work as intended: #[tokio::main]
async fn main() -> () {
if true {
return Ok(());
}
} Tokio today. Note that the tokio output can be improved by adding
Example breaking out of a loop: #[tokio::main]
async fn main() {
loop {
break Ok(());
}
} Tokio today:
Spurious "consider using a semicolon here" suggestions#[tokio::main]
async fn main() {
true
} Tokio today:
The reason this happens is that the compiler diagnostics considers the fully expanded expression, which probably makes sense to keep (e.g. it might have side effects): use tokio::runtime::Builder;
fn main() {
Builder::new().build().unwrap().block_on(async {
true
})
} It's avoided with the other form of expansion, because the compiler gets to consider a normal full function: use tokio::runtime::{Runtime, Builder};
fn main() {
async fn main() {
true
}
Builder::new().build().unwrap().block_on(main())
} |
Argument support is a feature that was added intentionally. (see #1564 and #1594) My comment in #3766 is about the |
Thanks for context! So arguments could be supported with a bit of mangling (the tricky aspect is patterns). |
Also worth noting that there are some more insidious examples of poor diagnostics today: Thanks to a fairly confused instance of type inference we've convinced rustc to say both of these things about the span representing the
#[tokio::main]
async fn foo() -> bool {
if true {
return Ok(());
}
true
}
fn main() {
}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- None-delimited groups do not seem to be handled properly. (dtolnay/proc-macro-hack@51a988b)
- There also seem to be problems with the function signature handling. (generics, where clause, etc.)
I have written a few such syn-independent proc macros, but it is quite difficult to write a parser that works properly with generics and where clause. Some of the cases where I made mistakes probably apply to this implementation as well (taiki-e/const_fn@7ed26cb, taiki-e/easy-ext@f374d87)
I like the approach of avoiding parsing the function body, but I think it is preferable to let syn handle the function signature. (an example of such a function parsing can be found here.)
It looks like this will be handled well if None-delimited groups are handled properly, but I think I'm likely still missing an edge case. |
Several users are already using the self argument in tokio::main, so I'm not going to accept/approve removing support for self argument. |
I think a full evaluation demands that it be weighed against the downside of things like poor diagnostics for everyone who is using the attributes a The behavior today is quite poor and has caused me to struggle to understand what went wrong on a few occasions. The only reason I knew to get better diagnostics by moving my function body into my own secondary function is because I was vaguely aware of how the attribute was implemented. But if this break is not desired it will simply have to wait until Tokio 2.0! Edit: I think I'll punt that for now anyways into a separate issue. There might be things with typed closures we can use to improve diagnostics while retaining the full ability of today. I haven't investigated it fully yet. |
439841d fixes |
That should be that. Return type heuristics added in dcc8a7a and the preliminary comments should have been addressed so I'm marking this as ready for review. Note that this does not include a breaking transformation. Or put in other terms, any breakage is unintended (and isn't covered by a test!). I'll convert #4513 (comment) into a separate issue once I get a second to reformat it. |
This sets the minimal version of `syn` to `1.0.43` - the actual minimal version that is required to build `tracing-attributes`. See this build failure in Tokio: https://github.com/tokio-rs/tokio/runs/5344598894?check_suite_focus=true CC: tokio-rs/tokio#4513
The minimal versions check has a weird failure in a dependency:
|
@Darksonn that should be tokio-rs/tracing#1960 - something could probably be done solely in Tokio but I'm not sure what. |
This sets the minimal version of `syn` to `1.0.43` - the actual minimal version that is required to build `tracing-attributes`. See this build failure in Tokio: https://github.com/tokio-rs/tokio/runs/5344598894?check_suite_focus=true CC: tokio-rs/tokio#4513
This sets the minimal version of `syn` to `1.0.43` - the actual minimal version that is required to build `tracing-attributes`. See this build failure in Tokio: https://github.com/tokio-rs/tokio/runs/5344598894?check_suite_focus=true CC: tokio-rs/tokio#4513
Any updates on this? |
No, we just needed to bump |
Hm, another one:
See rustwasm/wasm-bindgen#2378 Minimal version of |
What is the status? |
PR is in an OK state. It's the minimal versions stuff that is both very time consuming when I sit down with it and I have little motivation to work on. For now I'm blocked on figuring out how to get all individual crates of tracing to build with minimal-versions (and that sometimes lead me down a tree of broken deps). And lack of tooling is making it difficult. See the associated PR above. Alternatives or help is welcome! |
This adds a minimal-versions check to the tracing project. Adapted from `tokio-rs/tokio`. Adding this avoids breaking downstream dependencies from accidentally under-constraining minimal versions of dependencies when they depend on tracing. I've currently just introduced the check. I will try to and do encourage others to add patches to fix this where possible since it can be a fair bit of work to chase down a version of all dependencies that passes minimal-versions and is msrv. I've also seen some really odd windows-specific issues (which are not being tested for here). This is currently only testing `tracing`, `tracing-core`, and `tracing-subscriber`. Packages such as `tracing-futures` are proving to be a bit harder to deal with due to having features which enable very old dependencies. Steps to test the build minimal versions locally: ```sh cargo install cargo-hack rustup default nightly cargo hack --remove-dev-deps --workspace cargo update -Z minimal-versions cargo hack check --all-features --ignore-private ``` CC: tokio-rs/tokio#4513
This adds a minimal-versions check to the tracing project. Adapted from `tokio-rs/tokio`. Adding this avoids breaking downstream dependencies from accidentally under-constraining minimal versions of dependencies when they depend on tracing. I've currently just introduced the check. I will try to and do encourage others to add patches to fix this where possible since it can be a fair bit of work to chase down a version of all dependencies that passes minimal-versions and is msrv. I've also seen some really odd windows-specific issues (which are not being tested for here). This is currently only testing `tracing`, `tracing-core`, and `tracing-subscriber`. Packages such as `tracing-futures` are proving to be a bit harder to deal with due to having features which enable very old dependencies. Steps to test the build minimal versions locally: ```sh cargo install cargo-hack rustup default nightly cargo hack --remove-dev-deps --workspace cargo update -Z minimal-versions cargo hack check --all-features --ignore-private ``` CC: tokio-rs/tokio#4513
This adds a minimal-versions check to the tracing project. Adapted from `tokio-rs/tokio`. Adding this avoids breaking downstream dependencies from accidentally under-constraining minimal versions of dependencies when they depend on tracing. I've currently just introduced the check. I will try to and do encourage others to add patches to fix this where possible since it can be a fair bit of work to chase down a version of all dependencies that passes minimal-versions and is msrv. I've also seen some really odd windows-specific issues (which are not being tested for here). This is currently only testing `tracing`, `tracing-core`, and `tracing-subscriber`. Packages such as `tracing-futures` are proving to be a bit harder to deal with due to having features which enable very old dependencies. Steps to test the build minimal versions locally: ```sh cargo install cargo-hack rustup default nightly cargo hack --remove-dev-deps --workspace cargo update -Z minimal-versions cargo hack check --all-features --ignore-private ``` CC: tokio-rs/tokio#4513
This sets the minimal version of `syn` to `1.0.43` - the actual minimal version that is required to build `tracing-attributes`. See this build failure in Tokio: https://github.com/tokio-rs/tokio/runs/5344598894?check_suite_focus=true CC: tokio-rs/tokio#4513
This adds a minimal-versions check to the tracing project. Adapted from `tokio-rs/tokio`. Adding this avoids breaking downstream dependencies from accidentally under-constraining minimal versions of dependencies when they depend on tracing. I've currently just introduced the check. I will try to and do encourage others to add patches to fix this where possible since it can be a fair bit of work to chase down a version of all dependencies that passes minimal-versions and is msrv. I've also seen some really odd windows-specific issues (which are not being tested for here). This is currently only testing `tracing`, `tracing-core`, and `tracing-subscriber`. Packages such as `tracing-futures` are proving to be a bit harder to deal with due to having features which enable very old dependencies. Steps to test the build minimal versions locally: ```sh cargo install cargo-hack rustup default nightly cargo hack --remove-dev-deps --workspace cargo update -Z minimal-versions cargo hack check --all-features --ignore-private ``` CC: tokio-rs/tokio#4513
This reverts commit 0d85b2c.
Closing in favor of #5621 (which adopts the approach I mentioned in #4513 (review)) for now. It is more robust and easy to maintain. |
This changes processing of entrypoints to operate over token streams directly instead of parsing them (through
syn
) and drop the dependency on bothsyn
(since it's not used), andquote
which can be replaced with a fairly simple internal abstraction (seeToTokens
).Motivation
We do a few things with the current solution which is somewhat "wasteful" in terms of macro processing time:
We feed the item being processed through syn by parsing it as an
ItemFn
. This has the unintended side effect of parsing the entire function body (including all the code in it) to produce an instance ofItemFn
: https://github.com/tokio-rs/tokio/blob/8943e8a/tokio-macros/src/entry.rs#L387. We manipulate the function by producing a token stream which we parse again (this time as a block) causing the entire function body to be parsed another time: https://github.com/tokio-rs/tokio/blob/8943e8a/tokio-macros/src/entry.rs#L355.This can be quite slow. I've measured a large entrypoint in the wild which takes 200ms to process with the old solution and ~200us with the new one. While this doesn't have a huge impact on full compiles, it appears to be noticeable when used interactively through rust-analyzer (and it obviously stacks up when compiling a lot of things).
Parsing with
syn
doesn't give us anything that passing the stream back torustc
once it's been manipulated does.rustc
even tends to produce better diagnostics for malformed inputs.*Compare this (error generated and reported with
syn
):With this (error generated by passing the tokens to rustc with the new solution):
Note the extra
error: expected identifier
in the first variant. This is diagnostics that is produced bysyn
failing to parse. And since we pass tokens through without processing on errors we get the diagnostics byrustc
immediately before it anyways.In the latter we don't care what syntax the body contains beyond some fairly simple markers we look for, we correctly process the main body and avoid producing the third noisy error stating "
main
function is not allowed to beasync
" despite it being so.Solution
Do our own custom internal processing of macros instead of relying on
syn
that simply looks for known "markers". Like the function signature is expected to be before a braced ({}
) group and perform minimal processing over it to produce the desired entrypoint.I've also made all diagnostics use the following convention (instead of it being mixed) which causes some ui tests to need updates. Messages are single sentence and all lowercase with no dots
An error happened. Supported syntax is: foo.
then becomesan error happened, supported syntax is: foo
(this follows rustc convention AFAICT).I've also decided against pre-validating some options that are parsed in the entrypoint. As can be seen in the error we now get in the ui test if a flavor doesn't exist:
This used to say:
I personally think the new behavior is better, since it informs the user exactly what to change their illegal input to regardless of what it happens to be. So you avoid an extra step of making it into a string and then potentially correcting it.
open questions
What is the purpose of looking for additional
#[test]
attributes in#[tokio::test]
? I didn't implement it yet, and the only test that fails is a UI test. But multiple#[test]
attributes are legal from the perspective ofrustc
- the test will simply run multiple times. Should the old behavior be reproduced?The code was ported from experimental entrypoints I tinkered with in
selectme
(which I'm the author of so I'm giving it to Tokio instead). Some rough edges might still be present in the implementation so a review is welcome!