Un-unsafe the StableOrd trait

[eggyal]

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

Discussed on Zulip.

cc MCP 533, #105175, @michaelwoerister

r? @Nilstrieb

[michaelwoerister]

Yes, I agree that this shouldn't be unsafe. Implementing the trait has nothing to do with language invariants. @Nilstrieb was right all along 🙂

It would be nice though if we had some kind of #[rustc::triple_check_every_impl_of_this_trait] and #[rustc::this_impl_has_been_triple_checked] functionality.

[eggyal]

Perhaps just add an associated THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED constant to the trait?

[eggyal]

However none of these approaches prevent the type later being modified to then have fields that break the invariant.

Perhaps a more robust approach would be to define it as an auto-trait that is unimplemented for types that are unstable across compilations?

[Urgau]

It would be nice though if we had some kind of #[rustc::triple_check_every_impl_of_this_trait] and #[rustc::this_impl_has_been_triple_checked] functionality.

This feels very similar to the proposed the clippy::dangerous attribute. I wish it were already merged and usable.

[michaelwoerister]

[rustc::this_impl_has_been_triple_checked_for_transitive_hash_of_all_reachable_code(0x124159831948129481821948)] 😉

[michaelwoerister]

Perhaps just add an associated THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED constant to the trait?

That seems as effective as marking as unsafe, at least 👍

[michaelwoerister]

Perhaps a more robust approach would be to define it as an auto-trait that is unimplemented for types that are unstable across compilations?

If we can reliably define that set of types, that would be the best solution.

[michaelwoerister]

However, we also have to take the implementation of Ord into account which can do whatever it wants.

[eggyal]

I'll experiment.

@rustbot author

[eggyal]

However, we also have to take the implementation of Ord into account which can do whatever it wants.

Indeed, for the current definition of StableOrd a StableAcrossCompilations auto-trait is neither sufficient (Ord might depend upon global state) nor necessary (Ord might ignore any "unstable" fields).

However, if the definition of StableOrd was narrowed to require that the Ord implementation be structural, we have some interesting options: StructuralPartialEq becomes necessary (though sadly not sufficient, as Ord could still rely on global state to determine the order of unequal values); and a(n as yet non-existent) StructuralPartialOrd analogue would be both sufficient and necessary, which would enable us to automate the whole shebang whilst statically enforcing the contract:

trait StructuralPartialOrd {} // implemented by derive(PartialOrd)
auto trait StableAcrossCompilations {} // negative impls TBD

trait StableOrd: StableAcrossCompilations + StructuralPartialOrd + Ord {}
impl<T: ?Sized + StableAcrossCompilations + StructuralPartialOrd + Ord> StableOrd for T {}

Types that are currently StableOrd but would cease to be so under this narrowed definition would instead have to implement StableCompare directly. So far as I can see, the only ones are Option<T>, PathBuf and Path.

Furthermore we could adjust the StableCompare trait so that (instead of it having a stable_cmp method that performs a "stable comparison") it has a fn stable_sort_key(&self) -> impl StableOrd method. This would guarantee that the ordering is stable even in these cases (provided that the returned key is derived only from local data).

As regards negative implementations of the StableAcrossCompilations auto-trait:

If we can reliably define that set of types, that would be the best solution.

My immediate thought is that negative implementations would have to be provided for raw pointers and rustc_index::Idx types (modulo some exceptions, e.g. ItemLocalId).

---

Questions:

1. Do the risks justify this complexity?
2. Is such a StructuralPartialOrd trait likely to be accepted into the standard library, even as perma-unstable, when there is (presently) no call for it outside of rustc? If not, might StructualPartialEq be an acceptable proxy on the basis that a type that derives PartialEq is unlikely to rely on global state in PartialOrd?
3. Are there any other types that are unstable across compilations? Is this approach sufficiently reliable?
4. I have ignored the CAN_USE_UNSTABLE_SORT constant, as all implementations are currently true: can we therefore remove it altogether?

[Noratrieb]

i feel like this is overcomplicated it. keeping it as-is as a normal trait and maybe having a "I promise it's fine" const should be more than enough.

[eggyal]

@rustbot ready

[michaelwoerister]

Feel free to r=me, unless @Nilstrieb has any further comments.

[Noratrieb]

i don't!
@bors r=michaelwoerister

[bors]

📌 Commit 0e73e70 has been approved by michaelwoerister

It is now in the queue for this repository.

[michaelwoerister]

Thanks for cleaning this up, @eggyal 🙂

[bors]

⌛ Testing commit 0e73e70 with merge c290e9d...

[bors]

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing c290e9d to master...

[rust-timer]

Finished benchmarking commit (c290e9d): comparison URL.

Overall result: ❌ regressions - 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)	3.0%	[0.3%, 5.7%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 2.4%, secondary 6.0%)

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)	2.4%	[2.4%, 2.4%]	1
Regressions ❌ (secondary)	6.0%	[4.5%, 8.2%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.4%	[2.4%, 2.4%]	1

Cycles

Results (secondary 3.8%)

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

Binary size

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

Bootstrap: 692.703s -> 694.374s (0.24%)
Artifact size: 326.77 MiB -> 324.66 MiB (-0.64%)