Fix span of visibility #47799

topecongiro · 2018-01-27T07:28:16Z

This PR

adds a closing parenthesis to the span of Visibility::Crate (e.g. pub(crate)). The current span only covers pub(crate.
adds a span field to Visibility::Restricted. This span covers the entire visibility expression (e.g. pub (in self)). Currently all we can have is a span for Path.

This PR is motivated by the bug found in rustfmt (rust-lang/rustfmt#2398).

The first change is a strict improvement IMHO. The second change may not be desirable, as it adds a field which is currently not used by the compiler.

rust-highfive · 2018-01-27T07:28:28Z

r? @michaelwoerister

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

petrochenkov · 2018-01-27T12:21:10Z

IIRC, the whole visibility span is used in couple of places, but it's retrieved from codemap when necessary rather than precomputed.
I'd rather add a span field to Visibility itself, but it's a larger refactoring.

r=me without the second commit.
@bors delegate+

bors · 2018-01-27T12:21:11Z

✌️ @topecongiro can now approve this pull request

topecongiro · 2018-01-28T02:34:52Z

@petrochenkov Thank you for your review!

If adding a span filed to Visibility itself is ok, I would rather do that in this PR. I will see if I can do it.

topecongiro · 2018-01-28T02:53:12Z

Or, rather than adding a span field to Visibility, use Spanned? E.g.

pub type Visibility = Spanned<VisibilityKind>;

#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
pub enum VisibilityKind {
    Public,
    Crate(CrateSugar),
    Restricted { path: P<Path>, id: NodeId },
    Inherited,
}

petrochenkov · 2018-01-28T09:40:52Z

@topecongiro
The rule of thumb is that Spanned is used with enums because we cannot add fields to them directly and additional span field is used with structs, so Spanned would be okay in this case.

petrochenkov · 2018-01-29T20:52:08Z

src/librustc_allocator/expand.rs

@@ -97,7 +97,7 @@ impl<'a> Folder for ExpandAllocatorDirectives<'a> {
        ]);
        let mut items = vec![
            f.cx.item_extern_crate(f.span, f.alloc),
-            f.cx.item_use_simple(f.span, Visibility::Inherited, super_path),
+            f.cx.item_use_simple(f.span, dummy_spanned(VisibilityKind::Inherited), super_path),


Dummy spans should not be used, unless completely unavoidable.
In most cases during expansion spans for generated code can be inherited from something. In this case item.span is used for the path and for the extern crate item, it can be used for visibility as well.

Dummy spans should not be used, unless completely unavoidable.

It's probably not so dire for visibilities because they don't have hygiene, but it's still desirable to use some valid spans when possible.

@petrochenkov Thank you for your advice!

Is it acceptable to use 'empty' span like the following, rather than item.span? I think it makes easier to handle comments in rustfmt and reflects the syntactic structure of source code more accurately (as Inherited visibility has no keyword).

Span { hi: item.span.lo(), ..item.span }

Yes, that would be even better.
(I was concerned about item.span.lo pointing to the location before item attributes, but it looks like this is not the case.)

topecongiro · 2018-02-04T12:22:46Z

Updated and rebased against the master to resolve conflicts.

petrochenkov · 2018-02-04T15:37:03Z

r=me after removing unintended submodule updates and fixing tidy.
@bors delegate+

bors · 2018-02-04T15:37:04Z

✌️ @topecongiro can now approve this pull request

topecongiro · 2018-02-04T23:21:14Z

@bors r+

bors · 2018-02-04T23:21:14Z

📌 Commit 7d51391 has been approved by topecongiro

topecongiro · 2018-02-05T01:41:02Z

@bors r-

Had to fix tidy.

petrochenkov · 2018-02-05T08:47:43Z

Travis fails, unit tests in libsyntax need updating.

pietroalbini · 2018-02-12T10:39:23Z

Hi @topecongiro, we haven't heard from you on this in a while! There are some broken tests (#47799 (comment)), could you update them so the PR can be merged?

topecongiro · 2018-02-14T14:37:21Z

Updated broken tests. I am sorry that this took a long time to get fixed.

petrochenkov · 2018-02-19T21:27:11Z

Timeout on i686-pc-windows-msvc again! Strange, but let's try again.
@bors retry

bors · 2018-02-19T22:23:04Z

⌛ Testing commit 8e9fa57 with merge b2642ba62d41570201a9d85e7f4df133590e5fed...

@nrc

… r=petrochenkov Fixes rust-lang#47311. r? @nrc

Manishearth · 2018-02-20T00:58:19Z

@bors clean

(included in the rollup)

bors · 2018-02-20T02:23:03Z

💥 Test timed out

Manishearth · 2018-02-20T04:03:04Z

@bors r=petrochenkov retry clean

bors · 2018-02-20T04:03:06Z

💡 This pull request was already approved, no need to approve it again.

bors · 2018-02-20T04:03:06Z

📌 Commit 8e9fa57 has been approved by petrochenkov

bors · 2018-02-23T11:21:36Z

⌛ Testing commit 8e9fa57 with merge 063deba...

…nkov Fix span of visibility This PR 1. adds a closing parenthesis to the span of `Visibility::Crate` (e.g. `pub(crate)`). The current span only covers `pub(crate`. 2. adds a `span` field to `Visibility::Restricted`. This span covers the entire visibility expression (e.g. `pub (in self)`). Currently all we can have is a span for `Path`. This PR is motivated by the bug found in rustfmt (rust-lang/rustfmt#2398). The first change is a strict improvement IMHO. The second change may not be desirable, as it adds a field which is currently not used by the compiler.

bors · 2018-02-23T14:17:07Z

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

@Manishearth

Tested on commit rust-lang/rust@063deba. 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk).

kennytm · 2018-02-23T14:44:54Z

Clippy is broken by this PR, cc @Manishearth @llogiq @mcarton @oli-obk.

[01:21:49]    Compiling clippy_lints v0.0.186 (file:///checkout/src/tools/clippy/clippy_lints)
[01:21:56] error[E0599]: no associated item named `Public` found for type `syntax::codemap::Spanned<syntax::ast::VisibilityKind>` in the current scope
[01:21:56]    --> tools/clippy/clippy_lints/src/enum_variants.rs:263:36
[01:21:56]     |
[01:21:56] 263 |                     if item.vis == Visibility::Public {
[01:21:56]     |                                    ^^^^^^^^^^^^^^^^^^ associated item not found in `syntax::codemap::Spanned<syntax::ast::VisibilityKind>`
[01:21:56] 
[01:21:56] error[E0599]: no associated item named `Public` found for type `syntax::codemap::Spanned<syntax::ast::VisibilityKind>` in the current scope
[01:21:56]    --> tools/clippy/clippy_lints/src/enum_variants.rs:288:17
[01:21:56]     |
[01:21:56] 288 |                 Visibility::Public => PUB_ENUM_VARIANT_NAMES,
[01:21:56]     |                 ^^^^^^^^^^^^^^^^^^ associated item not found in `syntax::codemap::Spanned<syntax::ast::VisibilityKind>`
[01:21:56] 
[01:21:56] error[E0599]: no associated item named `Public` found for type `syntax::codemap::Spanned<syntax::ast::VisibilityKind>` in the current scope
[01:21:56]    --> tools/clippy/clippy_lints/src/enum_variants.rs:263:36
[01:21:56]     |
[01:21:56] 263 |                     if item.vis == Visibility::Public {
[01:21:56]     |                                    ^^^^^^^^^^^^^^^^^^ associated item not found in `syntax::codemap::Spanned<syntax::ast::VisibilityKind>`
[01:21:56] 
[01:21:56] error[E0599]: no associated item named `Public` found for type `syntax::codemap::Spanned<syntax::ast::VisibilityKind>` in the current scope
[01:21:56]    --> tools/clippy/clippy_lints/src/enum_variants.rs:288:17
[01:21:56]     |
[01:21:56] 288 |                 Visibility::Public => PUB_ENUM_VARIANT_NAMES,
[01:21:56]     |                 ^^^^^^^^^^^^^^^^^^ associated item not found in `syntax::codemap::Spanned<syntax::ast::VisibilityKind>`
[01:21:56] 
[01:21:59] error: aborting due to 2 previous errors
[01:21:59] 
[01:21:59] error: Could not compile `clippy_lints`.
[01:21:59] warning: build failed, waiting for other jobs to finish...
[01:21:59] error: aborting due to 2 previous errors

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.)

rust-highfive assigned michaelwoerister Jan 27, 2018

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 27, 2018

petrochenkov assigned petrochenkov and unassigned michaelwoerister Jan 27, 2018

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 Jan 27, 2018

petrochenkov reviewed Jan 29, 2018

View reviewed changes

topecongiro force-pushed the fix-span-of-visibility branch from 8dcbba9 to 93e87c4 Compare February 4, 2018 12:22

topecongiro force-pushed the fix-span-of-visibility branch from 93e87c4 to 7d51391 Compare February 4, 2018 23:16

topecongiro force-pushed the fix-span-of-visibility branch from 7d51391 to e4d4003 Compare February 5, 2018 01:38

topecongiro force-pushed the fix-span-of-visibility branch 2 times, most recently from 4bdf937 to 2e17f62 Compare February 5, 2018 03:59

topecongiro force-pushed the fix-span-of-visibility branch from 2e17f62 to 5ace5b6 Compare February 14, 2018 14:32

pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 14, 2018

bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 19, 2018

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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2018

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 20, 2018

Rollup merge of rust-lang#47799 - topecongiro:fix-span-of-visibility,…

bbea863

… r=petrochenkov Fixes rust-lang#47311. r? @nrc

Manishearth closed this Feb 20, 2018

Manishearth reopened this Feb 20, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 20, 2018

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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2018

bors merged commit 8e9fa57 into rust-lang:master Feb 23, 2018

topecongiro deleted the fix-span-of-visibility branch February 23, 2018 14:19

bors mentioned this pull request Feb 23, 2018

Anonymize some line numbers in UI test output #48449

Merged

ordovicia mentioned this pull request Feb 25, 2018

Fix for the latest libsyntax regexident/cargo-modules#13

Merged

pnkfelix mentioned this pull request Jun 1, 2018

powerpc-unknown-linux-gnu fails to compile simple crates #50960

Closed

petrochenkov mentioned this pull request Jun 28, 2018

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix span of visibility #47799

Fix span of visibility #47799

topecongiro commented Jan 27, 2018

rust-highfive commented Jan 27, 2018

petrochenkov commented Jan 27, 2018

bors commented Jan 27, 2018

topecongiro commented Jan 28, 2018

topecongiro commented Jan 28, 2018

petrochenkov commented Jan 28, 2018

petrochenkov Jan 29, 2018

petrochenkov Jan 29, 2018 •

edited

Loading

topecongiro Jan 29, 2018

petrochenkov Jan 29, 2018

topecongiro commented Feb 4, 2018

petrochenkov commented Feb 4, 2018

bors commented Feb 4, 2018

topecongiro commented Feb 4, 2018

bors commented Feb 4, 2018

topecongiro commented Feb 5, 2018

petrochenkov commented Feb 5, 2018

pietroalbini commented Feb 12, 2018

topecongiro commented Feb 14, 2018

petrochenkov commented Feb 19, 2018

bors commented Feb 19, 2018

Manishearth commented Feb 20, 2018

bors commented Feb 20, 2018

Manishearth commented Feb 20, 2018

bors commented Feb 20, 2018

bors commented Feb 20, 2018

bors commented Feb 23, 2018

bors commented Feb 23, 2018

kennytm commented Feb 23, 2018

Fix span of visibility #47799

Fix span of visibility #47799

Conversation

topecongiro commented Jan 27, 2018

rust-highfive commented Jan 27, 2018

petrochenkov commented Jan 27, 2018

bors commented Jan 27, 2018

topecongiro commented Jan 28, 2018

topecongiro commented Jan 28, 2018

petrochenkov commented Jan 28, 2018

petrochenkov Jan 29, 2018

Choose a reason for hiding this comment

petrochenkov Jan 29, 2018 • edited Loading

Choose a reason for hiding this comment

topecongiro Jan 29, 2018

Choose a reason for hiding this comment

petrochenkov Jan 29, 2018

Choose a reason for hiding this comment

topecongiro commented Feb 4, 2018

petrochenkov commented Feb 4, 2018

bors commented Feb 4, 2018

topecongiro commented Feb 4, 2018

bors commented Feb 4, 2018

topecongiro commented Feb 5, 2018

petrochenkov commented Feb 5, 2018

pietroalbini commented Feb 12, 2018

topecongiro commented Feb 14, 2018

petrochenkov commented Feb 19, 2018

bors commented Feb 19, 2018

Manishearth commented Feb 20, 2018

bors commented Feb 20, 2018

Manishearth commented Feb 20, 2018

bors commented Feb 20, 2018

bors commented Feb 20, 2018

bors commented Feb 23, 2018

bors commented Feb 23, 2018

kennytm commented Feb 23, 2018

petrochenkov Jan 29, 2018 •

edited

Loading