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

Stop using ty::GenericPredicates for non-predicates_of queries #129725

Merged
merged 1 commit into from
Aug 31, 2024

Conversation

compiler-errors
Copy link
Member

GenericPredicates is a struct of several parts: A list of of an item's own predicates, and a parent def id (and some effects related stuff, but ignore that since it's kinda irrelevant). When instantiating these generic predicates, it calls predicates_of on the parent and instantiates its predicates, and appends the item's own instantiated predicates too:

if let Some(def_id) = self.parent {
tcx.predicates_of(def_id).instantiate_into(tcx, instantiated, args);
}
instantiated.predicates.extend(
self.predicates.iter().map(|(p, _)| EarlyBinder::bind(*p).instantiate(tcx, args)),
);
instantiated.spans.extend(self.predicates.iter().map(|(_, sp)| *sp));

Notice how this should result in a recursive set of calls to predicates_of... However, GenericPredicates is also misused by a bunch of other queries as a convenient way of passing around a list of predicates. For these queries, we don't ever set the parent def id of the GenericPredicates, but if we did, then this would be very easy to mistakenly call predicates_of instead of some other intended parent query.

Given that footgun, and the fact that we don't ever even use the parent def id in the GenericPredicates returned from queries like explicit_super_predicates_of, It really has no benefit over just returning &'tcx [(Clause<'tcx>, Span)].

This PR additionally opts to wrap the results of EarlyBinder, as we've tended to use that in the return type of these kinds of queries to properly convey that the user has params to deal with, and it also gives a convenient way of iterating over a slice of things after instantiating.

@rustbot
Copy link
Collaborator

rustbot commented Aug 29, 2024

r? @nnethercote

rustbot has assigned @nnethercote.
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 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 Aug 29, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 29, 2024

HIR ty lowering was modified

cc @fmease

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@compiler-errors
Copy link
Member Author

r? @fmease or reassign :)

@rustbot rustbot assigned fmease and unassigned nnethercote Aug 29, 2024
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

r=me with comment addressed in one way or another

compiler/rustc_metadata/src/rmeta/mod.rs Show resolved Hide resolved
@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 29, 2024
@compiler-errors
Copy link
Member Author

I'm not certain that I understand your idea of encoding EarlyBinder<&'tcx [(Clause<'tcx>, Span)]> as LazyArray<EarlyBinder<(Clause<'tcx>, Span)>> -- I wouldn't want to change the query result to be &'tcx [EarlyBinder<(Clause<'tcx>, Span)>] since that's more awkward to work with, and it seems like it would require an annoying amount of shunting between EarlyBinders on the elements and on the whole slice. It would also be an objectively worse encoding if we ever tracked the real bound vars on the EarlyBinder.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 29, 2024
@fmease
Copy link
Member

fmease commented Aug 29, 2024

Thanks for checking and sorry for the confusion.

@bors r+ rollup (no behavioral change)

@bors
Copy link
Contributor

bors commented Aug 29, 2024

📌 Commit 9200452 has been approved by fmease

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 Aug 29, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Aug 30, 2024
…mease

Stop using `ty::GenericPredicates` for non-predicates_of queries

`GenericPredicates` is a struct of several parts: A list of of an item's own predicates, and a parent def id (and some effects related stuff, but ignore that since it's kinda irrelevant). When instantiating these generic predicates, it calls `predicates_of` on the parent and instantiates its predicates, and appends the item's own instantiated predicates too:

https://github.com/rust-lang/rust/blob/acb4e8b6251f1d8da36f08e7a70fa23fc581839e/compiler/rustc_middle/src/ty/generics.rs#L407-L413

Notice how this should result in a recursive set of calls to `predicates_of`... However, `GenericPredicates` is *also* misused by a bunch of *other* queries as a convenient way of passing around a list of predicates. For these queries, we don't ever set the parent def id of the `GenericPredicates`, but if we did, then this would be very easy to mistakenly call `predicates_of` instead of some other intended parent query.

Given that footgun, and the fact that we don't ever even *use* the parent def id in the `GenericPredicates` returned from queries like `explicit_super_predicates_of`, It really has no benefit over just returning `&'tcx [(Clause<'tcx>, Span)]`.

This PR additionally opts to wrap the results of `EarlyBinder`, as we've tended to use that in the return type of these kinds of queries to properly convey that the user has params to deal with, and it also gives a convenient way of iterating over a slice of things after instantiating.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 30, 2024
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#120221 (Don't make statement nonterminals match pattern nonterminals)
 - rust-lang#127897 (add `aarch64_unknown_nto_qnx700` target - QNX 7.0 support for aarch64le)
 - rust-lang#129123 (rustdoc-json: Add test for `Self` type)
 - rust-lang#129642 (Bump backtrace to 0.3.74~ish)
 - rust-lang#129675 (allow BufReader::peek to be called on unsized types)
 - rust-lang#129723 (Simplify some extern providers)
 - rust-lang#129724 (Remove `Option<!>` return types.)
 - rust-lang#129725 (Stop using `ty::GenericPredicates` for non-predicates_of queries)
 - rust-lang#129733 (Subtree update of `rust-analyzer`)
 - rust-lang#129754 (wasi: Fix sleeping for `Duration::MAX`)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Aug 30, 2024
…mease

Stop using `ty::GenericPredicates` for non-predicates_of queries

`GenericPredicates` is a struct of several parts: A list of of an item's own predicates, and a parent def id (and some effects related stuff, but ignore that since it's kinda irrelevant). When instantiating these generic predicates, it calls `predicates_of` on the parent and instantiates its predicates, and appends the item's own instantiated predicates too:

https://github.com/rust-lang/rust/blob/acb4e8b6251f1d8da36f08e7a70fa23fc581839e/compiler/rustc_middle/src/ty/generics.rs#L407-L413

Notice how this should result in a recursive set of calls to `predicates_of`... However, `GenericPredicates` is *also* misused by a bunch of *other* queries as a convenient way of passing around a list of predicates. For these queries, we don't ever set the parent def id of the `GenericPredicates`, but if we did, then this would be very easy to mistakenly call `predicates_of` instead of some other intended parent query.

Given that footgun, and the fact that we don't ever even *use* the parent def id in the `GenericPredicates` returned from queries like `explicit_super_predicates_of`, It really has no benefit over just returning `&'tcx [(Clause<'tcx>, Span)]`.

This PR additionally opts to wrap the results of `EarlyBinder`, as we've tended to use that in the return type of these kinds of queries to properly convey that the user has params to deal with, and it also gives a convenient way of iterating over a slice of things after instantiating.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 30, 2024
…kingjubilee

Rollup of 9 pull requests

Successful merges:

 - rust-lang#120221 (Don't make statement nonterminals match pattern nonterminals)
 - rust-lang#129123 (rustdoc-json: Add test for `Self` type)
 - rust-lang#129642 (Bump backtrace to 0.3.74~ish)
 - rust-lang#129675 (allow BufReader::peek to be called on unsized types)
 - rust-lang#129723 (Simplify some extern providers)
 - rust-lang#129724 (Remove `Option<!>` return types.)
 - rust-lang#129725 (Stop using `ty::GenericPredicates` for non-predicates_of queries)
 - rust-lang#129733 (Subtree update of `rust-analyzer`)
 - rust-lang#129751 (interpret/visitor: make memory order iteration slightly more efficient)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 30, 2024
…kingjubilee

Rollup of 9 pull requests

Successful merges:

 - rust-lang#120221 (Don't make statement nonterminals match pattern nonterminals)
 - rust-lang#129123 (rustdoc-json: Add test for `Self` type)
 - rust-lang#129642 (Bump backtrace to 0.3.74~ish)
 - rust-lang#129675 (allow BufReader::peek to be called on unsized types)
 - rust-lang#129723 (Simplify some extern providers)
 - rust-lang#129724 (Remove `Option<!>` return types.)
 - rust-lang#129725 (Stop using `ty::GenericPredicates` for non-predicates_of queries)
 - rust-lang#129733 (Subtree update of `rust-analyzer`)
 - rust-lang#129751 (interpret/visitor: make memory order iteration slightly more efficient)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 30, 2024
…kingjubilee

Rollup of 9 pull requests

Successful merges:

 - rust-lang#120221 (Don't make statement nonterminals match pattern nonterminals)
 - rust-lang#129123 (rustdoc-json: Add test for `Self` type)
 - rust-lang#129642 (Bump backtrace to 0.3.74~ish)
 - rust-lang#129675 (allow BufReader::peek to be called on unsized types)
 - rust-lang#129723 (Simplify some extern providers)
 - rust-lang#129724 (Remove `Option<!>` return types.)
 - rust-lang#129725 (Stop using `ty::GenericPredicates` for non-predicates_of queries)
 - rust-lang#129733 (Subtree update of `rust-analyzer`)
 - rust-lang#129751 (interpret/visitor: make memory order iteration slightly more efficient)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Aug 31, 2024
…mease

Stop using `ty::GenericPredicates` for non-predicates_of queries

`GenericPredicates` is a struct of several parts: A list of of an item's own predicates, and a parent def id (and some effects related stuff, but ignore that since it's kinda irrelevant). When instantiating these generic predicates, it calls `predicates_of` on the parent and instantiates its predicates, and appends the item's own instantiated predicates too:

https://github.com/rust-lang/rust/blob/acb4e8b6251f1d8da36f08e7a70fa23fc581839e/compiler/rustc_middle/src/ty/generics.rs#L407-L413

Notice how this should result in a recursive set of calls to `predicates_of`... However, `GenericPredicates` is *also* misused by a bunch of *other* queries as a convenient way of passing around a list of predicates. For these queries, we don't ever set the parent def id of the `GenericPredicates`, but if we did, then this would be very easy to mistakenly call `predicates_of` instead of some other intended parent query.

Given that footgun, and the fact that we don't ever even *use* the parent def id in the `GenericPredicates` returned from queries like `explicit_super_predicates_of`, It really has no benefit over just returning `&'tcx [(Clause<'tcx>, Span)]`.

This PR additionally opts to wrap the results of `EarlyBinder`, as we've tended to use that in the return type of these kinds of queries to properly convey that the user has params to deal with, and it also gives a convenient way of iterating over a slice of things after instantiating.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 31, 2024
…kingjubilee

Rollup of 16 pull requests

Successful merges:

 - rust-lang#129123 (rustdoc-json: Add test for `Self` type)
 - rust-lang#129675 (allow BufReader::peek to be called on unsized types)
 - rust-lang#129678 (Deny imports of `rustc_type_ir::inherent` outside of type ir + new trait solver)
 - rust-lang#129723 (Simplify some extern providers)
 - rust-lang#129724 (Remove `Option<!>` return types.)
 - rust-lang#129725 (Stop using `ty::GenericPredicates` for non-predicates_of queries)
 - rust-lang#129730 (f32 docs: define 'arithmetic' operations)
 - rust-lang#129749 (llvm-wrapper: adapt for LLVM API changes)
 - rust-lang#129757 (Add a test for trait solver overflow in MIR inliner cycle detection)
 - rust-lang#129760 (Make the "detect-old-time" UI test more representative)
 - rust-lang#129762 (Update the `wasm-component-ld` binary dependency)
 - rust-lang#129767 (Remove `#[macro_use] extern crate tracing`, round 4)
 - rust-lang#129774 (Remove `#[macro_use] extern crate tracing` from rustdoc and rustfmt)
 - rust-lang#129780 (add crashtests for several old unfixed ICEs)
 - rust-lang#129782 (couple more crash tests)
 - rust-lang#129791 (mark joboet as on vacation)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Aug 31, 2024
…mease

Stop using `ty::GenericPredicates` for non-predicates_of queries

`GenericPredicates` is a struct of several parts: A list of of an item's own predicates, and a parent def id (and some effects related stuff, but ignore that since it's kinda irrelevant). When instantiating these generic predicates, it calls `predicates_of` on the parent and instantiates its predicates, and appends the item's own instantiated predicates too:

https://github.com/rust-lang/rust/blob/acb4e8b6251f1d8da36f08e7a70fa23fc581839e/compiler/rustc_middle/src/ty/generics.rs#L407-L413

Notice how this should result in a recursive set of calls to `predicates_of`... However, `GenericPredicates` is *also* misused by a bunch of *other* queries as a convenient way of passing around a list of predicates. For these queries, we don't ever set the parent def id of the `GenericPredicates`, but if we did, then this would be very easy to mistakenly call `predicates_of` instead of some other intended parent query.

Given that footgun, and the fact that we don't ever even *use* the parent def id in the `GenericPredicates` returned from queries like `explicit_super_predicates_of`, It really has no benefit over just returning `&'tcx [(Clause<'tcx>, Span)]`.

This PR additionally opts to wrap the results of `EarlyBinder`, as we've tended to use that in the return type of these kinds of queries to properly convey that the user has params to deal with, and it also gives a convenient way of iterating over a slice of things after instantiating.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 31, 2024
…mease

Stop using `ty::GenericPredicates` for non-predicates_of queries

`GenericPredicates` is a struct of several parts: A list of of an item's own predicates, and a parent def id (and some effects related stuff, but ignore that since it's kinda irrelevant). When instantiating these generic predicates, it calls `predicates_of` on the parent and instantiates its predicates, and appends the item's own instantiated predicates too:

https://github.com/rust-lang/rust/blob/acb4e8b6251f1d8da36f08e7a70fa23fc581839e/compiler/rustc_middle/src/ty/generics.rs#L407-L413

Notice how this should result in a recursive set of calls to `predicates_of`... However, `GenericPredicates` is *also* misused by a bunch of *other* queries as a convenient way of passing around a list of predicates. For these queries, we don't ever set the parent def id of the `GenericPredicates`, but if we did, then this would be very easy to mistakenly call `predicates_of` instead of some other intended parent query.

Given that footgun, and the fact that we don't ever even *use* the parent def id in the `GenericPredicates` returned from queries like `explicit_super_predicates_of`, It really has no benefit over just returning `&'tcx [(Clause<'tcx>, Span)]`.

This PR additionally opts to wrap the results of `EarlyBinder`, as we've tended to use that in the return type of these kinds of queries to properly convey that the user has params to deal with, and it also gives a convenient way of iterating over a slice of things after instantiating.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 31, 2024
…iaskrgr

Rollup of 15 pull requests

Successful merges:

 - rust-lang#120221 (Don't make statement nonterminals match pattern nonterminals)
 - rust-lang#126183 (Separate core search logic with search ui)
 - rust-lang#129123 (rustdoc-json: Add test for `Self` type)
 - rust-lang#129366 (linker: Synchronize native library search in rustc and linker)
 - rust-lang#129527 (Don't use `TyKind` in a lint)
 - rust-lang#129534 (Deny `wasm_c_abi` lint to nudge the last 25%)
 - rust-lang#129640 (Re-enable android tests/benches in alloc/core)
 - rust-lang#129642 (Bump backtrace to 0.3.74~ish)
 - rust-lang#129675 (allow BufReader::peek to be called on unsized types)
 - rust-lang#129723 (Simplify some extern providers)
 - rust-lang#129724 (Remove `Option<!>` return types.)
 - rust-lang#129725 (Stop using `ty::GenericPredicates` for non-predicates_of queries)
 - rust-lang#129731 (Allow running `./x.py test compiler`)
 - rust-lang#129751 (interpret/visitor: make memory order iteration slightly more efficient)
 - rust-lang#129754 (wasi: Fix sleeping for `Duration::MAX`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5f10a99 into rust-lang:master Aug 31, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 31, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 31, 2024
Rollup merge of rust-lang#129725 - compiler-errors:predicates-of, r=fmease

Stop using `ty::GenericPredicates` for non-predicates_of queries

`GenericPredicates` is a struct of several parts: A list of of an item's own predicates, and a parent def id (and some effects related stuff, but ignore that since it's kinda irrelevant). When instantiating these generic predicates, it calls `predicates_of` on the parent and instantiates its predicates, and appends the item's own instantiated predicates too:

https://github.com/rust-lang/rust/blob/acb4e8b6251f1d8da36f08e7a70fa23fc581839e/compiler/rustc_middle/src/ty/generics.rs#L407-L413

Notice how this should result in a recursive set of calls to `predicates_of`... However, `GenericPredicates` is *also* misused by a bunch of *other* queries as a convenient way of passing around a list of predicates. For these queries, we don't ever set the parent def id of the `GenericPredicates`, but if we did, then this would be very easy to mistakenly call `predicates_of` instead of some other intended parent query.

Given that footgun, and the fact that we don't ever even *use* the parent def id in the `GenericPredicates` returned from queries like `explicit_super_predicates_of`, It really has no benefit over just returning `&'tcx [(Clause<'tcx>, Span)]`.

This PR additionally opts to wrap the results of `EarlyBinder`, as we've tended to use that in the return type of these kinds of queries to properly convey that the user has params to deal with, and it also gives a convenient way of iterating over a slice of things after instantiating.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants