privacy: Assert that compared visibilities are (usually) ordered

[petrochenkov]

View all comments

And make "greater than" (>) the new primary operation for comparing visibilities instead of "is at least" (>=).

[rustbot]

r? @adwinwhite

rustbot has assigned @adwinwhite.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 12 candidates

[petrochenkov]

@bors try @rust-timer queue

[petrochenkov]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 0c52563 (0c5256376cfe5d7821eaee808f971cc64299b5ff, parent: 12f35ad39ed3e39df4d953c46d4f6cc6c82adc96)

[rust-timer]

Finished benchmarking commit (0c52563): comparison URL.

Overall result: ❌ regressions - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.2%]	9
Regressions ❌ (secondary)	0.6%	[0.3%, 1.7%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.2%, 0.2%]	9

Max RSS (memory usage)

Results (primary 1.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.6%	[1.1%, 2.1%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.6%	[1.1%, 2.1%]	2

Cycles

Results (secondary -3.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-4.4%, -2.1%]	2
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 489.936s -> 490.568s (0.13%)
Artifact size: 394.18 MiB -> 394.10 MiB (-0.02%)

[petrochenkov]

One small regression on libc (which is a sort of stress test for large number of imports and other items).
I think we can merge like this, or switch to debug_assert, but then only @matthiaskrgr's fuzzing or something like that will find the potential issues.

[petrochenkov]

This could also be potentially blocked on #155213 which addresses on of the FIXMEs here.

[adwinwhite]

@petrochenkov Sorry but perhaps you can manually pick a reviewer who's familiar with this part? I'm not sure that reroll can get to the right person.

[petrochenkov]

Hm, this is a general visibility infra used everywhere from name resolution to type checking, I don't think anyone really specializes in it.
I'll block it on #155213 and then maybe reroll.

[petrochenkov]

@adwinwhite
Visibilities (pub or pub(in module)) form a tree, the same tree as crate root and mod items.
So, like modules they are partially ordered, if module A is an ancestor of module B, then they are ordered and pub(in A) > pub(in b).
However, modules are not always ancestors of each other, they can be neighbors, for example, then the corresponding visibilities are unordered.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[petrochenkov]

@rustbot ready

@adwinwhite feel free to reroll if the explanation in #155257 (comment) does not make it clear.

[petrochenkov]

This can be Option<Ordering>, but then we'll need to add some Debug and Copy bounds on fn greater_than.
Not sure what is better, maybe the bounds, because that would be a purely compile-time trouble.

View changes since the review

[petrochenkov]

One small regression on libc (which is a sort of stress test for large number of imports and other items).

#155608 has a way to avoid comparisons in both directions if this regression shows up again, so no need to switch to debug-only checking.