Make some `newtype_index!` derived impls opt-in instead of opt-out #118125

nnethercote · 2023-11-21T08:06:04Z

Opt-in is the standard Rust way of doing things, and avoids some unnecessary dependencies on the rustc_serialize crate.

r? @lcnr

rustbot · 2023-11-21T08:06:14Z

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

nnethercote · 2023-11-21T08:06:17Z

Best reviewed one commit at a time.

rustbot · 2023-11-21T09:05:22Z

Could not assign reviewer from: lcnr.
User(s) lcnr are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

nnethercote · 2023-11-21T09:06:07Z

r? @compiler-errors

bors · 2023-11-21T14:08:24Z

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

compiler-errors

r=me after rebased

nnethercote · 2023-11-21T21:31:15Z

I rebased.

@bors r=compiler-errors

bors · 2023-11-21T21:31:18Z

📌 Commit e55e28b has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-11-22T06:06:20Z

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

By default, `newtype_index!` types get a default `Encodable`/`Decodable` impl. You can opt out of this with `custom_encodable`. Opting out is the opposite to how Rust normally works with autogenerated (derived) impls. This commit inverts the behaviour, replacing `custom_encodable` with `encodable` which opts into the default `Encodable`/`Decodable` impl. Only 23 of the 59 `newtype_index!` occurrences need `encodable`. Even better, there were eight crates with a dependency on `rustc_serialize` just from unused default `Encodable`/`Decodable` impls. This commit removes that dependency from those eight crates.

Similar to the previous commit, this replaces `newtype_index`'s opt-out `no_ord_impl` attribute with the opt-in `orderable` attribute.

nnethercote · 2023-11-22T07:46:44Z

I rebased again.

@bors r=compiler-errors

bors · 2023-11-22T07:46:47Z

📌 Commit 0991374 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-11-22T09:29:52Z

⌛ Testing commit 0991374 with merge 5a9e0e8...

bors · 2023-11-22T11:43:49Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 5a9e0e8 to master...

rust-timer · 2023-11-22T22:22:48Z

Finished benchmarking commit (5a9e0e8): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-8.2%	[-20.2%, -2.0%]	6
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 677.73s -> 676.065s (-0.25%)
Artifact size: 313.77 MiB -> 313.79 MiB (0.01%)

klensy · 2023-11-26T10:34:56Z

compiler/rustc_index_macros/src/lib.rs

+/// - #[encodable]: derives `Encodable`/`Decodable`.
+/// - #[orderable]: derives `PartialOrd`/`Ord`, plus step-related methods.
+/// - #[debug_format = "Foo({})"]: derives `Debug` with particular output.
+/// - #[max = 0xFFFF_FFFD]: specifies the max value, which allows niche
+///   optimizations. The default max value is 0xFFFF_FF00.
+/// - #[gate_rustc_only]: makes parts of the generated code nightly-only.


https://github.com/rust-lang-ci/rust/actions/runs/6990882559/job/19020846394#step:26:27071

warning: unresolved link to `encodable` --> compiler/rustc_index_macros/src/lib.rs:28:9 | 28 | /// - #[encodable]: derives `Encodable`/`Decodable`. | ^^^^^^^^^ no item named `encodable` in scope | = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]` = note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default warning: unresolved link to `orderable` --> compiler/rustc_index_macros/src/lib.rs:29:9 | 29 | /// - #[orderable]: derives `PartialOrd`/`Ord`, plus step-related methods. | ^^^^^^^^^ no item named `orderable` in scope | = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]` warning: unresolved link to `gate_rustc_only` --> compiler/rustc_index_macros/src/lib.rs:33:9 | 33 | /// - #[gate_rustc_only]: makes parts of the generated code nightly-only. | ^^^^^^^^^^^^^^^ no item named `gate_rustc_only` in scope | = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`

Should be fixed by #118338.

How did you notice this? I never notice warnings buried deep within CI runs like this...

Reading logs from time to time. It's nice to have github action to automate log diffing and grepping to notice interesting stuff.

rustbot assigned lcnr Nov 21, 2023

This comment was marked as resolved.

Sign in to view

nnethercote force-pushed the custom_encodable branch from 93cc392 to 531f4c2 Compare November 21, 2023 09:05

nnethercote changed the title ~~Make some newtype_index! derived impls opt-out instead of opt-in~~ Make some newtype_index! derived impls opt-in instead of opt-out Nov 21, 2023

rustbot assigned compiler-errors and unassigned lcnr Nov 21, 2023

compiler-errors approved these changes Nov 21, 2023

View reviewed changes

compiler-errors 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 Nov 21, 2023

nnethercote force-pushed the custom_encodable branch from 531f4c2 to e55e28b Compare November 21, 2023 21:24

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 Nov 21, 2023

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 22, 2023

nnethercote added 3 commits November 22, 2023 18:37

Replace no_ord_impl with orderable.

7060fc8

Similar to the previous commit, this replaces `newtype_index`'s opt-out `no_ord_impl` attribute with the opt-in `orderable` attribute.

Document newtype_index attributes.

0991374

nnethercote force-pushed the custom_encodable branch from e55e28b to 0991374 Compare November 22, 2023 07:46

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 Nov 22, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 22, 2023

bors merged commit 5a9e0e8 into rust-lang:master Nov 22, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Nov 22, 2023

nnethercote deleted the custom_encodable branch November 22, 2023 20:25

klensy reviewed Nov 26, 2023

View reviewed changes

nnethercote mentioned this pull request Nov 26, 2023

Backticks fixes #118338

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make some `newtype_index!` derived impls opt-in instead of opt-out #118125

Make some `newtype_index!` derived impls opt-in instead of opt-out #118125

nnethercote commented Nov 21, 2023

rustbot commented Nov 21, 2023

nnethercote commented Nov 21, 2023

This comment was marked as resolved.

rustbot commented Nov 21, 2023

nnethercote commented Nov 21, 2023

bors commented Nov 21, 2023

compiler-errors left a comment

nnethercote commented Nov 21, 2023

bors commented Nov 21, 2023

bors commented Nov 22, 2023

nnethercote commented Nov 22, 2023

bors commented Nov 22, 2023

bors commented Nov 22, 2023

bors commented Nov 22, 2023

rust-timer commented Nov 22, 2023

klensy Nov 26, 2023

nnethercote Nov 26, 2023

klensy Nov 27, 2023

Make some newtype_index! derived impls opt-in instead of opt-out #118125

Make some newtype_index! derived impls opt-in instead of opt-out #118125

Conversation

nnethercote commented Nov 21, 2023

rustbot commented Nov 21, 2023

nnethercote commented Nov 21, 2023

This comment was marked as resolved.

rustbot commented Nov 21, 2023

nnethercote commented Nov 21, 2023

bors commented Nov 21, 2023

compiler-errors left a comment

Choose a reason for hiding this comment

nnethercote commented Nov 21, 2023

bors commented Nov 21, 2023

bors commented Nov 22, 2023

nnethercote commented Nov 22, 2023

bors commented Nov 22, 2023

bors commented Nov 22, 2023

bors commented Nov 22, 2023

rust-timer commented Nov 22, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

klensy Nov 26, 2023

Choose a reason for hiding this comment

nnethercote Nov 26, 2023

Choose a reason for hiding this comment

klensy Nov 27, 2023

Choose a reason for hiding this comment

Make some `newtype_index!` derived impls opt-in instead of opt-out #118125

Make some `newtype_index!` derived impls opt-in instead of opt-out #118125