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

Remove feed_local_def_id #121089

Merged
merged 9 commits into from
Mar 8, 2024
Merged

Remove feed_local_def_id #121089

merged 9 commits into from
Mar 8, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 14, 2024

best reviewed commit by commit

Basically I returned TyCtxtFeed from create_def and then preserved that in the local caches

based on #121084

r? @petrochenkov

@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. labels Feb 14, 2024
@oli-obk

This comment was marked as resolved.

@bors

This comment was marked as resolved.

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 18, 2024

📌 Commit a640db5 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 Feb 18, 2024
@petrochenkov
Copy link
Contributor

I don't understand what we get from this, to be honest.
What are the additional guarantees from keeping Feeds in node_id_to_def_id tables?

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 18, 2024

What are the additional guarantees from keeping Feeds in node_id_to_def_id tables?

that you don't feed from another query but the one that created the DefId. Feed cannot be transferred across query boundaries.

feed_local_def_id is a massive footgun and if anone uses it from anywhere but the resolver it will cause undebuggable incremental ICEs. The reason the resolver is fine is that it is (like the analysis query, an outermost (not called from another query) eval_always query.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2024
Remove `feed_local_def_id`

best reviewed commit by commit

Basically I returned `TyCtxtFeed` from `create_def` and then preserved that in the local caches

based on rust-lang#121084

r? `@petrochenkov`
@bors
Copy link
Contributor

bors commented Feb 19, 2024

⌛ Testing commit a640db5 with merge 5bffa10...

@petrochenkov
Copy link
Contributor

What are the additional guarantees from keeping Feeds in node_id_to_def_id tables?

that you don't feed from another query but the one that created the DefId. Feed cannot be transferred across query boundaries.

feed_local_def_id is a massive footgun and if anone uses it from anywhere but the resolver it will cause undebuggable incremental ICEs. The reason the resolver is fine is that it is (like the analysis query, an outermost (not called from another query) eval_always query.

I probably makes sense to put this explanation as a comment on struct Feed.

@petrochenkov
Copy link
Contributor

Feed cannot be transferred across query boundaries.

I'm not sure if it's a hard guarantee, or something that we need to keep in mind when writing new code.

For example, what happens if we put Feed into ResolverOutputs?
Then it will be usable from other queries through tcx.resolutions(()).

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as resolved.

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 19, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 19, 2024

I'm not sure if it's a hard guarantee

I'll add a negative impl for the relevant traits

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 19, 2024

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 19, 2024
@bors
Copy link
Contributor

bors commented Feb 19, 2024

⌛ Trying commit a0aeca0 with merge 9b35aff...

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2024
Remove `feed_local_def_id`

best reviewed commit by commit

Basically I returned `TyCtxtFeed` from `create_def` and then preserved that in the local caches

based on rust-lang#121084

r? `@petrochenkov`
@oli-obk oli-obk force-pushed the create_def_feed branch 2 times, most recently from f835818 to 7361780 Compare March 5, 2024 09:01
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 5, 2024

@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 Mar 5, 2024
@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

r=me after updating the failing test.
@rustbot author

@rustbot rustbot 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 Mar 5, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 5, 2024

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Mar 5, 2024

📌 Commit ef00fae has been approved by petrochenkov

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 5, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 6, 2024
…enkov

Remove `feed_local_def_id`

best reviewed commit by commit

Basically I returned `TyCtxtFeed` from `create_def` and then preserved that in the local caches

based on rust-lang#121084

r? `@petrochenkov`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 6, 2024
…enkov

Remove `feed_local_def_id`

best reviewed commit by commit

Basically I returned `TyCtxtFeed` from `create_def` and then preserved that in the local caches

based on rust-lang#121084

r? ``@petrochenkov``
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 6, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#113518 (bootstrap/libtest: print test name eagerly on failure even with `verbose-tests=false` / `--quiet`)
 - rust-lang#119888 (Stabilize the `#[diagnostic]` namespace and `#[diagnostic::on_unimplemented]` attribute)
 - rust-lang#121089 (Remove `feed_local_def_id`)
 - rust-lang#121926 (`f16` and `f128` step 3: compiler support & feature gate)
 - rust-lang#121959 (Removing absolute path in proc-macro)
 - rust-lang#122015 (Add better explanation for `rustc_index::IndexVec`)
 - rust-lang#122027 (Uplift some feeding out of `associated_type_for_impl_trait_in_impl` and into queries)
 - rust-lang#122038 (Fix linting paths with qself in `unused_qualifications`)

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

Remove `feed_local_def_id`

best reviewed commit by commit

Basically I returned `TyCtxtFeed` from `create_def` and then preserved that in the local caches

based on rust-lang#121084

r? ```@petrochenkov```
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2024
…kingjubilee

Rollup of 13 pull requests

Successful merges:

 - rust-lang#113525 (Dynamically size sigaltstk in std)
 - rust-lang#114009 (compiler: allow transmute of ZST arrays with generics)
 - rust-lang#116793 (Allow targets to override default codegen backend)
 - rust-lang#118623 (Improve std::fs::read_to_string example)
 - rust-lang#120504 (Vec::try_with_capacity)
 - rust-lang#121089 (Remove `feed_local_def_id`)
 - rust-lang#121280 (Implement MaybeUninit::fill{,_with,_from})
 - rust-lang#122087 (Add missing background color for top-level rust documentation page and increase contrast by setting text color to black)
 - rust-lang#122104 (Rust is a proper name: rust → Rust)
 - rust-lang#122110 (Make `x t miri` respect `MIRI_TEMP`)
 - rust-lang#122114 (Make not finding core a fatal error)
 - rust-lang#122115 (Cancel parsing ever made during recovery)
 - rust-lang#122126 (Fix `tidy --bless` on  ̶X̶e̶n̶i̶x̶ Windows)

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

Rollup of 10 pull requests

Successful merges:

 - rust-lang#119888 (Stabilize the `#[diagnostic]` namespace and `#[diagnostic::on_unimplemented]` attribute)
 - rust-lang#121089 (Remove `feed_local_def_id`)
 - rust-lang#122004 (AST validation: Improve handling of inherent impls nested within functions and anon consts)
 - rust-lang#122087 (Add missing background color for top-level rust documentation page and increase contrast by setting text color to black)
 - rust-lang#122136 (Include all library files in artifact summary on CI)
 - rust-lang#122137 (Don't pass a break scope to `Builder::break_for_else`)
 - rust-lang#122138 (Record mtime in bootstrap's LLVM linker script)
 - rust-lang#122141 (sync (try_)instantiate_mir_and_normalize_erasing_regions implementation)
 - rust-lang#122142 (cleanup rustc_infer)
 - rust-lang#122147 (Make `std::os::unix::ucred` module private)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4de78d2 into rust-lang:master Mar 8, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
Rollup merge of rust-lang#121089 - oli-obk:create_def_feed, r=petrochenkov

Remove `feed_local_def_id`

best reviewed commit by commit

Basically I returned `TyCtxtFeed` from `create_def` and then preserved that in the local caches

based on rust-lang#121084

r? ````@petrochenkov````
@oli-obk oli-obk deleted the create_def_feed branch March 8, 2024 05:51
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.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants