Improve compile errors for unimplemented traits #1436
Conversation
axum/src/handler/mod.rs
Outdated
| #[cfg_attr( | ||
| feature = "nightly-error-messages", | ||
| rustc_on_unimplemented( | ||
| note = "Consider using `#[axum_macros::debug_handler]` to improve the error message" |
There was a problem hiding this comment.
Nice! 😄
Can you maybe add one or two tests for the on_unimplemented errors?
There was a problem hiding this comment.
The question is where and how such tests can be added? Running them would always require a nightly compiler + trybuild/compiletests. I'm not that familiar with the axum project setup to answer this question, but if someone points me into the right direction I'm happy to add them there.
There was a problem hiding this comment.
We are already feature-gating the existing trybuild tests to a single rust version because stable and nightly often disagree. Have a look at the end of axum-macros/src/lib.rs. I think you can copy that to make a nightly-only trybuild test for the axum crate.
There was a problem hiding this comment.
Although if you're doing this anyways, it's probably more sensible to always compile the test itself, and ignore it on other compiler versions using #[rustversion::attr(not(nightly), ignore)]. (instead of having the test do nothing)
There was a problem hiding this comment.
@weiznich We can also merge this as-is and add those tests in a separate PR, if you prefer?
There was a problem hiding this comment.
That would be a good idea. I'm currently quite busy with other stuff, so I cannot address this comment now (although I've planed to do that next week or the week after).
There was a problem hiding this comment.
Okay. Looks like CI needs to be updated though (we're using --all-features which is now trying to activate the nightly-only stuff on stable / beta).
There was a problem hiding this comment.
I'm also fine with making the required changes later this month.
There was a problem hiding this comment.
I've pushed a new version that enables this by default on nightly compilers and removes the explicit feature flag. Also this is now integrated in the axum-macros test suite output, although it's not clear for me if that's run on CI yet?
This results in better localised error messages, as they now point directly to the corresponding argument instead of to the macro itself.
flag This uses the nightly only `rustc_on_unimplemented` attribute to improve some error messages when users try to use invalid handler functions. This should be seen as prove of concept, not as full solution for all potential error cases. The underlying feature is currently marked as permanently unstable, but I'm working on getting this specific attribute (or an attribute with different name, similar functionality) ready to work on a stable compiler.
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
weiznich
force-pushed
the
improve_error_messages
branch
from
1be6236 to
fe94ab1
Compare
axum-core/src/extract/mod.rs
Outdated
| #[cfg_attr( | ||
| nightly_error_messages, | ||
| rustc_on_unimplemented( | ||
| on( | ||
| _Self = "axum::http::Request<axum::body::Body>", | ||
| message = "Request must always be the last argument to a handler function", | ||
| label = "Move this argument to the end of the argument list", | ||
| ), | ||
| note = "Function argument is no valid axum extractor. \nSee `https://docs.rs/axum/latest/axum/extract/index.html` for details", | ||
| ) | ||
| )] |
There was a problem hiding this comment.
I think this could be tweaked a bit and also need one for FromRequest. But we can play around with that once this is merged.
There was a problem hiding this comment.
Wait a second. Doesn't seem like the nightly tests are run on CI. Need to fix that before we can merge this. I'll look at that later.
I'm also getting an odd error from running this locally:
$ cargo +nightly-2022-10-20 check --tests --all-features
...
error: unreachable `pub` item
--> axum/src/extract/mod.rs:56:9
|
56 | pub use self::matched_path::MatchedPath;
| --- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| help: consider restricting its visibility: `pub(crate)`
|
= help: or consider exporting it for use by other crates
note: the lint level is defined here
--> axum/src/lib.rs:429:9
|
429 | #![deny(unreachable_pub, private_in_public)]
| ^^^^^^^^^^^^^^^
|
Maybe try a newer nightly and bump the toolchain file if things work? I don't get that error locally with a current nightly. |
davidpdrsn
changed the title
These two commits try to improve error messages generated by rustc if some trait bound is not fulfilled.
The first commit simply improves the
#[debug_handler]helper proc-macro attribute to set spans ins certain situation. This results in the corresponding error messages always pointing to a concrete handler argument instead of the attribute itself (see the updated compile tests).The second commit introduces a rust nightly only
nightly-error-messagesfeature flag + uses unstable rust attributes to improve some other error messages as well. It currently mostly points the users to use the#[debug_handler]attribute. In specific situation it additionally adds more information.. This is mostly meant as demonstration of what's possible. I currently try to push for stabilization of the underlying#[rustc_on_unimplemented]attribute.