Privatize constructors of tuple structs with private fields #38932

petrochenkov · 2017-01-08T21:56:34Z

This PR implements the strictest version of such "privatization" - it just sets visibilities for struct constructors, this affects everything including imports.

visibility(struct_ctor) = min(visibility(struct), visibility(field_1), ..., visibility(field_N))

Needs crater run before proceeding.

Resolves rust-lang/rfcs#902

r? @nikomatsakis

nikomatsakis · 2017-01-09T22:50:01Z

Kicked off a crater run.

nikomatsakis · 2017-01-10T18:22:01Z

petrochenkov · 2017-01-10T19:15:57Z

There are 3 distinct regressions:
rotor-stream-0.6.2, radiant-rs-0.1.2 and syntex_syntax-0.54.0.
Other "root" regressions actually depend on older versions of syntex_syntax.

All the regressions can be minimized like this:

mod m {
    // Tuple struct with a private field.
    // Type S is pub(pub).
    // The field S::0 is pub(m).
    // Constructor S is min(pub(pub), pub(m)) -> pub(m).
    pub struct S(u8);
    
    fn f() {
        // Try to use S from the root module in value namespace.
        // No success, ::S exists only in type namespace.
        // How to fix: use S from this module instead of ::S from the root module.
        ::S;
    }
}

// This imports S only in type namespace, value S is too private.
// This is expected filtering behavior of imports described in RFC 1560.
use m::S;

fn main() {}

The semantics visibility(struct_ctor) = min(visibility(struct), visibility(field_1), ..., visibility(field_N)) is what I'd prefer to go for instead of just prohibiting private ctors in patterns. I'm interested in what @nrc and @jseyfried think about this.

Warning period may be non-trivial, but I'll try to look what inaccessible_extern_crate does and repeat it.

nikomatsakis · 2017-01-10T19:54:38Z

@petrochenkov nice analysis, interesting example. I too am curious what @jseyfried and @nrc think =)

cc @rust-lang/lang -- this has to do with the visibility of struct constructors, and the (somewhat odd, but at one time legal) pattern that breaks if you change it. Basically you could pub use the value namespace from a place that isn't allowed to use it, but only use from inside another module where it is legal to use it. This seems surprising.

jseyfried · 2017-01-11T04:56:34Z

I'm in favor of this solution -- I think it's the cleanest way to guarantee that e.g. pub struct S(u32); is indistinguishable from pub struct S { x: u32 } outside S's module.

To implement a warning cycle, I would

not touch import resolution; I believe this PR is highly unlikely to break import resolution in practice
after import resolution, warn iff
- we can't resolve a value S in a given scope,
- the type S in that scope resolves to a tuple struct, and
- the tuple struct's constructor is visible.

nrc · 2017-01-11T18:44:20Z

I'm also in favour of this, seems like the sensible approach.

nikomatsakis · 2017-01-17T16:21:38Z

@petrochenkov -- it seems like there is a general 👍 to this approach. Are you pursuing code for a warning period? @jseyfried has some suggestions here.

petrochenkov · 2017-01-17T16:36:55Z

Yes, I'll update the PR in a few days.

petrochenkov · 2017-01-20T17:06:41Z

Updated with a compatibility lint (following the strategy from #38932 (comment)).
I've made the lint deny-by-default because it's really hacky and not entirely precise, and the number of regressions is small.

bors · 2017-01-21T03:26:06Z

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

petrochenkov · 2017-01-21T10:00:29Z

Rebased.

bors · 2017-01-22T06:41:12Z

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

jseyfried

r=me modulo comments

jseyfried · 2017-01-22T06:47:41Z

src/test/compile-fail/privacy-struct-ctor.rs

+
+fn main() {
+    m::S; //~ ERROR tuple struct `S` is private
+    S; //~ ERROR expected value, found struct `S`


Could we inform the user here that the tuple struct constructor S is private?
We could use the same condition as the warning cycle, except that we would report if the constructor were not visible.

Yeah, that would be useful, but I hoped to get rid of legacy_ctor_visibilities together with the compatibility lint.
What I don't like is that this table is filled on all successful runs, but is actually needed very rarely.
Is there any way to map struct's DefId into (ctor's DefId + ctor's ty::Visibility) on demand without building tables in advance?
It'll also require extending metadata reading interface to obtain the same mapping for other crates. I really dislike adding code through the compiler that is necessary only for reporting a single special note, but well...

I agree with @jseyfried that the warning seems nice

@petrochenkov

Is there any way to map struct's DefId into (ctor's DefId + ctor's ty::Visibility) on demand without building tables in advance?

We could simplify a little by mapping the struct's DefId to the ctor's &'a NameBinding<'a>, but we'd still be building an extra table.

To avoid that, we could add a variant

NameBindingKind::TupleStruct(Def, &'a NameBinding<'a> /* ctor */)

and use that for tuple struct type bindings instead of NameBindingKind::Def.
NameBindingKind is well encapsulated, so that shouldn't be too big of a change -- still might not be worth it though.

jseyfried · 2017-01-22T07:14:33Z

src/librustc/lint/builtin.rs

@@ -224,6 +224,12 @@ declare_lint! {
 }

 declare_lint! {
+    pub LEGACY_CONSTRUCTOR_VISIBILITY,
+    Deny,


Do we need to make this deny-by-default, given that there are three breaking crates in the wild?
While the warning cycle is a bit hacky, I think the risk of "false positives" is very low.

3 is small, no?
I preferred deny-by-default mostly to avoid successfully compiling more illegal code by default.

The policy is usually warn-by-default, then deny-by-default, no? But we can make an exception, though we should make an RFC bot request about it. Either way, we should contact the authors of the crates in question and/or submit PRs.

nikomatsakis · 2017-01-23T21:36:53Z

@rfcbot fcp merge

This is a fix to a privacy oversight that allows people to observe whether a tuple struct with a private field was defined as a tuple struct or not, something which was not intended to be public. The problem is that people can do let Foo(_) = ... only if the struct is defined as a tuple struct. The fix is to consider this illegal if the struct has only private fields.

The question is whether to make this a WARN-by-default or DENY-by-default lint. The usual policy would be warn-by-default. There are currently three root crates breaking in the wild. There are a lot of affected crates out there (285?) but I believe that most of those will be unaffected in short term because of cap-lints, though we will want to ensure they upgrade at some point.

rotor-stream-0.6.2 (cc @tailhook)
radiant-rs-0.1.2 (cc @sinesc)
syntex_syntax-0.54.0 (any need for a cc here?)

Given the small number of affected crates, I think starting with deny-by-default is reasonable (in particular, since this can still be overridden by cap-lints, and so most crates should be unaffected until there is time to fix the problem). Hence the FCP "merge" request. But I'd like to hear others' opinions (including the authors of the affected crates).

cc @rust-lang/compiler @rust-lang/core

rfcbot · 2017-01-23T21:43:42Z

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

aturon · 2017-01-23T21:44:46Z

Agreed that deny-by-default seems OK here. Glad to see this getting fixed!

sinesc · 2017-01-24T07:20:50Z

Regarding radiant, it looks like I just forgot a "super" here and it happens to work anyway because the root pub uses the struct as well. (?)
PR makes sense to me, assuming my understanding is correct :-)

rfcbot · 2017-01-25T17:05:02Z

🔔 This is now entering its final comment period, as per the review above. 🔔

nikomatsakis · 2017-01-25T17:25:18Z

r? @jseyfried

The code looks reasonable to me, but I think @jseyfried is better suited to really review this.

jseyfried · 2017-01-27T09:16:02Z

@petrochenkov r=me with the diagnostics improvement.

petrochenkov · 2017-01-29T00:37:11Z

Updated with better diagnostics.
I'm not sending this to bors, the FCP still continues. (for how long?)

petrochenkov · 2017-01-29T00:38:46Z

src/test/compile-fail/issue-38412.rs

@@ -11,6 +11,7 @@
 fn main() {
    let Box(a) = loop { };
    //~^ ERROR expected tuple struct/variant, found struct `Box`
+    //~| ERROR expected tuple struct/variant, found struct `Box`


This is a compiletest bug. If one error has two labels, compiletest will require two "expected errors".

aturon · 2017-01-31T16:02:44Z

@petrochenkov

I'm not sending this to bors, the FCP still continues. (for how long?)

We're abusing RFC bot here (someday we want a mode just for getting team approval of a PR). There's not actually an FCP here; you can merge at any time.

petrochenkov · 2017-01-31T16:05:22Z

Excellent.
@bors r=jseyfried

bors · 2017-01-31T16:05:23Z

📌 Commit d38a8ad has been approved by jseyfried

bors · 2017-02-01T02:53:13Z

⌛ Testing commit d38a8ad with merge 558ea08...

bors · 2017-02-01T04:34:00Z

💔 Test failed - status-appveyor

petrochenkov · 2017-02-01T09:46:15Z

@bors retry
(curl failure)

bors · 2017-02-01T11:15:44Z

⌛ Testing commit d38a8ad with merge 8060de2...

bors · 2017-02-01T12:58:44Z

💔 Test failed - status-appveyor

petrochenkov · 2017-02-01T13:04:36Z

@bors retry
(the same curl failure)

bors · 2017-02-01T13:40:14Z

⌛ Testing commit d38a8ad with merge 5dc365f...

bors · 2017-02-01T14:25:08Z

💔 Test failed - status-travis

petrochenkov · 2017-02-01T14:30:00Z

That curl issue again.
Looks like something persistent.

alexcrichton · 2017-02-01T17:17:36Z

@bors: retry

…

On Wed, Feb 1, 2017 at 6:30 AM, Vadim Petrochenkov ***@***.*** > wrote: That curl issue again. Looks like something persistent. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#38932 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95LA10zE6MuS8yIfT-rZN7iSi7GNDks5rYJbqgaJpZM4Ld0MC> .

bors · 2017-02-02T00:27:23Z

⌛ Testing commit d38a8ad with merge 43e169e...

bors · 2017-02-02T02:42:04Z

💔 Test failed - status-travis

alexcrichton · 2017-02-02T02:46:58Z

@bors: retry * osx linker segfault #38878

…

On Wed, Feb 1, 2017 at 6:42 PM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/rust/builds/197495794> — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#38932 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95CiOYSExNQCXLrazDUXU4O2HRSVuks5rYUJ-gaJpZM4Ld0MC> .

bors · 2017-02-02T05:10:45Z

⌛ Testing commit d38a8ad with merge 1b6b20a...

@nikomatsakis

Privatize constructors of tuple structs with private fields This PR implements the strictest version of such "privatization" - it just sets visibilities for struct constructors, this affects everything including imports. ``` visibility(struct_ctor) = min(visibility(struct), visibility(field_1), ..., visibility(field_N)) ``` Needs crater run before proceeding. Resolves rust-lang/rfcs#902 r? @nikomatsakis

bors · 2017-02-02T07:38:59Z

☀️ Test successful - status-appveyor, status-travis
Approved by: jseyfried
Pushing 1b6b20a to master...

rust-highfive assigned nikomatsakis Jan 8, 2017

petrochenkov force-pushed the privctor branch from 7d1e94b to ebda4aa Compare January 9, 2017 07:24

petrochenkov force-pushed the privctor branch from ebda4aa to 16d98fd Compare January 20, 2017 17:03

petrochenkov mentioned this pull request Jan 20, 2017

Tracking issue for legacy_constructor_visibility compatibility lint #39207

Closed

2 tasks

petrochenkov force-pushed the privctor branch from 16d98fd to a4e1795 Compare January 20, 2017 17:13

petrochenkov force-pushed the privctor branch from a4e1795 to 8de826e Compare January 21, 2017 10:00

jseyfried reviewed Jan 22, 2017

View reviewed changes

nikomatsakis added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 23, 2017

rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jan 25, 2017

rust-highfive assigned jseyfried and unassigned nikomatsakis Jan 25, 2017

petrochenkov commented Jan 29, 2017

View reviewed changes

bors merged commit d38a8ad into rust-lang:master Feb 2, 2017

petrochenkov deleted the privctor branch March 16, 2017 19:43

tailhook mentioned this pull request May 10, 2017

Rust 1.17 regression, rotor-stream 0.6.2, private struct constructors #41802

Closed

petrochenkov mentioned this pull request Jun 14, 2017

Future-proofing enums/structs with #[non_exhaustive] attribute rust-lang/rfcs#2008

Merged

petrochenkov mentioned this pull request Feb 5, 2019

Change privacy checks, particularly for tuple structs #58173

Closed

Privatize constructors of tuple structs with private fields #38932

Privatize constructors of tuple structs with private fields #38932

Conversation

petrochenkov commented Jan 8, 2017 • edited Loading

nikomatsakis commented Jan 9, 2017

nikomatsakis commented Jan 10, 2017

Root regressions, sorted by rank (checked means "investigated"):

petrochenkov commented Jan 10, 2017 • edited Loading

nikomatsakis commented Jan 10, 2017

jseyfried commented Jan 11, 2017 • edited Loading

nrc commented Jan 11, 2017

nikomatsakis commented Jan 17, 2017

petrochenkov commented Jan 17, 2017

petrochenkov commented Jan 20, 2017

bors commented Jan 21, 2017

petrochenkov commented Jan 21, 2017

bors commented Jan 22, 2017

jseyfried left a comment

Choose a reason for hiding this comment

jseyfried Jan 22, 2017

Choose a reason for hiding this comment

petrochenkov Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

nikomatsakis Jan 25, 2017

Choose a reason for hiding this comment

jseyfried Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

jseyfried Jan 22, 2017

Choose a reason for hiding this comment

petrochenkov Jan 23, 2017

Choose a reason for hiding this comment

nikomatsakis Jan 23, 2017

Choose a reason for hiding this comment

nikomatsakis commented Jan 23, 2017 • edited Loading

rfcbot commented Jan 23, 2017 • edited by michaelwoerister Loading

aturon commented Jan 23, 2017

sinesc commented Jan 24, 2017

rfcbot commented Jan 25, 2017

nikomatsakis commented Jan 25, 2017

jseyfried commented Jan 27, 2017

petrochenkov commented Jan 29, 2017

petrochenkov Jan 29, 2017

Choose a reason for hiding this comment

aturon commented Jan 31, 2017

petrochenkov commented Jan 31, 2017

bors commented Jan 31, 2017

bors commented Feb 1, 2017

bors commented Feb 1, 2017

petrochenkov commented Feb 1, 2017

bors commented Feb 1, 2017

bors commented Feb 1, 2017

petrochenkov commented Feb 1, 2017

bors commented Feb 1, 2017

bors commented Feb 1, 2017

petrochenkov commented Feb 1, 2017

alexcrichton commented Feb 1, 2017 via email

bors commented Feb 2, 2017

bors commented Feb 2, 2017

alexcrichton commented Feb 2, 2017 via email

bors commented Feb 2, 2017

bors commented Feb 2, 2017

petrochenkov commented Jan 8, 2017 •

edited

Loading

petrochenkov commented Jan 10, 2017 •

edited

Loading

jseyfried commented Jan 11, 2017 •

edited

Loading

petrochenkov Jan 23, 2017 •

edited

Loading

jseyfried Jan 27, 2017 •

edited

Loading

nikomatsakis commented Jan 23, 2017 •

edited

Loading

rfcbot commented Jan 23, 2017 •

edited by michaelwoerister

Loading