one-time diagnostics for private enum variants glob reëxport #46248

zackmdavis · 2017-11-24T22:16:44Z

estebank

Like the approach! Just a couple of nitpicks.

estebank · 2017-11-24T22:53:51Z

src/librustc_resolve/resolve_imports.rs

+                                                           DiagnosticMessageId::ErrorId(0),
+                                                           enum_def_start_span,
+                                                           "consider making the enum public",
+                                "pub ".to_owned());


Fix indentation.

Instead of using enum_def_start_span I'd use the codemap().def_span(enum_def_span) in order to have the following output:

18 | enum Professor { | -------------- help: consider making the enum public: `pub enum Professor`

I don't remember how the suggestion output looks when encountering multiline spans, like what would be caused by the following snippet of code:

enum Professor { Adjunct, Assistant, Associate, Full }

I don't remember how the suggestion output looks when encountering multiline spans

It's the thing that goes like

/ struct MyStruct { | ... | | } |__^

Not recommended here.

estebank · 2017-11-24T22:55:00Z

src/librustc_resolve/resolve_imports.rs

+                            let enum_def_span = directive.parent.resolutions.borrow()
+                                .get(&(enum_ident, TypeNS))
+                                .expect("enum resolution should exist")
+                                .borrow().binding


align the binding.expect in the same line (I personally like to make it visually easy to notice you can skip the expect when skimming).

estebank · 2017-11-24T22:55:20Z

src/librustc_resolve/resolve_imports.rs

                    }
-                }
+                },


This comma was not necessary.

estebank · 2017-11-24T22:55:25Z

src/librustc_resolve/resolve_imports.rs

@@ -891,7 +925,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
                    resolution.binding = Some(self.arenas.alloc_name_binding(NameBinding {
                        kind: NameBindingKind::Ambiguity { b1: b1, b2: b2, legacy: true }, ..*b1
                    }));
-                }
+                },


This comma was not necessary.

estebank · 2017-11-24T22:59:53Z

src/test/ui/resolve/issue-46209-private-enum-variant-reexport.stderr

+   |             ^^^^^^^^^^^^^^^^^^^
+...
+18 |     enum Professor {
+   |     - help: consider making the enum public: `pub `


Love this! (With the above nitpick)

estebank · 2017-11-24T23:09:28Z

src/test/ui/resolve/issue-46209-private-enum-variant-reexport.stderr

+  --> $DIR/issue-46209-private-enum-variant-reexport.rs:14:32
+   |
+14 |     pub use self::Lieutenant::{JuniorGrade, Full};
+   |                                ^^^^^^^^^^^


Ideally I would like these to follow the same output as #43447, but it could get hairy fairly quickly...

motion to defer

zackmdavis · 2017-11-26T03:52:07Z

Travis failure (of a revision that I'm about to force-push over) makes it look like we have some nondeterminism:

[00:46:10] -  --> $DIR/issue-46209-private-enum-variant-reexport.rs:14:45

[00:46:10] +  --> $DIR/issue-46209-private-enum-variant-reexport.rs:14:32

[00:46:10]     |

[00:46:10]  14 |     pub use self::Lieutenant::{JuniorGrade, Full};

[00:46:10] -   |                                             ^^^^

[00:46:10] +   |                                ^^^^^^^^^^^

[00:46:10]  ...

[00:46:10]  25 |     enum Lieutenant {

[00:46:10]     |     - help: consider making the enum public: `pub `

[00:46:10]  

[00:46:10]  error: variant is private and cannot be reexported

[00:46:10] -  --> $DIR/issue-46209-private-enum-variant-reexport.rs:14:32

[00:46:10] +  --> $DIR/issue-46209-private-enum-variant-reexport.rs:14:45

[00:46:10]     |

[00:46:10]  14 |     pub use self::Lieutenant::{JuniorGrade, Full};

[00:46:10] -   |                                ^^^^^^^^^^^

[00:46:10] +   |                                             ^^^^

I'd rather actually fix the nondeterminism rather than going for the lazy "fix" of only including one of the non-glob erroneous re-exports, because I want the UI test to show that the suggestion doesn't appear the second time (thanks to .diag_span_suggestion_once).

This is still rust-lang#46248.

zackmdavis · 2017-11-26T04:22:42Z

... wait, maybe I had to amend something the other night without updating the UI test (can't recall), and that's what threw off the hash ordering (but it would be stable with the same compiler/same code)?

zackmdavis · 2017-11-26T04:25:30Z

force-updated to address review comments, plus a new commit for correct suggestions in the restricted-but-not-entirely-private case (pub(crate)-alikes)

zackmdavis · 2017-11-27T03:20:29Z

Travis failure is ordering-related again; I guess we do need to stabilize it? I'm not sure what the hash depends on such that same-compiler-code/same-program/different-machine (mine vs. Travis) gives different results.

estebank · 2017-11-27T14:26:18Z

@zackmdavis the way to make the ui output completely stable is to defer the emission of the diagnostic until all of them have been processed and then sorter right before output. I wouldn't bother with this case in particular and move the test to compile-fail in order to avoid further complicating the code.

bors · 2017-12-02T07:50:28Z

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

zackmdavis · 2017-12-03T08:58:31Z

(well, this also turns out to have a more serious failure (compile-fail/private-variant-reexport.rs ICEs); I won't forget to take another look)

zackmdavis · 2017-12-05T05:31:01Z

Travis is green now

estebank

Couple of nitpicks, but it looks great and almost ready to merge. r=me after addressing the comments.

estebank · 2017-12-07T00:20:44Z

src/librustc_resolve/resolve_imports.rs

+                                ImportDirectiveSubclass::GlobImport { .. } => {
+                                    let msg = "enum is private and its variants \
+                                               cannot be reexported".to_owned();
+                                    let error_id = (DiagnosticMessageId::ErrorId(0), // no code?!


You can create a new code in https://github.com/rust-lang/rust/blob/master/src/librustc_resolve/diagnostics.rs (probably a good idea to do it as part of this PR)

I think there's a strong case to be made that this should just be E0364, rather than a new code.

I'd rather defer this to some future PR. (I started coding, but the pieces don't quite fit together for the elegant thing that I wanted to do.)

estebank · 2017-12-07T00:22:49Z

src/test/compile-fail/private-variant-reexport.rs

 }

 mod m2 {
-    pub use ::E::{V}; //~ ERROR variant `V` is private, and cannot be reexported
+    pub use ::E::{V}; //~ ERROR variant is private and cannot be reexported


What is the stderr output for this file? I want to make sure that it is pointing at the V path segment exclusively (here and elsewhere in this file), otherwise change the msg above to include the identifier as it was before to avoid confusion, as if the output points at the entire use statement (which is my understanding from reading the code) it could be referring to E or V (and it gets worse the more path segments there are).

It was

zmd@MaximumLikelihood:~/Code/rust$ ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/test/compile-fail/private-variant-reexport.rs error: variant is private and cannot be reexported --> src/test/compile-fail/private-variant-reexport.rs:12:13 | 12 | pub use ::E::V; //~ ERROR variant is private and cannot be reexported | ^^^^^^ ... 27 | enum E { V } | ------ help: consider making the enum public: `pub enum E` error: variant is private and cannot be reexported --> src/test/compile-fail/private-variant-reexport.rs:16:19 | 16 | pub use ::E::{V}; //~ ERROR variant is private and cannot be reexported | ^ error: variant is private and cannot be reexported --> src/test/compile-fail/private-variant-reexport.rs:20:22 | 20 | pub use ::E::V::{self}; //~ ERROR variant is private and cannot be reexported | ^^^^ error: enum is private and its variants cannot be reexported --> src/test/compile-fail/private-variant-reexport.rs:24:13 | 24 | pub use ::E::*; //~ ERROR enum is private and its variants cannot be reexported | ^^^^^^ error: aborting due to 4 previous errors

(Span is including the E:: for the non-brace case; can add variant name back into the message.)

304c8b1 made the Session's one-time-diagnostics set take a special-purpose `DiagnosticMessageId` enum rather than a LintID so that it could support more than just lints, but the `diag_span_note_once` and `diag_note_once` methods continued to take references to lints: for API consistency, we now make these methods take a `DiagnosticMessageId` while we add support for one-time span-suggestions.

We issue just one message for an erroneous glob private variant reëxport (using the Session's one-time-diagnostics capability), but individual (non-glob) such erroneous reëxports still get their own messages. The suggestion to make the enum public is also one-time. The enum variant reëxport error didn't have an associated error code (and remedying this here is deemed out of the scope of this commit), so we resort to the expediency of using 0 as the `DiagnosticMessageId` value. Adding Debug to NameResolution was helpful in development. This resolves rust-lang#46209.

zackmdavis · 2017-12-10T03:42:00Z

(updated)

estebank · 2017-12-10T21:40:14Z

@bors r+

bors · 2017-12-10T21:40:14Z

📌 Commit 4fb57e0 has been approved by estebank

bors · 2017-12-10T23:32:15Z

⌛ Testing commit 4fb57e0 with merge ea16814...

@estebank

…ort_error, r=estebank one-time diagnostics for private enum variants glob reëxport ![private_enum_reexport](https://user-images.githubusercontent.com/1076988/33224719-4e5805f0-d121-11e7-8bc0-a708a277a5db.png) r? @estebank

bors · 2017-12-11T02:04:51Z

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing ea16814 to master...

rust-highfive assigned estebank Nov 24, 2017

zackmdavis force-pushed the one_time_private_enum_variant_reexport_error branch from f352fb5 to 42d744e Compare November 24, 2017 22:47

estebank reviewed Nov 24, 2017

View reviewed changes

petrochenkov self-assigned this Nov 24, 2017

kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 25, 2017

zackmdavis force-pushed the one_time_private_enum_variant_reexport_error branch from 42d744e to e6033b6 Compare November 26, 2017 04:21

zackmdavis added a commit to zackmdavis/rust that referenced this pull request Nov 26, 2017

correct suggestion for restricted-if-not-fully-private reëxport error

e32419d

This is still rust-lang#46248.

zackmdavis force-pushed the one_time_private_enum_variant_reexport_error branch from e6033b6 to e32419d Compare November 26, 2017 04:23

petrochenkov removed their assignment Nov 26, 2017

zackmdavis force-pushed the one_time_private_enum_variant_reexport_error branch from 1bb63e3 to 155e852 Compare November 27, 2017 04:37

zackmdavis force-pushed the one_time_private_enum_variant_reexport_error branch from 155e852 to f1c3c3a Compare December 3, 2017 08:56

zackmdavis force-pushed the one_time_private_enum_variant_reexport_error branch 2 times, most recently from d1f6dc1 to b130d08 Compare December 5, 2017 03:56

estebank approved these changes Dec 7, 2017

View reviewed changes

zackmdavis added 2 commits December 9, 2017 16:33

zackmdavis force-pushed the one_time_private_enum_variant_reexport_error branch from b130d08 to 4fb57e0 Compare December 10, 2017 02:00

bors merged commit 4fb57e0 into rust-lang:master Dec 11, 2017

zackmdavis deleted the one_time_private_enum_variant_reexport_error branch May 22, 2018 04:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

one-time diagnostics for private enum variants glob reëxport #46248

one-time diagnostics for private enum variants glob reëxport #46248

zackmdavis commented Nov 24, 2017

estebank left a comment

estebank Nov 24, 2017

estebank Nov 24, 2017

zackmdavis Nov 26, 2017

estebank Nov 24, 2017

estebank Nov 24, 2017

estebank Nov 24, 2017

estebank Nov 24, 2017

estebank Nov 24, 2017

zackmdavis Nov 26, 2017

zackmdavis commented Nov 26, 2017

zackmdavis commented Nov 26, 2017

zackmdavis commented Nov 26, 2017

zackmdavis commented Nov 27, 2017

estebank commented Nov 27, 2017

bors commented Dec 2, 2017

zackmdavis commented Dec 3, 2017

zackmdavis commented Dec 5, 2017

estebank left a comment

estebank Dec 7, 2017

zackmdavis Dec 10, 2017

zackmdavis Dec 10, 2017

estebank Dec 7, 2017

zackmdavis Dec 10, 2017

zackmdavis commented Dec 10, 2017

estebank commented Dec 10, 2017

bors commented Dec 10, 2017

bors commented Dec 10, 2017

bors commented Dec 11, 2017

one-time diagnostics for private enum variants glob reëxport #46248

one-time diagnostics for private enum variants glob reëxport #46248

Conversation

zackmdavis commented Nov 24, 2017

estebank left a 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

Choose a reason for hiding this comment

zackmdavis commented Nov 26, 2017

zackmdavis commented Nov 26, 2017

zackmdavis commented Nov 26, 2017

zackmdavis commented Nov 27, 2017

estebank commented Nov 27, 2017

bors commented Dec 2, 2017

zackmdavis commented Dec 3, 2017

zackmdavis commented Dec 5, 2017

estebank left a 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

zackmdavis commented Dec 10, 2017

estebank commented Dec 10, 2017

bors commented Dec 10, 2017

bors commented Dec 10, 2017

bors commented Dec 11, 2017