add large future lint #10414
add large future lint #10414
Conversation
|
r? @xFrednet (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
Hey @csmoe, thank you for the PR. I'll review it over the week. The CI is currently still red, could you maybe take a look at it? :) |
|
@xFrednet Error fixed, may I have your review? thanks :) |
csmoe
force-pushed
the
large-future
branch
7 times, most recently
from
3061ad7
to
e7e84ed
Compare
|
rustc is trying to address this problem, let's wait for that :) |
|
Do you have an estimate for the timeframe? If that'll take a few releases/months we could still add the lint and deprecate it later. :) |
csmoe
force-pushed
the
large-future
branch
2 times, most recently
from
9b7e7a2
to
dc1d61c
Compare
|
@xFrednet Done~ :) |
csmoe
force-pushed
the
large-future
branch
3 times, most recently
from
44d7138
to
b9a239d
Compare
|
@xFrednet ping :) |
|
Hey @csmoe, thank you for your patience, the last two weeks have been very chaotic. You'll have a review by the end of the week! :) |
|
|
||
| impl<'tcx> LateLintPass<'tcx> for LargeFuture { | ||
| fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { | ||
| if let ExprKind::Match(expr, _, MatchSource::AwaitDesugar) = expr.kind { |
There was a problem hiding this comment.
Could you add a check that the future doesn't originate from a macro? I believe it would look like this:
if expr.span.from_expansion() {
return;
}I'm hoping that from_expansion is not true for desugared code, but it shouldn't :)
There was a problem hiding this comment.
if will not works here since we are checking the lowered expr.await by lower_expr_await, which marks the span of expr.await as Desugred as a whole, thus check_if_expand_from_macro always returns false.
Co-authored-by: Fridtjof Stoldt <xFrednet@gmail.com>
|
This version looks good to me, thank you very much for the implementation :) @bors r+ |
|
💔 Test failed - checks-action_test |
|
Let's try this again :) @bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
1 similar comment
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
| /// ```rust | ||
| /// async fn wait(f: impl std::future::Future<Output = ()>) {} | ||
| /// | ||
| /// async fn big_fut(arg: [u8; 1024]) {} |
There was a problem hiding this comment.
@csmoe Given the default threshold of 16*1024 bytes, I don't think the example code in the lint documentation will actually trigger the lint.
Besides this, can you maybe shed some light on why:
async fn fut() {
let _x = [0; 16*1024];
async {}.await;
}
async fn main() {
fut().await; // <- expect to trigger here
}Does not trigger the lint? In my mind the future returned by fut needs to keep around _x until after the await point. Is this somehow optimized out or am I just not thinking about this correctly?
There was a problem hiding this comment.
yes, _x is optimized out as it does not live across await point(see my test cases, I made it by a printf). You can check fut()'s size with std::mem::size_of_val, it should be very small.
There was a problem hiding this comment.
Interesting that it can optimize a local but not an argument.
I have opened #11553 to address what I believe to be a broken example.
Closes #5263
changelog: new lint: [
large_futures]#10414