Use `DefId`s instead of `NodeId`s for `pub(restricted)` visibilities #38490

jseyfried · 2016-12-20T09:21:36Z

This is groundwork for hygiene 2.0, specifically privacy checking hygienic intercrate name resolutions.
r? @nrc

jseyfried · 2016-12-20T09:24:00Z

src/librustc_metadata/schema.rs

@@ -200,7 +200,7 @@ pub struct TraitImpls {
 #[derive(RustcEncodable, RustcDecodable)]
 pub struct Entry<'tcx> {
    pub kind: EntryKind<'tcx>,
-    pub visibility: ty::Visibility,
+    pub visibility: Lazy<ty::Visibility>,


I had to change this to Lazy so that we don't try to decode a DefId before we have access to the cnum_map (causing a panic).

cc @eddyb

This makes sense, I only had it like that because it was always just an enum discriminant IIRC.

petrochenkov · 2016-12-20T13:29:37Z

privacy checking hygienic intercrate name resolutions

What is privacy checking hygienic intercrate name resolutions exactly? Is there any pre-RFC or other text in work?

jseyfried · 2016-12-21T01:31:05Z

@petrochenkov (cc @nrc)

What is privacy checking hygienic intercrate name resolutions exactly?

For example,

mod foo {
    pub(super) fn bar() {} // (i)
    fn baz() {} // (ii)

    fn f() { super::m!(); } // (example invocation of `m`)
}

pub macro m() {
    foo::bar(); // This should always resolve to (i), regardless of where `m` is invoked.
    foo::baz(); // This should always be a privacy error, regardless of where `m` is invoked.
}

More generally, consider a macro

pub macro m($e:expr) {
    ... foo ... $e ... bar ...  //< definition body
}

and an invocation of that macro path::to::m!(/* invocation body */);.

I think hygiene should provide the following dual guarantees:

The resolution of an ident in the definition body (e.g. foo, bar) does not depend on
- where the macro is invoked, or
- what expression the macro is invoked with.
The resolution of an ident in the invocation body does not depend on
- where the macro is defined (this is already true today), or
- the names of the idents in the definition body (e.g. replacing bar with baz in the definition body could not effect the resolution of the invocation body).

Is there any pre-RFC or other text in work?

Just assuming these guarantees almost entirely specifies hygiene 2.0, including hygiene for items, imports, methods, fields, privacy, etc.

I'm working on a prototype of hygiene 2.0 that will satisfy these guarantees (modulo an escape hatch). The prototype should be ready next week, and it will include a comprehensive suite of tests/examples as well as some more exposition and motivation.

bors · 2016-12-21T02:52:38Z

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

nrc

LGTM, other than the rename

nrc · 2016-12-21T02:51:04Z

src/librustc/ty/mod.rs

    /// Not visible anywhere in the local crate. This is the visibility of private external items.
-    PrivateExternal,
+    Empty,


I'm not sure about this rename - why does Empty make sense for a visibility?

How about Invisible?

petrochenkov · 2016-12-21T21:32:03Z

@jseyfried
I see, thanks, that's what I expected.
These rules completely break private-in-public guarantees though.

// One module
struct Private;

pub macro m() {
    Private // <- ok, the name is resolved, path privacy check passes
}

// Other module
let secret = m!(); // <- object of a private type is walking freely outside of its module

Macros don't have interfaces to check them for private types.
This may still be solvable by using type privacy checks (see #34537 (comment) and below, cc @eddyb), however traces of macro expansion will need to be kept through all compilation until trans (ugh).
Privacy checks performed lately (fields, methods) are also a problem.

jseyfried · 2016-12-22T03:27:10Z

@petrochenkov

These rules completely break private-in-public guarantees though.

Good point. I think the best way forward here is to fix #30476, make the fix hygienic in the prototype, and then assess what further restrictions we need for macros.

I believe we can fix #30476 by improving privacy checks for method calls. For example, I think [].arr0_secret(); from #30476 (comment) should be a privacy error.
arr0_secret resolves to <[Priv; 0] as Arr0>::arr0_secret, which has type fn(&[Priv; 0]), so to satisfy the private-in-public guarantees we should consider its visibility to be pub(m), not pub(crate).

EDIT: We would also have to similarly improve privacy checks for uses of generic functions that monomorphize into less visible types.

Is this the solution you had in mind?

If we fix #30476 as I am proposing, it would still be possible to get a private type from a macro (e.g. let secret = m!(); from #38490 (comment)), but it would be impossible to use the private type, except by passing it into another macro whose definition can see the private type. This might be acceptable.

If we want to also forbid getting a private type from a macro (even given that the only thing we can do with it is pass it into another macro), we could check that

if an invocation is in an expression position, the type of its expansion must be visible to the invoker
- this might be a little tricky to apply to eager expansion (Eager expansion of macros rfcs#1628), depending on the specifics of the eager expansion design
each local variable is not more visible than its type
- we can think of this as generalizing the private-in-public restriction to locals as well as items.

Privacy checks performed lately (fields, methods) are also a problem

In what way?
I've finished implementing most late privacy checks in the prototype, it's easier than it sounds :)

petrochenkov · 2016-12-22T14:11:53Z

@jseyfried

Privacy checks performed lately (fields, methods) are also a problem

In what way?
I've finished implementing most late privacy checks in the prototype, it's easier than it sounds :)

I haven't found the prototype you're mentioning, https://github.com/jseyfried/rust/tree/hygiene looks similar, but incomplete.
Privacy checks done entirely during type checking (inherent methods, fields, including privacy-skipping autoderef) will need to know macro definition/expansion contexts, which I dislike a lot.
Private stuff in macros interferes with linkage as well, reachability pass will have to know about macros as well to "externalize" macro-used private symbols similarly to how it's done with inline functions. In sum, this privacy scheme opens the door for heavy leakage of macros into later passes.
EDIT: There are many interesting cases in early path resolution alone, e.g. how to resolve and privacy-check

macro m($prefix: ident, $suffix: ident) {
    let a = $prefix::suffix;
    let b = prefix::$suffix;
}

My primary message is that all this stuff needs some text to discuss before jumping into implementation (but I'm still all for prototyping, groundwork, etc).

jseyfried · 2016-12-22T22:26:26Z

@petrochenkov

I haven't found the prototype you're mentioning

Right, I haven't made the late privacy checks public yet; they'll be in that branch by next week.

including privacy-skipping autoderef

What do you mean by "privacy-skipping autoderef"? Autoderef has taken privacy into account since #31938.

Privacy checks done entirely during type checking (inherent methods, fields, including privacy-skipping autoderef) will need to know macro definition/expansion contexts, which I dislike a lot.

Hmm, I'm not sure that's such a bad thing. Without it, we wouldn't be able to support e.g.

mod foo {
    struct S;
    impl S {
        fn f(&self) {}
    }
    pub macro m() {
        S.f();
    }
}
... foo::m!(); ...

which would break hygiene, at least according to the definition from #38490 (comment).

Private stuff in macros interferes with linkage as well, reachability pass will have to know about macros as well to "externalize" macro-used private symbols similarly to how it's done with inline functions.

Right, I think this is worth it. It will certainly need more discussion though, including potential compromises to simplify implementation or avoid exporting too many symbols.

My primary message is that all this stuff needs some text to discuss before jumping into implementation

Agreed, perhaps it was premature to label this PR "groundwork for hygiene 2.0" before we agree that we actually want hygienic privacy for intercrate name resolutions. I think this is a better way to implement visibility anyway though.

petrochenkov · 2016-12-22T22:54:33Z

What do you mean by "privacy-skipping autoderef"?

Just bad wording, I meant autoderef skipping inaccessible fields.

Hmm, I'm not sure that's such a bad thing. Without it, we wouldn't be able to support e.g. ... which would break hygiene, at least according to the definition from #38490 (comment).

It moves macros further and further away from the "purely syntactic" ideal, manually expanded code becomes less and less self-sufficient, compilation stages become more and more intermingled - first macros leaked into resolve, now they start leaking into type checking. I agree it may worth in the end if macros turn into some non-syntactic, but self-consistent (and practical) feature, something like fancy inline functions.

jseyfried · 2016-12-22T23:21:30Z

It moves macros further and further away from the "purely syntactic" ideal

I don't think this is the ideal, since it precludes any hygiene at all (unless I'm misunderstanding) -- even hygienic local variables require "leakage" into resolve.

I agree it may worth in the end if macros turn into some non-syntactic, but self-consistent (and practical) feature, something like fancy inline functions.

This sounds close to my vision of fully hygienic macros 2.0, but I wouldn't call it "non-syntactic" (maybe "epi-syntactic", meaning syntax plus hygiene info). For example, procedural macros operate on tokens but the tokens have normative hygiene information attached (HygieneObject from https://github.com/rust-lang/rfcs/blob/master/text/1566-proc-macros.md#tokens).

bors · 2016-12-23T09:12:32Z

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

jseyfried · 2016-12-24T00:26:06Z

@bors r=nrc

bors · 2016-12-24T00:26:07Z

📌 Commit 41f1e18 has been approved by nrc

bors · 2016-12-24T03:11:16Z

⌛ Testing commit 41f1e18 with merge cf0157a...

@nrc

Use `DefId`s instead of `NodeId`s for `pub(restricted)` visibilities This is groundwork for hygiene 2.0, specifically privacy checking hygienic intercrate name resolutions. r? @nrc

bors · 2016-12-24T13:28:22Z

💥 Test timed out

eddyb · 2016-12-24T14:04:56Z

@bors retry

bors · 2016-12-25T21:32:29Z

⌛ Testing commit 41f1e18 with merge 44ad63e...

@nrc

Use `DefId`s instead of `NodeId`s for `pub(restricted)` visibilities This is groundwork for hygiene 2.0, specifically privacy checking hygienic intercrate name resolutions. r? @nrc

bors · 2016-12-26T00:47:32Z

rust-highfive assigned nrc Dec 20, 2016

jseyfried commented Dec 20, 2016

View reviewed changes

jseyfried force-pushed the def_id_vis branch from a7d0c54 to a85266d Compare December 20, 2016 22:14

nrc approved these changes Dec 21, 2016

View reviewed changes

jseyfried closed this Dec 22, 2016

jseyfried reopened this Dec 22, 2016

jseyfried force-pushed the def_id_vis branch 3 times, most recently from 7e2c51b to 95285af Compare December 22, 2016 06:55

Use DefIds instead of NodeIds for pub(restricted) visibilities.

41f1e18

jseyfried force-pushed the def_id_vis branch from 95285af to 41f1e18 Compare December 24, 2016 00:23

bors merged commit 41f1e18 into rust-lang:master Dec 26, 2016

bors mentioned this pull request Dec 26, 2016

More systematic error reporting in path resolution #38154

Merged

jseyfried deleted the def_id_vis branch December 26, 2016 01:13

Mark-Simulacrum mentioned this pull request Jun 7, 2018

Missing tokens in custom derive input generated by macro #51331

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `DefId`s instead of `NodeId`s for `pub(restricted)` visibilities #38490

Use `DefId`s instead of `NodeId`s for `pub(restricted)` visibilities #38490

jseyfried commented Dec 20, 2016

jseyfried Dec 20, 2016

eddyb Dec 20, 2016

petrochenkov commented Dec 20, 2016

jseyfried commented Dec 21, 2016

bors commented Dec 21, 2016

nrc left a comment

nrc Dec 21, 2016

jseyfried Dec 21, 2016

nrc Dec 21, 2016

petrochenkov commented Dec 21, 2016

jseyfried commented Dec 22, 2016 •

edited

Loading

petrochenkov commented Dec 22, 2016 •

edited

Loading

jseyfried commented Dec 22, 2016

petrochenkov commented Dec 22, 2016 •

edited by jseyfried

Loading

jseyfried commented Dec 22, 2016

bors commented Dec 23, 2016

jseyfried commented Dec 24, 2016

bors commented Dec 24, 2016

bors commented Dec 24, 2016

bors commented Dec 24, 2016

eddyb commented Dec 24, 2016

bors commented Dec 25, 2016

bors commented Dec 26, 2016

Use DefIds instead of NodeIds for pub(restricted) visibilities #38490

Use DefIds instead of NodeIds for pub(restricted) visibilities #38490

Conversation

jseyfried commented Dec 20, 2016

jseyfried Dec 20, 2016

Choose a reason for hiding this comment

eddyb Dec 20, 2016

Choose a reason for hiding this comment

petrochenkov commented Dec 20, 2016

jseyfried commented Dec 21, 2016

bors commented Dec 21, 2016

nrc left a comment

Choose a reason for hiding this comment

nrc Dec 21, 2016

Choose a reason for hiding this comment

jseyfried Dec 21, 2016

Choose a reason for hiding this comment

nrc Dec 21, 2016

Choose a reason for hiding this comment

petrochenkov commented Dec 21, 2016

jseyfried commented Dec 22, 2016 • edited Loading

petrochenkov commented Dec 22, 2016 • edited Loading

jseyfried commented Dec 22, 2016

petrochenkov commented Dec 22, 2016 • edited by jseyfried Loading

jseyfried commented Dec 22, 2016

bors commented Dec 23, 2016

jseyfried commented Dec 24, 2016

bors commented Dec 24, 2016

bors commented Dec 24, 2016

bors commented Dec 24, 2016

eddyb commented Dec 24, 2016

bors commented Dec 25, 2016

bors commented Dec 26, 2016

Use `DefId`s instead of `NodeId`s for `pub(restricted)` visibilities #38490

Use `DefId`s instead of `NodeId`s for `pub(restricted)` visibilities #38490

jseyfried commented Dec 22, 2016 •

edited

Loading

petrochenkov commented Dec 22, 2016 •

edited

Loading

petrochenkov commented Dec 22, 2016 •

edited by jseyfried

Loading