-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
new lint: missing_must_use_on_future_types
#14066
base: master
Are you sure you want to change the base?
new lint: missing_must_use_on_future_types
#14066
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Centri3 (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
acf6e40
to
dc0d159
Compare
missing_must_use_on_future_types
/// fn foo() -> Foo { Foo } | ||
/// | ||
/// fn main() { | ||
/// foo(); |
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.
I think this would be better as foo().await
, or something else that shows what the correct code is. A more complex example would be nice, i.e., this example might not get the point across
Also, we can remove no_run
if you add .await
, can't we?
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.
I was trying to show accidentally missing the .await
after calling foo()
but yeah I agree this might not be the best example
|
||
impl<'tcx> LateLintPass<'tcx> for MissingMustUseOnFutureTypes { | ||
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { | ||
if let ItemKind::Impl(Impl { |
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.
Would it not be better to start at the type instead and use implements_trait
? It seems like it'd be a lot simpler...
Not sure about performance tho
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.
We should also check if we're in an external macro here.
You can add //@aux-build:proc_macros.rs
at the top of your test to test macros.
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.
In the local macro case, we mostly should ensure that the span points to the right place. This could be problematic
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.
Would it not be better to start at the type instead and use
implements_trait
? It seems like it'd be a lot simpler...Not sure about performance tho
I think this wouldn't work for the SometimesFuture
since it would always need to hold?
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.
I'm honestly not sure! If not then yeah, this is fine.
I'd look and see anyways because I'm not sure how implements_trait
/what it calls handles not passing any arguments, or whether there's a way to make it check for any impl at all (I believe this same code would be used for T: Clone
for example). It uses Obligation
+ InferCtxt
if curious (I am).
/// fn foo() -> Foo { Foo } | ||
/// | ||
/// fn main() { | ||
/// foo().await; |
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.
This isn't complete; I meant adding .await
alongside #[must_use]
in the "Use instead" example to convey the improvement this makes. Maybe also with a comment pointing this out
|
||
struct SometimesFuture<T>(PhantomData<T>); //~ ERROR: missing a `#[must_use]` attribute | ||
|
||
impl<T: Copy> Future for SometimesFuture<T> { |
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.
I would like to make sure two Future
impls doesn't lint twice.
One idea if they do: Store all the data needed to lint in the struct, then lint in check_crate_post
using span_lint_hir_and_then
.
Adds a lint that checks if
Future
is implemented for a type (struct or enum) and will warn if there is no#[must_use]
attribute since futures must be awaited to do work, similar to howasync fn
works.Example output:
Closes #13886
changelog: [
missing_must_use_on_future_types
]: new lint