Change privacy checks, particularly for tuple structs #58173

estebank · 2019-02-05T00:11:54Z

Move tuple struct privacy checks away from resolve in order to evaluate
more explicitly point at private tuple struct fields when they are the
cause of the tuple struct being inaccessible.

For structs that are inaccessible, point at the definition span.

Reword privacy messages to be more specific about the ADT kind name.

Group private field errors per struct.

error[E0451]: fields of struct `a::A` are private
  --> $DIR/multiple-private-fields.rs:8:18
   |
LL |     pub struct A(usize, usize);
   |     --------------------------- `a::A` defined here
...
LL |     let x = a::A(3, 4);
   |             ---- ^  ^ private field
   |             |    |
   |             |    private field
   |             `a::A` cannot be built due to private fields

 error[E0451]: fields of struct `a::B` are private
  --> $DIR/multiple-private-fields.rs:12:19
   |
LL |     pub struct B {a: usize, b: usize}
   |     --------------------------------- `a::B` defined here
...
LL |     let x = a::B {a:1, b:2};
   |             ------^^^--^^^-
   |             |     |    |
   |             |     |    private field
   |             |     private field
   |             `a::B` cannot be built due to private fields

Fix #58017.

rust-highfive · 2019-02-05T00:12:05Z

r? @michaelwoerister

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

estebank · 2019-02-05T00:31:36Z

src/test/ui/privacy/associated-item-privacy-trait.stderr

@@ -47,7 +47,7 @@ error: trait `priv_trait::PrivTr` is private
  --> $DIR/associated-item-privacy-trait.rs:25:16
   |
 LL |         let _: <Pub as PrivTr>::AssocTy;
-   |                ^^^^^^^^^^^^^^^^^^^^^^^^
+   |                ^^^^^^^^^^^^^^^^^^^^^^^^ private


It'd be nice to only emit one of this and the prior error.

The hightlight is also not optimal. It should probably be just on the PrivTr or at least on the <Pub as PrivTr>

Move tuple struct privacy checks away from `resolve` in order to evaluate more explicitely point at private tuple struct fields when they are the cause of the tuple struct being inaccessible. For structs that are inaccessible, point at the definition span. Reword privacy messages to be more specific about the ADT kind name. Group private field errors per struct.

oli-obk · 2019-02-05T08:53:26Z

src/librustc_privacy/lib.rs

+                }
+                self.emit_field_checks(adt, expr.span, field_errors, "built");
+            }
+            hir::ExprKind::Call(ref path, ref fields) => {


Add a comment that this is about tuple struct constructors

oli-obk · 2019-02-05T08:54:57Z

src/librustc_privacy/lib.rs

 }

 impl<'a, 'tcx> TypePrivacyVisitor<'a, 'tcx> {
    fn item_is_accessible(&self, did: DefId) -> bool {
-        def_id_visibility(self.tcx, did).0.is_accessible_from(self.current_item, self.tcx)
+        let (a, ..) = def_id_visibility(self.tcx, did);


use a better variable name than a

oli-obk · 2019-02-05T08:56:11Z

src/librustc_privacy/lib.rs

@@ -799,22 +801,71 @@ struct NamePrivacyVisitor<'a, 'tcx: 'a> {
    tables: &'a ty::TypeckTables<'tcx>,
    current_item: ast::NodeId,
    empty_tables: &'a ty::TypeckTables<'tcx>,
+    reported_tuple_structs: FxHashSet<Span>,


explain this field here instead of just at the use sites

oli-obk · 2019-02-05T08:56:30Z

src/librustc_privacy/lib.rs

@@ -927,11 +1042,13 @@ struct TypePrivacyVisitor<'a, 'tcx: 'a> {
    in_body: bool,
    span: Span,
    empty_tables: &'a ty::TypeckTables<'tcx>,
+    reported_tuple_structs: FxHashSet<Span>,


explain this field here instead of at the use sites

oli-obk · 2019-02-05T08:57:30Z

src/librustc_privacy/lib.rs

-            self.tcx.sess.span_err(self.span, &format!("{} `{}` is private", kind, descr));
+        let is_ok = self.item_is_accessible(def_id);
+        if !is_ok {
+            match self.tcx.hir().as_local_node_id(def_id) {


use if let instead of matches here

oli-obk · 2019-02-05T09:35:34Z

src/test/ui/hygiene/impl_items.stderr

@@ -1,8 +1,8 @@
-error: type `for<'r> fn(&'r foo::S) {foo::S::f}` is private
+error: method `foo::S::f` is private


oli-obk · 2019-02-05T09:36:38Z

src/librustc_privacy/lib.rs

+                let kind_name = path.def.kind_name();
+                let sp = path.span;
+                let msg = format!("{} `{}` is private", kind_name, path);
+                let label = format!("{} not accesssible from here", kind_name);


Suggested change

let label = format!("{} not accesssible from here", kind_name);

let label = format!("private {} is not accesssible from here", kind_name);

oli-obk · 2019-02-05T09:38:40Z

src/test/ui/privacy/associated-item-privacy-trait.stderr

@@ -47,7 +47,7 @@ error: trait `priv_trait::PrivTr` is private
  --> $DIR/associated-item-privacy-trait.rs:25:16
   |
 LL |         let _: <Pub as PrivTr>::AssocTy;
-   |                ^^^^^^^^^^^^^^^^^^^^^^^^
+   |                ^^^^^^^^^^^^^^^^^^^^^^^^ private


The hightlight is also not optimal. It should probably be just on the PrivTr or at least on the <Pub as PrivTr>

oli-obk · 2019-02-05T09:39:15Z

src/test/ui/privacy/associated-item-privacy-trait.stderr

 ...
 LL |     priv_parent_substs::mac!();
   |     --------------------------- in this macro invocation

-error: type `priv_parent_substs::Priv` is private
+error: struct `priv_parent_substs::Priv` is private
  --> $DIR/associated-item-privacy-trait.rs:131:35
   |
 LL |         pub type InSignatureTy2 = <Priv as PubTr<Pub>>::AssocTy;


Similarly here, the highlight should only be on the Priv

oli-obk · 2019-02-05T09:40:09Z

src/test/ui/privacy/multiple-private-fields.stderr

+LL |     let x = a::A(3, 4);
+   |             ---- ^  ^ private field
+   |             |    |
+   |             |    private field


I think there's a way to only have a single message for multiple spans in one line

petrochenkov · 2019-02-05T10:22:31Z

Move tuple struct privacy checks away from resolve in order to evaluate
more explicitly point at private tuple struct fields when they are the
cause of the tuple struct being inaccessible.

From this description and a quick glance at the code this PR does a thing that's fundamentally wrong.

I'll review this in detail a bit later, but here's the PR that introduced the current visibility rules for constructors and moved the privacy check to resolve - #38932.

petrochenkov · 2019-02-05T18:17:49Z

I'm against going into this direction of moving privacy checks from rustc_resolve to rustc_privacy.
This seriously goes against logic of the language and compiler staging, diagnostic improvements don't worth a hack/debt this big.

It would be ok to attach some extra data to PrivacyError or do something more fancy when reporting it.

petrochenkov · 2019-02-05T18:32:57Z

A bit of context.

When we resolving an entity (name, path, field, method) to a DefId we generally need to immediately check it for privacy and stability.
When we are doing it some other way, we generally have issues (e.g. imports are not checked for stability right now).

Privacy checks mostly follow this rule with exception of NamePrivacyVisitor which is a rudiment of the previous system and needs to be eliminated in favor of checking fields in struct literals/patterns immediately after they are resolved in typeck, like it's done e.g. for dot field or method accesses.
Previously, NamePrivacyVisitor did more, including path privacy checks, but they were moved to resolve to fix issues with imports.
(I would expect this PR to have issues with imports as well, but I haven't looked in too much details.)
As a result, NamePrivacyVisitor should certainly not be expanded.

The situation is worse with stability checking, where path stability is still not moved to resolve-time, like it's done for method or associated item stability.

What remains in rustc_privacy is more or less "reachability tracking" + obsolete stuff that needs to be removed once #48054 is done, so the name rustc_privacy itself is somewhat a misnomer now.
End even for the reachability tracking, some parts of it need to moved to resolve to avoid issues with imports (#57922 (comment)).

bors · 2019-02-07T12:55:49Z

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

rust-highfive assigned michaelwoerister Feb 5, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 5, 2019

This comment has been minimized.

Sign in to view

estebank force-pushed the tuple-struct-priv branch from 129226c to c565418 Compare February 5, 2019 00:29

estebank commented Feb 5, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

estebank force-pushed the tuple-struct-priv branch from c565418 to 7ce29fa Compare February 5, 2019 06:32

petrochenkov assigned petrochenkov and unassigned michaelwoerister Feb 5, 2019

oli-obk reviewed Feb 5, 2019

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 Feb 5, 2019

estebank closed this Feb 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change privacy checks, particularly for tuple structs #58173

Change privacy checks, particularly for tuple structs #58173

estebank commented Feb 5, 2019 •

edited

rust-highfive commented Feb 5, 2019

This comment has been minimized.

estebank Feb 5, 2019

oli-obk Feb 5, 2019

This comment has been minimized.

oli-obk Feb 5, 2019

oli-obk Feb 5, 2019

oli-obk Feb 5, 2019

oli-obk Feb 5, 2019

oli-obk Feb 5, 2019

oli-obk Feb 5, 2019

oli-obk Feb 5, 2019

oli-obk Feb 5, 2019

oli-obk Feb 5, 2019

oli-obk Feb 5, 2019

petrochenkov commented Feb 5, 2019

petrochenkov commented Feb 5, 2019

petrochenkov commented Feb 5, 2019

bors commented Feb 7, 2019

		@@ -1,8 +1,8 @@
		error: type `for<'r> fn(&'r foo::S) {foo::S::f}` is private
		error: method `foo::S::f` is private

	let label = format!("{} not accesssible from here", kind_name);
	let label = format!("private {} is not accesssible from here", kind_name);

Change privacy checks, particularly for tuple structs #58173

Change privacy checks, particularly for tuple structs #58173

Conversation

estebank commented Feb 5, 2019 • edited

rust-highfive commented Feb 5, 2019

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Feb 5, 2019

petrochenkov commented Feb 5, 2019

petrochenkov commented Feb 5, 2019

bors commented Feb 7, 2019

estebank commented Feb 5, 2019 •

edited