Account for sealed traits in privacy and trait bound errors #112686

estebank · 2023-06-16T00:27:47Z

On trait bound errors caused by super-traits, identify if the super-trait is publicly accessibly and if not, explain "sealed traits".

error[E0277]: the trait bound `S: Hidden` is not satisfied
  --> $DIR/sealed-trait-local.rs:17:20
   |
LL | impl a::Sealed for S {}
   |                    ^ the trait `Hidden` is not implemented for `S`
   |
note: required by a bound in `Sealed`
  --> $DIR/sealed-trait-local.rs:3:23
   |
LL |     pub trait Sealed: self::b::Hidden {
   |                       ^^^^^^^^^^^^^^^ required by this bound in `Sealed`
   = note: `Sealed` is a "sealed trait", because to implement it you also need to implelement `a::b::Hidden`, which is not accessible; this is usually done to force you to use one of the provided types that already implement it

Deduplicate privacy errors that point to the same path segment even if their deduplication span are different.

When encountering a path that is not reachable due to privacy constraints path segments other than the last, keep metadata for the last path segment's Res in order to look for alternative import paths for that item to suggest. If there are none, be explicit that the item is not accessible.

error[E0603]: module `b` is private
  --> $DIR/re-exported-trait.rs:11:9
   |
LL | impl a::b::Trait for S {}
   |         ^ private module
   |
note: the module `b` is defined here
  --> $DIR/re-exported-trait.rs:5:5
   |
LL |     mod b {
   |     ^^^^^
help: consider importing this trait through its public re-export instead
   |
LL | impl a::Trait for S {}
   |      ~~~~~~~~

error[E0603]: module `b` is private
  --> $DIR/private-trait.rs:8:9
   |
LL | impl a::b::Hidden for S {}
   |         ^  ------ trait `b` is not publicly reachable
   |         |
   |         private module
   |
note: the module `b` is defined here
  --> $DIR/private-trait.rs:2:5
   |
LL |     mod b {
   |     ^^^^^

rustbot · 2023-06-16T00:27:52Z

r? @fee1-dead

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

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

compiler/rustc_resolve/src/imports.rs

tests/ui/macros/issue-88228.stderr

compiler-errors · 2023-06-16T02:08:20Z

tests/ui/privacy/sealed-traits/private-trait-non-local.stderr

+error[E0603]: module `index` is private
+  --> $DIR/private-trait-non-local.rs:2:18
+   |
+LL | use core::slice::index::private_slice_index::Sealed;


I quite like this framing, though I wonder if we should either link to somewhere with privacy rules, or at least drive this home by noting something like "an item is only reachable via a path if all of the segments are reachable" -- with maybe better wording than what I wrote...

Just want something that folks can take away from the error message as intuition for why public-unreachable error happen the way they do?

I'd like to add some note about it, but I'm not sure what it should look like at the moment.

petrochenkov · 2023-06-16T08:04:04Z

This touches name resolution infra, not only diagnostics.
r? @petrochenkov

petrochenkov · 2023-06-16T16:51:54Z

You guys really want me to suffer.

In a typical estebank's fashion the PR

makes several independent changes but merges them together, so it's not clear a) what effect each change has on diagnostics (test outputs) and b) what cost benefit ratio each independent change has.
introduces ad hoc code in random places and technical debt so I have to understand which changes are not worth it and are better dropped, which I cannot do due to 1.
has no PR description summarizing what you want to achieve

As a result, I don't want to see this PR merged due to 2. but I also don't want to review it to suggest improvements due to 1-3..

petrochenkov · 2023-06-16T17:04:24Z

Could you split this into parts small enough to make it digestable?

one commit introducing new tests
several commits introducing minimal orthogonal changes to name resolution infra (non-diagnostics.rs code, this is what I want to keep maintainable and garbage-free) and showing their effect on test outputs
commit with remaining changes in diagnostics.rs files

(And also add a PR description.)
@rustbot author

khionu · 2023-06-16T22:03:27Z

@petrochenkov This review was really in bad form. You have valid critique that could have carried its own weight without either of the first two lines. Slipping in a public, passive-aggressive callout is no way to talk things out with your fellow project members. Please avoid that in the future, and if you feel you need help resolving issues with other people in the community, please reach out to Mods. We're here to help issues before they escalate, not just after.

estebank · 2023-06-16T22:51:40Z

I've split this PR to the best of my abilities into easily digestible units. That being done, I do not believe initial PR was unreasonably unfocused, the changes were tightly related. Given the nature of the work when I'm doing these kind of changes, I sometimes have to experiment with multiple alternative ways of accomplishing the result I'm looking for looking for the most minimally invasive way of doing so. The above separation of commits is ahistorical and somewhat artificial and the ask requires more additional work from me than what would otherwise be required from the reviewer.

In more general terms, I understand that we have different perspectives on where different changes fall in the "increased complexity"/"user focus" axes, but I must say, I do not appreciate the antagonistic way you interact with me, here or in the past. If you desire to talk about it, with mods or on our own, you can find me in Zulip. I do not believe this is the appropriate place for that discussion.

petrochenkov · 2023-06-17T07:35:34Z

@estebank
I prefer transparency and no mod involvement is required here.
I've been talking about this not once, not twice, not even three times, but yesterday evening I opened this PR and saw all the same stuff, and it was pretty infuriating.
I'll review the new version in a couple of days.

fee1-dead · 2023-06-17T12:59:11Z

compiler/rustc_resolve/src/diagnostics.rs

+                err.span_label(
+                    outer_ident.span,
+                    format!("{} `{}` is not publicly reachable", this_res.descr(), ident),
+                );


can this use translatable diagnostics?

I'll do this as part of a separate PR.

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

petrochenkov · 2023-06-20T17:47:56Z

Some more detailed meta-comments to clarify what I'd like to see from PRs like this as a reviewer:

9f1279030655143b244aeb0f750641654a8da969 34c7c130987c12f11db222743da8bf62ed352e42 761d2b8c5937d72d3622e4664bf3280875e5df6d
- These commits both add new tests and change their output, so it's hard to see what changes.
- From a PR author's point of view it would be easiest to just dump all the new tests into the single first commit as I suggested in Account for sealed traits in privacy and trait bound errors #112686 (comment), and from a reviewer's point of view that would be totally fine.
55ef88eb8ab59351eb9a5406c8a51ad66776ae84
- Nit: moving code (sort) and changing code (dedup) in a same commit, so it's hard to see what changes.
9f1279030655143b244aeb0f750641654a8da969 24dbcab241c1e9c1558a840da2ceeaddf27d5aaa 196fbc6d425665072034c4e16cf0e6cf024ec087
- This change is more independent from the rest of the PR than others, and touches a different area of the compiler maintained by different people, so it would be better submitted as a separate PR.
- That's probably too much splitting into commits, the commits don't touch non-diagnostic infra and all of them add a single diagnostic in a single place, could be merged into one.
Otherwise the current split into commits is great and makes it much easier to understand what happens, compared to the original version, even if it seemed artificial to you.
- It still took me hours to review and write this post though, because there are many "logical units" of changes.
- Some GitHub review tools work great at PR level, but not at individual commit levels, that's why I ask people to split large PRs (not necessarily large in terms of code amount, but more in terms of these logical units of changes). This PR would be fine two PRs, as I mentioned a few lines above.

I'll post the comments on code a bit later.

tests/ui/privacy/privacy1.rs

tests/ui/privacy/privacy-in-paths.rs

compiler/rustc_resolve/src/ident.rs

compiler/rustc_resolve/src/diagnostics.rs

compiler/rustc_resolve/src/ident.rs

compiler/rustc_resolve/src/diagnostics.rs

compiler/rustc_resolve/src/imports.rs

compiler/rustc_resolve/src/diagnostics.rs

compiler/rustc_resolve/src/ident.rs

compiler/rustc_resolve/src/imports.rs

compiler/rustc_resolve/src/ident.rs

resolve: Minor cleanup to `fn resolve_path_with_ribs` A single-use closure is inlined and one unnecessary enum is removed. Noticed when reviewing rust-lang#112686.

compiler/rustc_resolve/src/lib.rs

compiler/rustc_resolve/src/diagnostics.rs

petrochenkov · 2023-06-22T16:14:53Z

r=me after addressing #112686 (comment) and cleaning up git history.
@rustbot author

estebank · 2023-06-22T16:16:45Z

Addressed the last review comments. Rebased on top of a recent master to incorporate #112892.

Do you want me to merge everything into a single commit?

When implementing a public trait with a private super-trait, we now emit a note that the missing bound is not going to be able to be satisfied, and we explain the concept of a sealed trait.

Suggest publicly accessible paths for items in private mod: When encountering a path in non-import situations that are not reachable due to privacy constraints, search for any public re-exports that the user could use instead. Track whether an import suggestion is offering a re-export. When encountering a path with private segments, mention if the item at the final path segment is not publicly accessible at all. Add item visibility metadata to privacy errors from imports: On unreachable imports, record the item that was being imported in order to suggest publicly available re-exports or to be explicit that the item is not available publicly from any path. In order to allow this, we add a mode to `resolve_path` that will not add new privacy errors, nor return early if it encounters one. This way we can get the `Res` corresponding to the final item in the import, which is used in the privacy error machinery.

petrochenkov · 2023-06-22T16:56:59Z

Well, ideally it would still be commit per independent change, but the PR is now much smaller so it's not too bad as is.
@bors r+

bors · 2023-06-22T16:57:01Z

📌 Commit 7dffd24 has been approved by petrochenkov

It is now in the queue for this repository.

bors · 2023-06-22T18:23:22Z

⌛ Testing commit 7dffd24 with merge 04075b3...

bors · 2023-06-22T21:26:17Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 04075b3 to master...

rust-timer · 2023-06-22T22:40:49Z

Finished benchmarking commit (04075b3): 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)

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

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)	1.4%	[1.0%, 1.8%]	3
Regressions ❌ (secondary)	3.1%	[2.5%, 3.7%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.4%	[1.0%, 1.8%]	3

Binary size

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

Bootstrap: 661.969s -> 663.045s (0.16%)

rustbot assigned fee1-dead Jun 16, 2023

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 Jun 16, 2023

This comment was marked as resolved.

Sign in to view

This comment has been minimized.

Sign in to view

estebank force-pushed the sealed-traits branch from 0d5859c to 844092c Compare June 16, 2023 00:38

This comment was marked as resolved.

Sign in to view

estebank force-pushed the sealed-traits branch from 844092c to a2efc01 Compare June 16, 2023 01:14

compiler-errors reviewed Jun 16, 2023

View reviewed changes

rustbot assigned petrochenkov and unassigned fee1-dead Jun 16, 2023

estebank force-pushed the sealed-traits branch from 2d33a9e to abb2696 Compare June 16, 2023 16:17

rustbot 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 Jun 16, 2023

estebank force-pushed the sealed-traits branch from abb2696 to 761d2b8 Compare June 16, 2023 21:50

estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 17, 2023

fee1-dead reviewed Jun 17, 2023

View reviewed changes

petrochenkov reviewed Jun 20, 2023

View reviewed changes

petrochenkov 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 Jun 20, 2023

petrochenkov reviewed Jun 21, 2023

View reviewed changes

estebank force-pushed the sealed-traits branch from e2bcbbc to 48da54b Compare June 21, 2023 17:57

estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 21, 2023

estebank force-pushed the sealed-traits branch from 7023519 to 1e964b6 Compare June 21, 2023 22:56

petrochenkov reviewed Jun 22, 2023

View reviewed changes

compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved

compiler/rustc_resolve/src/diagnostics.rs Outdated Show resolved Hide resolved

petrochenkov 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 Jun 22, 2023

estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 22, 2023

petrochenkov reviewed Jun 22, 2023

View reviewed changes

compiler/rustc_resolve/src/diagnostics.rs Outdated Show resolved Hide resolved

rustbot 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 Jun 22, 2023

estebank added 2 commits June 22, 2023 16:50

Account for sealed traits in trait bound errors

717c481

When implementing a public trait with a private super-trait, we now emit a note that the missing bound is not going to be able to be satisfied, and we explain the concept of a sealed trait.

estebank force-pushed the sealed-traits branch from 08eee16 to 7dffd24 Compare June 22, 2023 16:53

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

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

bors merged commit 04075b3 into rust-lang:master Jun 22, 2023

rustbot added this to the 1.72.0 milestone Jun 22, 2023

bors mentioned this pull request Jun 22, 2023

make help text for cannot find macro actionable #112091

Closed

estebank deleted the sealed-traits branch November 9, 2023 05:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for sealed traits in privacy and trait bound errors #112686

Account for sealed traits in privacy and trait bound errors #112686

estebank commented Jun 16, 2023 •

edited

Loading

rustbot commented Jun 16, 2023

This comment was marked as resolved.

This comment has been minimized.

This comment was marked as resolved.

compiler-errors Jun 16, 2023 •

edited

Loading

estebank Jun 16, 2023

petrochenkov commented Jun 16, 2023

petrochenkov commented Jun 16, 2023

petrochenkov commented Jun 16, 2023

khionu commented Jun 16, 2023

estebank commented Jun 16, 2023

petrochenkov commented Jun 17, 2023

fee1-dead Jun 17, 2023

estebank Jun 21, 2023

petrochenkov commented Jun 20, 2023 •

edited

Loading

petrochenkov commented Jun 22, 2023

estebank commented Jun 22, 2023

petrochenkov commented Jun 22, 2023

bors commented Jun 22, 2023

bors commented Jun 22, 2023

bors commented Jun 22, 2023

rust-timer commented Jun 22, 2023

Account for sealed traits in privacy and trait bound errors #112686

Account for sealed traits in privacy and trait bound errors #112686

Conversation

estebank commented Jun 16, 2023 • edited Loading

rustbot commented Jun 16, 2023

This comment was marked as resolved.

This comment has been minimized.

This comment was marked as resolved.

compiler-errors Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

estebank Jun 16, 2023

Choose a reason for hiding this comment

petrochenkov commented Jun 16, 2023

petrochenkov commented Jun 16, 2023

petrochenkov commented Jun 16, 2023

khionu commented Jun 16, 2023

estebank commented Jun 16, 2023

petrochenkov commented Jun 17, 2023

fee1-dead Jun 17, 2023

Choose a reason for hiding this comment

estebank Jun 21, 2023

Choose a reason for hiding this comment

petrochenkov commented Jun 20, 2023 • edited Loading

petrochenkov commented Jun 22, 2023

estebank commented Jun 22, 2023

petrochenkov commented Jun 22, 2023

bors commented Jun 22, 2023

bors commented Jun 22, 2023

bors commented Jun 22, 2023

rust-timer commented Jun 22, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

estebank commented Jun 16, 2023 •

edited

Loading

compiler-errors Jun 16, 2023 •

edited

Loading

petrochenkov commented Jun 20, 2023 •

edited

Loading