Skip to content
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

Rollup of 9 pull requests #101318

Merged
merged 47 commits into from
Sep 2, 2022
Merged

Rollup of 9 pull requests #101318

merged 47 commits into from
Sep 2, 2022

Conversation

GuillaumeGomez
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

a2aaron and others added 30 commits May 29, 2022 16:20
This lint checks for statements similar to `let _ = foo`, where `foo` is
a type that implements `Drop`. These types of let statements cause the
expression in them to be dropped immediately, instead of at the end of
the scope. Such behavior can be surprizing, especially if you are
relying on the value to be dropped at the end of the scope. Instead, the
binding should be an underscore prefixed name (like `_unused`) or the
value should explicitly be passed to `std::mem::drop()` if the value
really should be dropped immediately.
Similar to `let_underscore_drop`, this lint checks for statements similar
to `let _ = foo`, where `foo` is a lock guard. These types of let
statements are especially problematic because the lock gets released
immediately, instead of at the end of the scope. This behavior is almost
always the wrong thing.
Similar to `let_underscore_drop`, this lint checks for statements similar
to `let _ = foo`, where `foo` is an expression marked `must_use`.
These lints are very noisy and are allow-by-default in clippy anyways.
Hence, setting them to allow-by-default here makes more sense than
warning constantly on these cases.
This commit uses `span_suggestion_verbose` to add what specific code
changes can be done as suggested by the lint--in this case, either binding
the expression to an unused variable or using `std::mem::drop` to drop
the value explicitly.
Clippy sets this lint to Deny by default, and it having the lint be Deny
is useful for when we test the lint against a Crater run.
If the type has a trivial Drop implementation, then it is probably irrelevant
that the type was dropped immediately, since nothing important
happens on drop. Hence, we can bail out early instead of doing some
expensive checks.
…lock`

Using diagnostic items avoids having to update the paths if the guard
types ever get moved around for some reason. Additionally, it also greatly
simplifies the `is_sync_lock` check.
We don't actually care about running these programs, only checking the
warnings they generate.
The `let_underscore_must_use` lint was really only added because clippy
included it, but it doesn't actually seem very useful.
I forgot to add the diagnostic to the actual types in `std` earlier.
The "consider explicitly droping" can now suggest a machine applicable
suggestion now.
This is done so that we can check the noisiness of this lint in a Crater
run. Note that when I built the compiler, I actually encountered lots of
places where this lint will trigger and fail compilation, so I had to
also set `RUSTFLAGS_NOT_BOOSTRAP` to `-A let_underscore_drop` when
compiling to prevent that.
This lint is way way too noisy to have it be `Deny` by default.
Currently, the let_underscore_lock lint simply tells what is wrong, but
not why it is wrong. We fix this by using a `MultiSpan` to explain
specifically that doing `let _ = ` immediately drops the lock guard
because it does not assign the lock guard to a binding.
I'm not really sure why this is nessecary to do, but the checks on the
PR do not seem to work if do not do this.
There are times where computing a value may be cheap, or where
computing a reference may be expensive, so this fills out the
possibilities.
…ref_of}`

While the `provide_*` methods already short-circuit when a value has
been provided, there are times where an expensive computation is
needed to determine if the `provide_*` method can even be called.
@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. rollup A PR which is a rollup labels Sep 2, 2022
@GuillaumeGomez
Copy link
Member Author

@bors r+ p=5 rollup=never

@bors
Copy link
Contributor

bors commented Sep 2, 2022

📌 Commit 138121a has been approved by GuillaumeGomez

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 2, 2022
@bors
Copy link
Contributor

bors commented Sep 2, 2022

⌛ Testing commit 138121a with merge 9353538...

@bors
Copy link
Contributor

bors commented Sep 2, 2022

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 9353538 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 2, 2022
@bors bors merged commit 9353538 into rust-lang:master Sep 2, 2022
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #101318!

Tested on commit 9353538.
Direct link to PR: #101318

💔 miri on linux: test-pass → test-fail (cc @oli-obk @RalfJung).

@rustbot rustbot added this to the 1.65.0 milestone Sep 2, 2022
rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Sep 2, 2022
Tested on commit rust-lang/rust@9353538.
Direct link to PR: <rust-lang/rust#101318>

💔 miri on linux: test-pass → test-fail (cc @oli-obk @RalfJung).
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9353538): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
0.2% [0.2%, 0.2%] 3
Regressions ❌
(secondary)
1.0% [0.3%, 3.9%] 5
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.8% [-0.8%, -0.8%] 1
All ❌✅ (primary) 0.2% [0.2%, 0.2%] 3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
2.5% [2.5%, 2.5%] 1
Regressions ❌
(secondary)
5.4% [3.0%, 9.5%] 6
Improvements ✅
(primary)
-3.2% [-4.1%, -2.3%] 2
Improvements ✅
(secondary)
-3.9% [-5.8%, -2.0%] 3
All ❌✅ (primary) -1.3% [-4.1%, 2.5%] 3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
2.2% [2.2%, 2.2%] 1
Regressions ❌
(secondary)
4.6% [4.6%, 4.6%] 1
Improvements ✅
(primary)
-2.0% [-2.0%, -2.0%] 1
Improvements ✅
(secondary)
-5.1% [-5.1%, -5.1%] 1
All ❌✅ (primary) 0.1% [-2.0%, 2.2%] 2

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot added the perf-regression Performance regression. label Sep 2, 2022
@rylev
Copy link
Member

rylev commented Sep 6, 2022

@rust-timer build d1db6775c125a3f8e0a0546faa427d947ea52a88

@rust-timer
Copy link
Collaborator

Queued d1db6775c125a3f8e0a0546faa427d947ea52a88 with parent e21d771, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d1db6775c125a3f8e0a0546faa427d947ea52a88): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.3%, 0.3%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.3% [4.3%, 4.3%] 1
Improvements ✅
(primary)
-4.1% [-4.1%, -4.1%] 1
Improvements ✅
(secondary)
-3.0% [-3.0%, -3.0%] 1
All ❌✅ (primary) -4.1% [-4.1%, -4.1%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-5.3% [-5.3%, -5.3%] 1
All ❌✅ (primary) - - 0

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed perf-regression Performance regression. labels Sep 6, 2022
@davidtwco davidtwco removed the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. rollup A PR which is a rollup S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.