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

Make decoding non-optional LazyArray panic if not set #129829

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Aug 31, 2024

Tables may be defined as optional: or defaulted:. If optional, if we try to read a value from a key that was never encoded, we should panic. This has high value in ensuring correctness over a defaulted table, so the tradeoff is worth considering, since it signals the compiler has a buggy encode impl, rather than just defaulting to a value.

HOWEVER, optional: arrays were side-stepping this. So this PR fixes that, and makes optional: tables of LazyArray act like LazyValue, and panic if it's not assigned a value during encoding.

During this PR, I found that deduced_param_attrs has buggy (?? i think??) implementation where it will refuse to encode cross-crate deduced_param_attrs unless we're codegening, we're optimizing the library, and incremental is disabled. This seems incredibly wrong, but I don't want to fix it in this PR.

// Encode all the deduced parameter attributes for everything that has MIR, even for items
// that can't be inlined. But don't if we aren't optimizing in non-incremental mode, to
// save the query traffic.
if tcx.sess.opts.output_types.should_codegen()
&& tcx.sess.opts.optimize != OptLevel::No
&& tcx.sess.opts.incremental.is_none()
{
for &local_def_id in tcx.mir_keys(()) {
if let DefKind::AssocFn | DefKind::Fn = tcx.def_kind(local_def_id) {
record_array!(self.tables.deduced_param_attrs[local_def_id.to_def_id()] <-
self.tcx.deduced_param_attrs(local_def_id.to_def_id()));
}
}
}
}

@rustbot
Copy link
Collaborator

rustbot commented Aug 31, 2024

r? @lcnr

rustbot has assigned @lcnr.
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. labels Aug 31, 2024
@lcnr
Copy link
Contributor

lcnr commented Sep 2, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 2, 2024

📌 Commit 175238b has been approved by lcnr

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 Sep 2, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 2, 2024
…l, r=lcnr

Make decoding non-optional `LazyArray` panic if not set

Tables may be [defined](https://github.com/rust-lang/rust/blob/9649706eada1b2c68cf6504356efb058f68ad739/compiler/rustc_metadata/src/rmeta/mod.rs#L377) as `optional:` or `defaulted:`. If optional, if we try to read a value from a key that was never encoded, we should panic. This has high value in ensuring correctness over a defaulted table, so the tradeoff is worth considering, since it signals the compiler has a buggy encode impl, rather than just defaulting to a value.

HOWEVER, `optional:` arrays were side-stepping this. So this PR fixes that, and makes `optional:` tables of `LazyArray` act like `LazyValue`, and panic if it's not assigned a value during encoding.

During this PR, I found that `deduced_param_attrs` has buggy (?? i think??) implementation where it will refuse to encode cross-crate `deduced_param_attrs` unless we're codegening, we're optimizing the library, and incremental is disabled. This seems incredibly wrong, but I don't want to fix it in this PR.
https://github.com/rust-lang/rust/blob/9649706eada1b2c68cf6504356efb058f68ad739/compiler/rustc_metadata/src/rmeta/encoder.rs#L1733-L1747
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 2, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#129152 (custom/external clippy support for bootstrapping)
 - rust-lang#129529 (Add test to build crates used by r-a on stable)
 - rust-lang#129829 (Make decoding non-optional `LazyArray` panic if not set)
 - rust-lang#129860 (update `object` dependency to remove duplicate `wasmparser`)
 - rust-lang#129875 (chore: Fix typos in 'compiler' (batch 1))
 - rust-lang#129877 (chore: Fix typos in 'compiler' (batch 2))
 - rust-lang#129878 (chore: Fix typos in 'compiler' (batch 3))
 - rust-lang#129891 (Do not request sanitizers for naked functions)
 - rust-lang#129892 (Clarify language around ptrs in slice::raw)

Failed merges:

 - rust-lang#129777 (Add `unreachable_pub`, round 4)
 - rust-lang#129868 (Remove kobzol vacation status)

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

Rollup of 12 pull requests

Successful merges:

 - rust-lang#129748 (Box validity: update for new zero-sized rules)
 - rust-lang#129829 (Make decoding non-optional `LazyArray` panic if not set)
 - rust-lang#129856 (compiler_fence documentation: emphasize synchronization, not reordering)
 - rust-lang#129868 (Remove kobzol vacation status)
 - rust-lang#129875 (chore: Fix typos in 'compiler' (batch 1))
 - rust-lang#129877 (chore: Fix typos in 'compiler' (batch 2))
 - rust-lang#129878 (chore: Fix typos in 'compiler' (batch 3))
 - rust-lang#129890 (Remove stray word in a comment)
 - rust-lang#129892 (Clarify language around ptrs in slice::raw)
 - rust-lang#129905 (mailmap: add new email for davidtwco)
 - rust-lang#129906 (mailmapper?)
 - rust-lang#129907 (Fix compile error in solid's remove_dir_all)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c6410f5 into rust-lang:master Sep 3, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2024
Rollup merge of rust-lang#129829 - compiler-errors:decode-non-optional, r=lcnr

Make decoding non-optional `LazyArray` panic if not set

Tables may be [defined](https://github.com/rust-lang/rust/blob/9649706eada1b2c68cf6504356efb058f68ad739/compiler/rustc_metadata/src/rmeta/mod.rs#L377) as `optional:` or `defaulted:`. If optional, if we try to read a value from a key that was never encoded, we should panic. This has high value in ensuring correctness over a defaulted table, so the tradeoff is worth considering, since it signals the compiler has a buggy encode impl, rather than just defaulting to a value.

HOWEVER, `optional:` arrays were side-stepping this. So this PR fixes that, and makes `optional:` tables of `LazyArray` act like `LazyValue`, and panic if it's not assigned a value during encoding.

During this PR, I found that `deduced_param_attrs` has buggy (?? i think??) implementation where it will refuse to encode cross-crate `deduced_param_attrs` unless we're codegening, we're optimizing the library, and incremental is disabled. This seems incredibly wrong, but I don't want to fix it in this PR.
https://github.com/rust-lang/rust/blob/9649706eada1b2c68cf6504356efb058f68ad739/compiler/rustc_metadata/src/rmeta/encoder.rs#L1733-L1747
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
None yet
Development

Successfully merging this pull request may close these issues.

4 participants