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

A bunch of cleanups #133567

Merged
merged 8 commits into from
Dec 9, 2024
Merged

A bunch of cleanups #133567

merged 8 commits into from
Dec 9, 2024

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Nov 28, 2024

These are all extracted from a branch I have to get rid of driver queries. Most of the commits are not directly necessary for this, but were found in the process of implementing the removal of driver queries.

Previous PR: #132410

@rustbot
Copy link
Collaborator

rustbot commented Nov 28, 2024

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler 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. labels Nov 28, 2024
@rustbot
Copy link
Collaborator

rustbot commented Nov 28, 2024

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 28, 2024

☔ The latest upstream changes (presumably #133568) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@cjgillot cjgillot self-assigned this Nov 29, 2024
@bors
Copy link
Contributor

bors commented Dec 6, 2024

☔ The latest upstream changes (presumably #133956) made this pull request unmergeable. Please resolve the merge conflicts.

And pass this to the individual emitters when necessary.
It was inconsistently done (sometimes even within a single function) and
most of the rest of the compiler uses fatal errors instead, which need
to be caught using catch_with_exit_code anyway. Using fatal errors
instead of ErrorGuaranteed everywhere in the driver simplifies things a
bit.
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 nits and r=me

@@ -446,23 +449,21 @@ fn run_compiler(
return early_exit();
}

tcx.analysis(())?;
let _ = tcx.analysis(());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use tcx.ensure().analysis(()).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The analysis query returns a unit, so skipping returning of the query result doesn't save us anything. I think we do however need to make sure that the query gets executed (rather than just doing nothing if all dependencies are unchanged) to prevent query stealing from going wrong and I'm not sure if .ensure() does that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

analysis is eval_always, so yes, it will be re-executed in any case.
ensure() is more about uniform style. When the return type is not (), let _ = some query should always be replaced by ensure() or ensure_with_value(), to make meaning explicit.

if ppm.needs_analysis() && ex.tcx().analysis(()).is_err() {
FatalError.raise();
if ppm.needs_analysis() {
let _ = ex.tcx().analysis(());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use ensure().

compiler/rustc_interface/src/interface.rs Outdated Show resolved Hide resolved
@bjorn3
Copy link
Member Author

bjorn3 commented Dec 8, 2024

@bors r=cjgillot

@bors
Copy link
Contributor

bors commented Dec 8, 2024

📌 Commit 0df8094 has been approved by cjgillot

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 8, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#133567 (A bunch of cleanups)
 - rust-lang#133789 (Add doc alias 'then_with' for `then` method on `bool`)
 - rust-lang#133880 (Expand home_dir docs)
 - rust-lang#134036 (crash tests: use individual mir opts instead of mir-opt-level where easily possible)
 - rust-lang#134045 (Fix some triagebot mentions paths)
 - rust-lang#134046 (Remove ignored tests for hangs w/ new solver)
 - rust-lang#134050 (Miri subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d2881e4 into rust-lang:master Dec 9, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2024
Rollup merge of rust-lang#133567 - bjorn3:various_cleanups, r=cjgillot

A bunch of cleanups

These are all extracted from a branch I have to get rid of driver queries. Most of the commits are not directly necessary for this, but were found in the process of implementing the removal of driver queries.

Previous PR: rust-lang#132410
@bjorn3 bjorn3 deleted the various_cleanups branch December 9, 2024 06:47
github-merge-queue bot pushed a commit to model-checking/kani that referenced this pull request Dec 13, 2024
Upgrade toolchain to 12/12. The only substantive changes are for the
12/10 toolchain; 12/11 and 12/12 are just updating the LLBC tests.

Culprit PR: rust-lang/rust#133567 (specifically
[this
commit](rust-lang/rust@401dd84)
and [this
commit](rust-lang/rust@030545d)).

Resolves #3770

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 13, 2024
…l, r=oli-obk

Stop using driver queries in the public API

Follow up to rust-lang#132410 and rust-lang#133567

The next PR will completely get rid of driver queries. That PR will also contains some non-trivial refactorings enabled by no longer needing to support entering TyCtxt multiple times after it is constructed. The changes in the current PR have been split out to make it easier to review the api changes and to reduce the size of the next PR to review.

## Custom driver breaking change

The `after_crate_root_parsing` and `after_expansion` callbacks now accept `ast::Crate` and `TyCtxt` respectively rather than `Queries`. The only safe query in `Queries` to call inside these callbacks are `parse()` and `global_ctxt()` respectively which allows you to access the `ast::Crate` and `TyCtxt` either way. To fix your custom driver, replace the `queries: &'tcx Queries<'tcx>` argument with `crate_: ast::Crate` and `tcx: TyCtxt<'tcx>` respectively and for `after_expansion` remove your `queries.global_ctxt().unwrap().enter(|tcx| { ... })` call and only keep the contents of the closure.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 13, 2024
A bunch of cleanups (part 2)

Just like rust-lang#133567 these were all found while looking at the respective code, but are not blocking any other changes I want to make in the short term.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2024
Rollup merge of rust-lang#134130 - bjorn3:prepare_driver_query_removal, r=oli-obk

Stop using driver queries in the public API

Follow up to rust-lang#132410 and rust-lang#133567

The next PR will completely get rid of driver queries. That PR will also contains some non-trivial refactorings enabled by no longer needing to support entering TyCtxt multiple times after it is constructed. The changes in the current PR have been split out to make it easier to review the api changes and to reduce the size of the next PR to review.

## Custom driver breaking change

The `after_crate_root_parsing` and `after_expansion` callbacks now accept `ast::Crate` and `TyCtxt` respectively rather than `Queries`. The only safe query in `Queries` to call inside these callbacks are `parse()` and `global_ctxt()` respectively which allows you to access the `ast::Crate` and `TyCtxt` either way. To fix your custom driver, replace the `queries: &'tcx Queries<'tcx>` argument with `crate_: ast::Crate` and `tcx: TyCtxt<'tcx>` respectively and for `after_expansion` remove your `queries.global_ctxt().unwrap().enter(|tcx| { ... })` call and only keep the contents of the closure.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 14, 2024
A bunch of cleanups (part 2)

Just like rust-lang#133567 these were all found while looking at the respective code, but are not blocking any other changes I want to make in the short term.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 14, 2024
A bunch of cleanups (part 2)

Just like rust-lang#133567 these were all found while looking at the respective code, but are not blocking any other changes I want to make in the short term.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2024
Rollup merge of rust-lang#134251 - bjorn3:various_cleanups2, r=oli-obk

A bunch of cleanups (part 2)

Just like rust-lang#133567 these were all found while looking at the respective code, but are not blocking any other changes I want to make in the short term.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Dec 15, 2024
A bunch of cleanups (part 2)

Just like rust-lang/rust#133567 these were all found while looking at the respective code, but are not blocking any other changes I want to make in the short term.
smoelius added a commit to trailofbits/dylint that referenced this pull request Dec 15, 2024
smoelius added a commit to trailofbits/dylint that referenced this pull request Dec 15, 2024
smoelius added a commit to trailofbits/dylint that referenced this pull request Dec 15, 2024
flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 15, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#133567 (A bunch of cleanups)
 - rust-lang#133789 (Add doc alias 'then_with' for `then` method on `bool`)
 - rust-lang#133880 (Expand home_dir docs)
 - rust-lang#134036 (crash tests: use individual mir opts instead of mir-opt-level where easily possible)
 - rust-lang#134045 (Fix some triagebot mentions paths)
 - rust-lang#134046 (Remove ignored tests for hangs w/ new solver)
 - rust-lang#134050 (Miri subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
smoelius added a commit to trailofbits/dylint that referenced this pull request Dec 16, 2024
github-merge-queue bot pushed a commit to trailofbits/dylint that referenced this pull request Dec 16, 2024
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request Dec 18, 2024
A bunch of cleanups (part 2)

Just like rust-lang/rust#133567 these were all found while looking at the respective code, but are not blocking any other changes I want to make in the short term.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler 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.

8 participants