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

Add StableOrd trait as proposed in MCP 533. #105175

Merged
merged 2 commits into from
Dec 6, 2022

Conversation

michaelwoerister
Copy link
Member

The StableOrd trait can be used to mark types as having a stable sort order across compilation sessions. Collections that sort their items in a stable way can safely implement HashStable by hashing items in sort order.

See rust-lang/compiler-team#533 for more information.

The StableOrd trait can be used to mark types as having a stable
sort order across compilation sessions. Collections that sort their
items in a stable way can safely implement HashStable by
hashing items in sort order.
@rustbot
Copy link
Collaborator

rustbot commented Dec 2, 2022

r? @nagisa

(rustbot has picked a reviewer for you, use r? to override)

@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 Dec 2, 2022
@michaelwoerister
Copy link
Member Author

@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 Dec 2, 2022
@bors
Copy link
Contributor

bors commented Dec 2, 2022

⌛ Trying commit 3a58309 with merge e29ec0eb30603d64bdf1c7521883db1abae44331...

@Noratrieb
Copy link
Member

Why is the trait unsafe? Can it cause memory unsafety in the rustc process itself?

@michaelwoerister
Copy link
Member Author

I made it unsafe because the trait is giving a guarantee that the compiler has no way to check, either at compile time or via a runtime check and the consequences of getting it wrong are pretty severe. An error here can result in the kinds of miscompilations that we've seen in incremental compilation in the past.

@bors
Copy link
Contributor

bors commented Dec 2, 2022

☀️ Try build successful - checks-actions
Build commit: e29ec0eb30603d64bdf1c7521883db1abae44331 (e29ec0eb30603d64bdf1c7521883db1abae44331)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e29ec0eb30603d64bdf1c7521883db1abae44331): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

Max RSS (memory usage)

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)
-2.6% [-2.6%, -2.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.6% [-2.6%, -2.6%] 1

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)
-2.8% [-3.2%, -2.5%] 3
All ❌✅ (primary) - - 0

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 2, 2022
Copy link
Member

@nagisa nagisa 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

@@ -140,7 +140,7 @@ impl stable_hasher::StableHasherResult for Fingerprint {
}
}

impl_stable_hash_via_hash!(Fingerprint);
impl_stable_ord_and_stable_hash_via_hash!(Fingerprint);
Copy link
Member

Choose a reason for hiding this comment

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

Naming nit: are there any other Stable* traits? Could this be impl_stable_via_hash or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed the macro to impl_stable_traits_for_trivial_type!().

@michaelwoerister
Copy link
Member Author

Thanks for the review, @nagisa!

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Dec 5, 2022

📌 Commit 56aacb2 has been approved by nagisa

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 5, 2022
@bors
Copy link
Contributor

bors commented Dec 5, 2022

⌛ Testing commit 56aacb2 with merge 46f1f6f6123083f3ebf4544b45988e2c25cb9209...

@bors
Copy link
Contributor

bors commented Dec 5, 2022

💥 Test timed out

@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 Dec 5, 2022
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@michaelwoerister
Copy link
Member Author

Timeout while updating crates.io index.
@bors retry

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 6, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 6, 2022
@bors
Copy link
Contributor

bors commented Dec 6, 2022

⌛ Testing commit 56aacb2 with merge 9db224f...

@bors
Copy link
Contributor

bors commented Dec 6, 2022

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 9db224f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 6, 2022
@bors bors merged commit 9db224f into rust-lang:master Dec 6, 2022
@rustbot rustbot added this to the 1.67.0 milestone Dec 6, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9db224f): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.5% [1.5%, 1.5%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.3% [-0.3%, -0.3%] 3
All ❌✅ (primary) - - 0

Max RSS (memory usage)

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)
-5.4% [-5.4%, -5.4%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -5.4% [-5.4%, -5.4%] 1

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)
2.1% [2.1%, 2.1%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…t, r=nagisa

Add StableOrd trait as proposed in MCP 533.

The `StableOrd` trait can be used to mark types as having a stable sort order across compilation sessions. Collections that sort their items in a stable way can safely implement HashStable by hashing items in sort order.

See rust-lang/compiler-team#533 for more information.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 25, 2024
…elwoerister

Un-unsafe the `StableOrd` trait

Whilst incorrect implementations of this trait can cause miscompilation, they cannot cause memory unsafety in rustc.

[Discussed on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Policy.20of.20.60unsafe.60.20within.20the.20compiler).

cc [MCP 533](rust-lang/compiler-team#533), rust-lang#105175, `@michaelwoerister`

r? `@Nilstrieb`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 25, 2024
…woerister

Un-unsafe the `StableOrd` trait

Whilst incorrect implementations of this trait can cause miscompilation, they cannot cause memory unsafety in rustc.

[Discussed on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Policy.20of.20.60unsafe.60.20within.20the.20compiler).

cc [MCP 533](rust-lang/compiler-team#533), rust-lang#105175, `@michaelwoerister`

r? `@Nilstrieb`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

7 participants