add modifier keyword spans to hir::Visibility; improve unreachable-pub, private-no-mangle lint suggestions #51866

zackmdavis · 2018-06-28T06:58:30Z

#50455 pointed out that the unreachable-pub suggestion for brace-grouped uses was bogus; #50476 partially ameliorated this by marking the suggestion as Applicability::MaybeIncorrect, but this is the actual fix.

Meanwhile, another application of having spans available in hir::Visibility is found in the private-no-mangle lints, where we can now issue a suggestion to use pub if the item has a more restricted visibility marker (this seems much less likely to come up in practice than not having any visibility keyword at all, but thoroughness is a virtue). While we're there, we can also add a helpful note if the item does have a pub (but triggered the lint presumably because enclosing modules were private).

r? @nrc
cc @Manishearth

rust-highfive · 2018-06-28T07:07:41Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:05:11] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:05:11] tidy error: /checkout/src/librustc/hir/lowering.rs:2795: line longer than 100 chars
[00:05:11] tidy error: /checkout/src/librustc/hir/lowering.rs:2868: line longer than 100 chars
[00:05:13] some tidy checks failed
[00:05:13] 
[00:05:13] 
[00:05:13] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:13] 
[00:05:13] 
[00:05:13] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:13] Build completed unsuccessfully in 0:01:56
[00:05:13] Build completed unsuccessfully in 0:01:56
[00:05:13] make: *** [tidy] Error 1
[00:05:13] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:17c069f0
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:0bbde356:start=1530169510026242143,finish=1530169510033485753,duration=7243610
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:11e70700
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:040d6926
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

rust-highfive · 2018-06-28T07:49:36Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:36:26]    Compiling aho-corasick v0.6.4
[00:36:33]    Compiling tempfile v3.0.2
[00:37:08]    Compiling minifier v0.0.14
[00:37:12]    Compiling rustdoc v0.0.0 (file:///checkout/src/librustdoc)
[00:37:13] error[E0532]: expected unit struct/variant or constant, found tuple variant `hir::Visibility::Public`
[00:37:13]     --> librustdoc/clean/mod.rs:3229:13
[00:37:13]      |
[00:37:13] 3229 |             hir::Visibility::Public => Visibility::Public,
[00:37:13]      |             ^^^^^^^^^^^^^^^^^^^^^^^ not a unit struct/variant or constant
[00:37:13] help: possible better candidates are found in other modules, you can import them into scope
[00:37:13] 14   | use clean::Visibility::Public;
[00:37:13]      |
[00:37:13]      |
[00:37:13] 14   | use minifier::js::Keyword::Public;
[00:37:13]      |
[00:37:13] 14   | use rustc::middle::privacy::AccessLevel::Public;
[00:37:13] 14   | use rustc::ty::Visibility::Public;
[00:37:13]      |
[00:37:13] and 1 other candidates
[00:37:13] 
[00:37:13] 
[00:37:17] error[E0308]: mismatched types
[00:37:17]    --> librustdoc/clean/mod.rs:289:36
[00:37:17]     |
[00:37:17] 289 |                     if item.vis == hir::Visibility::Public => {
[00:37:17]     |                                    ^^^^^^^^^^^^^^^^^^^^^^^ expected enum `rustc::hir::Visibility`, found fn item
[00:37:17]     = note: expected type `rustc::hir::Visibility`
[00:37:17]     = note: expected type `rustc::hir::Visibility`
[00:37:17]                found type `fn(syntax_pos::Span) -> rustc::hir::Visibility {rustc::hir::Visibility::Public}`
[00:37:17] error[E0308]: mismatched types
[00:37:17]    --> librustdoc/clean/mod.rs:331:36
[00:37:17]     |
[00:37:17]     |
[00:37:17] 331 |                     if item.vis == hir::Visibility::Public => {
[00:37:17]     |                                    ^^^^^^^^^^^^^^^^^^^^^^^ expected enum `rustc::hir::Visibility`, found fn item
[00:37:17]     = note: expected type `rustc::hir::Visibility`
[00:37:17]     = note: expected type `rustc::hir::Visibility`
[00:37:17]                found type `fn(syntax_pos::Span) -> rustc::hir::Visibility {rustc::hir::Visibility::Public}`
[00:37:20] error[E0023]: this pattern has 1 field, but the corresponding tuple variant has 2 fields
[00:37:20]     --> librustdoc/clean/mod.rs:3231:13
[00:37:20]      |
[00:37:20]      |
[00:37:20] 3231 |             hir::Visibility::Crate(_) => Visibility::Crate,
[00:37:20] 
[00:37:22] error[E0308]: mismatched types
[00:37:22]     --> librustdoc/clean/mod.rs:3935:34
[00:37:22]      |
[00:37:22]      |
[00:37:22] 3935 |         let denied = self.vis != hir::Public || self.attrs.iter().any(|a| {
[00:37:22]      |                                  ^^^^^^^^^^^ expected enum `rustc::hir::Visibility`, found fn item
[00:37:22]      = note: expected type `rustc::hir::Visibility`
[00:37:22]      = note: expected type `rustc::hir::Visibility`
[00:37:22]                 found type `fn(syntax_pos::Span) -> rustc::hir::Visibility {rustc::hir::Visibility::Public}`
[00:37:29] error[E0308]: mismatched types
[00:37:29]   --> librustdoc/visit_ast.rs:97:47
[00:37:29]    |
[00:37:29] 97 |                                               hir::Public,
[00:37:29] 97 |                                               hir::Public,
[00:37:29]    |                                               ^^^^^^^^^^^ expected enum `rustc::hir::Visibility`, found fn item
[00:37:29]    = note: expected type `rustc::hir::Visibility`
[00:37:29]    = note: expected type `rustc::hir::Visibility`
[00:37:29]               found type `fn(syntax_pos::Span) -> rustc::hir::Visibility {rustc::hir::Visibility::Public}`
[00:37:29] error[E0308]: mismatched types
[00:37:29]    --> librustdoc/visit_ast.rs:207:43
[00:37:29]     |
[00:37:29]     |
[00:37:29] 207 |         self.inside_public_path &= vis == hir::Public;
[00:37:29]     |                                           ^^^^^^^^^^^ expected enum `rustc::hir::Visibility`, found fn item
[00:37:29]     = note: expected type `rustc::hir::Visibility`
[00:37:29]     = note: expected type `rustc::hir::Visibility`
[00:37:29]                found type `fn(syntax_pos::Span) -> rustc::hir::Visibility {rustc::hir::Visibility::Public}`
[00:37:30] error[E0308]: mismatched types
[00:37:30]    --> librustdoc/visit_ast.rs:379:24
[00:37:30]     |
[00:37:30]     |
[00:37:30] 379 |         if item.vis == hir::Public {
[00:37:30]     |                        ^^^^^^^^^^^ expected enum `rustc::hir::Visibility`, found fn item
[00:37:30]     = note: expected type `rustc::hir::Visibility`
[00:37:30]     = note: expected type `rustc::hir::Visibility`
[00:37:30]                found type `fn(syntax_pos::Span) -> rustc::hir::Visibility {rustc::hir::Visibility::Public}`
[00:37:31] error[E0308]: mismatched types
[00:37:31]    --> librustdoc/visit_ast.rs:390:68
[00:37:31]     |
[00:37:31]     |
[00:37:31] 390 |                         items: fm.items.iter().filter(|i| i.vis == hir::Public).cloned().collect(),
[00:37:31]     |                                                                    ^^^^^^^^^^^ expected enum `rustc::hir::Visibility`, found fn item
[00:37:31]     = note: expected type `rustc::hir::Visibility`
[00:37:31]     = note: expected type `rustc::hir::Visibility`
[00:37:31]                found type `fn(syntax_pos::Span) -> rustc::hir::Visibility {rustc::hir::Visibility::Public}`
[00:37:31] error[E0308]: mismatched types
[00:37:31]    --> librustdoc/visit_ast.rs:397:47
[00:37:31]     |
[00:37:31]     |
[00:37:31] 397 |             _ if self.inlining && item.vis != hir::Public => {}
[00:37:31]     |                                               ^^^^^^^^^^^ expected enum `rustc::hir::Visibility`, found fn item
[00:37:31]     = note: expected type `rustc::hir::Visibility`
[00:37:31]     = note: expected type `rustc::hir::Visibility`
[00:37:31]                found type `fn(syntax_pos::Span) -> rustc::hir::Visibility {rustc::hir::Visibility::Public}`
[00:37:32] error[E0308]: mismatched types
[00:37:32]    --> librustdoc/visit_ast.rs:417:32
[00:37:32]     |
[00:37:32]     |
[00:37:32] 417 |                 if item.vis == hir::Public && self.inside_public_path {
[00:37:32]     |                                ^^^^^^^^^^^ expected enum `rustc::hir::Visibility`, found fn item
[00:37:32]     = note: expected type `rustc::hir::Visibility`
[00:37:32]     = note: expected type `rustc::hir::Visibility`
[00:37:32]                found type `fn(syntax_pos::Span) -> rustc::hir::Visibility {rustc::hir::Visibility::Public}`
[00:37:32] error: aborting due to 11 previous errors
[00:37:32] 
[00:37:32] Some errors occurred: E0023, E0308, E0532.
[00:37:32] For more information about an error, try `rustc --explain E0023`.
---
[00:37:32] 
[00:37:32] 
[00:37:32] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:37:32] Build completed unsuccessfully in 0:32:41
[00:37:32] Makefile:28: recipe for target 'all' failed
[00:37:32] make: *** [all] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:2ace9bf3
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:00278a2a:start=1530172171413818708,finish=1530172171421374024,duration=7555316
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:01e0dc46
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:3172769a
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

petrochenkov · 2018-06-28T08:28:25Z

src/librustc/hir/mod.rs

-    Restricted { path: P<Path>, id: NodeId },
+    Public(Span),
+    Crate(Span, CrateSugar),
+    Restricted { span: Span, path: P<Path>, id: NodeId },


Could you mirror what #47799 did with ast::Visibility instead?

petrochenkov · 2018-06-28T08:35:49Z

src/test/ui/lint/suggestions.stderr

@@ -89,12 +89,16 @@ warning: static is marked #[no_mangle], but not exported
   |
 LL |     #[no_mangle] pub static DAUNTLESS: bool = true;
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: check that enclosing modules are public


This is hardly a realistic advice, making a module pub requires changing the whole crate structure.
More local fix is to export the fn/static from the crate with pub use.

Manishearth

lgtm from my side, though petrochenkov's question about the validity of the export suggestion may need handling

Manishearth · 2018-06-28T10:46:15Z

src/librustc_lint/builtin.rs

+                hir::Visibility::Public(_) => None
+            };
+            if let Some((replace_span, replacement)) = suggestion {
+                err.span_suggestion(replace_span, "try making it public", replacement);


Mark this MaybeIncorrect?

(such changes likely need review, hence it's not MachineApplicable)

petrochenkov · 2018-06-28T14:31:47Z

src/librustc/hir/mod.rs

    Inherited,
 }

 impl Visibility {
+    pub fn is_pub(&self) -> bool {
+        use self::Visibility::*;


Nit: Please avoid enum imports and use Enum::Variant, this really improves code searchability in rustc.

nrc · 2018-06-29T03:46:47Z

r? @petrochenkov

bors · 2018-06-30T11:30:19Z

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

Visibility spans were added to the AST in rust-lang#47799 (d6bdf29) as a `Spanned<_>`—which means that we need to choose a span even in the case of inherited visibility (what you get when there's no `pub` &c. keyword at all). That initial implementation's choice is pretty counterintuitive, which could matter if we want to use it as a site to suggest inserting a visibility modifier, &c. (The phrase "Schelling span" in the comment is meant in analogy to the game-theoretic concept of a "Schelling point", a value that is chosen simply because it's what one can expect to agree upon with other agents in the absence of explicit coördination.)

There are at least a couple (and plausibly even three) diagnostics that could use the spans of visibility modifiers in order to be reliably correct (rather than hacking and munging surrounding spans to try to infer where the visibility keyword must have been). We follow the naming convention established by the other `Spanned` HIR nodes: the "outer" type alias gets the "prime" node-type name, the "inner" enum gets the name suffixed with an underscore, and the variant names are prefixed with the prime name and `pub use` exported from here (from HIR). Thanks to veteran reviewer Vadim Petrochenkov for suggesting this uniform approach. (A previous draft, based on the reasoning that `Visibility::Inherited` should not have a span, tried to hack in a named `span` field on `Visibility::Restricted` and a positional field on `Public` and `Crate`. This was ... not so uniform.)

This is a true fix for rust-lang#50455, superior to the mere bandage offered in rust-lang#50476.

This is probably quite a lot less likely to come up in practice than the "inherited" (no visibility keyword) case, but now that we have visibility spans in the HIR, we can do this, and it presumably doesn't hurt to be exhaustive. (Who can say but that the attention to detail just might knock someone's socks off, someday, somewhere?) This is inspired by rust-lang#47383.

If the item is `pub`, one imagines users being confused as to why it's not reachable/exported; a code suggestion is beyond our local knowledge here, but we can at least offer a prose hint. (Thanks to Vadim Petrochenkov for shooting down the present author's original bad idea for the note text.) While we're here, use proper HELP expectations instead of ad hoc comments to communicate (and now, enforce) the expected suggestions in test/ui/lint/suggestions.rs.

April 2016's Issue rust-lang#33174 called out the E0446 diagnostics as confusing. While adding the name of the restricted type to the message (548e681) clarified matters somewhat, Esteban Küber pointed out that we could stand to place a secondary span on the restricted type. Here, we differentiate between crate-visible, truly private, and otherwise restricted types, and place a secondary span specifically on the visibility modifier of the restricted type's declaration (which we can do now that HIR visibilities have spans!). At long last, this resolves rust-lang#33174.

petrochenkov · 2018-07-01T09:18:27Z

src/librustc/hir/mod.rs

+    VisibilityPublic,
+    VisibilityCrate(CrateSugar),
+    VisibilityRestricted { path: P<Path>, id: NodeId },
+    VisibilityInherited,


The convention enum Something_ { SomethingVariant } is "deprecated" and exists in HIR only because not all HIR structures were updated to the "modern" form enum SomethingKind { Variant } like it was done in AST.

New structures should not use it, so it'd be better to rename Visibility_ to VisibilityKind.

understood; filed #51968 for the other HIR structures

petrochenkov · 2018-07-01T09:19:39Z

r=me after renaming Visibility_ -> VisibilityKind.

It was pointed out in review that the glob-exported underscore-suffixed convention for `Spanned` HIR nodes is no longer preferred: see February 2016's rust-lang#31487 for AST's migration away from this style towards properly namespaced NodeKind enums. This concerns rust-lang#51968.

zackmdavis · 2018-07-01T20:31:58Z

@petrochenkov changed to VisibilityKind. (Also, not mentioned in previous comments, but last night's rebase also included a new commit that adds a secondary span to address #33174.)

estebank · 2018-07-02T00:27:52Z

@bors r=petrochenkov

bors · 2018-07-02T00:27:53Z

📌 Commit 43a0a65 has been approved by petrochenkov

bors · 2018-07-02T03:18:55Z

⌛ Testing commit 43a0a65 with merge 4593564...

@nrc

…petrochenkov add modifier keyword spans to hir::Visibility; improve unreachable-pub, private-no-mangle lint suggestions #50455 pointed out that the unreachable-pub suggestion for brace-grouped `use`s was bogus; #50476 partially ameliorated this by marking the suggestion as `Applicability::MaybeIncorrect`, but this is the actual fix. Meanwhile, another application of having spans available in `hir::Visibility` is found in the private-no-mangle lints, where we can now issue a suggestion to use `pub` if the item has a more restricted visibility marker (this seems much less likely to come up in practice than not having any visibility keyword at all, but thoroughness is a virtue). While we're there, we can also add a helpful note if the item does have a `pub` (but triggered the lint presumably because enclosing modules were private). ![hir_vis](https://user-images.githubusercontent.com/1076988/42018064-ca830290-7a65-11e8-9c4c-48bc846f861f.png) r? @nrc cc @Manishearth

bors · 2018-07-02T05:19:22Z

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

rust-highfive assigned nrc Jun 28, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 28, 2018

zackmdavis force-pushed the hir_making_each_day_of_the_year branch from 5e8b1c8 to 4b68507 Compare June 28, 2018 07:10

petrochenkov reviewed Jun 28, 2018

View reviewed changes

petrochenkov assigned petrochenkov and unassigned nrc Jun 28, 2018

Manishearth approved these changes Jun 28, 2018

View reviewed changes

petrochenkov reviewed Jun 28, 2018

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 28, 2018

zackmdavis added 3 commits June 30, 2018 22:20

unreachable_pub lint: grab pub span from HIR rather than inferring it

104985b

This is a true fix for rust-lang#50455, superior to the mere bandage offered in rust-lang#50476.

zackmdavis force-pushed the hir_making_each_day_of_the_year branch from 4b68507 to 52835b5 Compare July 1, 2018 05:42

zackmdavis added 3 commits June 30, 2018 22:47

zackmdavis force-pushed the hir_making_each_day_of_the_year branch from 52835b5 to c2d44b2 Compare July 1, 2018 05:48

petrochenkov reviewed Jul 1, 2018

View reviewed changes

petrochenkov mentioned this pull request Jul 1, 2018

Remove most of PartialEq and Hash impls from AST and HIR structures #51829

Merged

zackmdavis mentioned this pull request Jul 1, 2018

spanned HIR node enums should be named FooKind rather than being named Foo_ and glob-exported #51968

Closed

4 tasks

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 Jul 2, 2018

nrc added the A-edition-2018-lints Area: Lints supporting the 2018 edition label Jul 2, 2018

bors merged commit 43a0a65 into rust-lang:master Jul 2, 2018

zackmdavis deleted the hir_making_each_day_of_the_year branch July 2, 2018 05:20

nrc mentioned this pull request Jul 4, 2018

2018 edition lints #52047

Closed

15 tasks

zackmdavis mentioned this pull request Aug 18, 2018

Improve "private type in public interface" message #50430

Open

ehuss mentioned this pull request Dec 20, 2018

cargo fix fails to apply all -Wunreachable-pub hints rust-lang/cargo#6465

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add modifier keyword spans to hir::Visibility; improve unreachable-pub, private-no-mangle lint suggestions #51866

add modifier keyword spans to hir::Visibility; improve unreachable-pub, private-no-mangle lint suggestions #51866

zackmdavis commented Jun 28, 2018

rust-highfive commented Jun 28, 2018

rust-highfive commented Jun 28, 2018

petrochenkov Jun 28, 2018

petrochenkov Jun 28, 2018

Manishearth left a comment

Manishearth Jun 28, 2018

petrochenkov Jun 28, 2018

nrc commented Jun 29, 2018

bors commented Jun 30, 2018

petrochenkov Jul 1, 2018

zackmdavis Jul 1, 2018

petrochenkov commented Jul 1, 2018

zackmdavis commented Jul 1, 2018

estebank commented Jul 2, 2018

bors commented Jul 2, 2018

bors commented Jul 2, 2018

bors commented Jul 2, 2018

add modifier keyword spans to hir::Visibility; improve unreachable-pub, private-no-mangle lint suggestions #51866

add modifier keyword spans to hir::Visibility; improve unreachable-pub, private-no-mangle lint suggestions #51866

Conversation

zackmdavis commented Jun 28, 2018

rust-highfive commented Jun 28, 2018

rust-highfive commented Jun 28, 2018

petrochenkov Jun 28, 2018

Choose a reason for hiding this comment

petrochenkov Jun 28, 2018

Choose a reason for hiding this comment

Manishearth left a comment

Choose a reason for hiding this comment

Manishearth Jun 28, 2018

Choose a reason for hiding this comment

petrochenkov Jun 28, 2018

Choose a reason for hiding this comment

nrc commented Jun 29, 2018

bors commented Jun 30, 2018

petrochenkov Jul 1, 2018

Choose a reason for hiding this comment

zackmdavis Jul 1, 2018

Choose a reason for hiding this comment

petrochenkov commented Jul 1, 2018

zackmdavis commented Jul 1, 2018

estebank commented Jul 2, 2018

bors commented Jul 2, 2018

bors commented Jul 2, 2018

bors commented Jul 2, 2018