Implement tool_attributes feature (RFC 2103) #47773

topecongiro · 2018-01-26T11:22:55Z

This PR implements a tool_attributes feature. If this implementaion is acceptable, I am going to create a similar PR that implements tool_lints.

This PR only adds rustfmt and clippy to known tool name list. The list resides in libsyntax/attr.rs.

cc #44690.

r? @nrc

topecongiro · 2018-01-26T13:02:56Z

Hmm. Should I add the section to the unstable book after this PR is merged, or am I missing something? I followed the instruction in https://forge.rust-lang.org/feature-guide.html.

kennytm · 2018-01-26T14:27:11Z

@topecongiro The problem is that unstable book file should be written in hyphenated lower case. Try to rename it as tool-attributes.md.

topecongiro · 2018-01-26T20:12:30Z

@kennytm Thank you!

petrochenkov · 2018-01-27T13:53:44Z

src/libsyntax/ast.rs

 #[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
 pub struct MetaItem {
-    pub name: Name,
+    pub name: MetaItemName,
    pub node: MetaItemKind,
    pub span: Span,


Contents of ast::Attribute and ast::MetaItem are the same thing by definition, but in the compiler they diverged significantly in recent months.
An attribute is a "macro invocation" that consists of a Path and a TokenStream, MetaItem should have the same structure and they should share code, ideally.
I'd rather do this refactoring instead of adding temporary hacks like Vec<Name>, preferably in a separate PR.

(The attribute/meta-item Path goes through usual path resolution procedure, and that's where resolution of the "tool module" rustfmt or clippy should be added as well, ideally.)

nrc

This looks fine to me, but I think someone more active in compiler work should sign off too.

nrc · 2018-01-29T02:37:06Z

src/libsyntax/ast.rs

+    /// Unscoped name, e.g. `name` in `#[name(..)]`.
+    Single(Name),
+    /// Scoped name, e.g. `rustfmt::skip` in `#[rustfmt::skip]`.
+    Scoped(Vec<Name>),


Could we use a Path here, rather than Vec<Name>?

In fact I wonder if we could use Path instead of MetaItemName and always treat attributes as paths rather than names (we have to do this for macro attributes anyway)

nrc · 2018-01-29T02:41:27Z

r? @petrochenkov

topecongiro · 2018-01-29T22:45:46Z

@petrochenkov @nrc Thank you for you reviews!

So I am going to replace MetaItemName with Path, ~~and move the tool name checking from feature_gate.rs to somewhere in parser code~~.

EDIT: I actually kept the tool name checking in feature_gate.rs.

petrochenkov · 2018-01-30T22:50:33Z

src/libsyntax/print/pprust.rs

@@ -708,6 +708,22 @@ pub trait PrintState<'a> {
        Ok(())
    }

+    fn print_attribute_path(&mut self, path: &ast::Path) -> io::Result<()> {


Looks like existing print_path can be used instead of introducing this new function.

print_path is not a method of PrintState trait, so I am not sure how to exploit it. Should I write like self.writer().word(&path_to_string(path))?

petrochenkov · 2018-01-30T22:54:08Z

The Travis CI build failed

rustdoc build fails

petrochenkov · 2018-01-30T23:01:27Z

src/libsyntax/attr.rs

-        let (span, name) = match tokens.next() {
-            Some(TokenTree::Token(span, Token::Ident(ident))) => (span, ident.name),
+        let name = match tokens.next() {
+            Some(TokenTree::Token(span, Token::Ident(ident))) => {


This looks like an imitation of parse_path, but I don't know how make the normal version and tokenstream version reuse the same code.
Could you leave a FIXME comment here?

petrochenkov · 2018-01-30T23:03:19Z

src/libsyntax/parse/parser.rs

@@ -1972,26 +1972,6 @@ impl<'a> Parser<'a> {
        Ok(ast::Path { segments, span: lo.to(self.prev_span) })
    }

-    /// Like `parse_path`, but also supports parsing `Word` meta items into paths for back-compat.


Where does this special back-compat support of Word meta items happen now?

Oh, wait parse_path_allowing_meta is reintroduced in the next commit.

petrochenkov · 2018-01-30T23:08:19Z

src/libsyntax/attr.rs

@@ -205,6 +211,10 @@ impl NestedMetaItem {
    }
 }

+fn name_from_path(path: &ast::Path) -> Name {
+    path.segments.iter().next().unwrap().identifier.name


Could you use the last segment instead?
In #[a::b::c] a and b are modules (possibly "tool modules") in which the attribute c is defined, and the last segment c is the attribute itself.

petrochenkov · 2018-01-30T23:21:17Z

src/libsyntax/feature_gate.rs

-                                        added to it in the future",
-                                       attr.path));
+                if attr.is_scoped() {
+                    gate_feature!(self, tool_attributes, attr.span,


This solution is 100% technical debt, but I guess it can work while everything is unstable, and we go into this branch only if previous ways to resolve the attribute failed.

I described the proper solution in rust-lang/rfcs#2103 (comment).
TLDR: a::b::c in #[a::b::c(...)] should be resolved as a usual path in macro namespace and therefore rustfmt and clippy should be added into the language prelude as special modules (primitive types u8, u16, etc work in very similar way).
I don't suggest to implement this now though.

I don't suggest to implement this now though.

But I suggest to implement this eventually, if you plan to continue working on macros/attributes and name resolution in rustc.

petrochenkov · 2018-01-31T23:03:56Z

src/libsyntax/feature_gate.rs

+                            self.parse_sess.span_diagnostic,
+                            attr.span,
+                            E0693,
+                            "An unkown tool name found in scoped attributes: `{}`.",


Typo "unkown" -> "unknown"

topecongiro · 2018-02-03T23:57:57Z

@petrochenkov Thank you for your reviews and I am sorry for the late reply. I hope that I can update this PR today...

petrochenkov · 2018-02-04T15:48:00Z

@bors r+

bors · 2018-02-04T15:48:01Z

📌 Commit 30214c9 has been approved by petrochenkov

bors · 2018-02-04T20:59:11Z

⌛ Testing commit 30214c90d560ddc3b6e83992f34afb340dd43d5e with merge cac046403a27dd42eb53ef8b9c94801592a12b5d...

bors · 2018-02-04T22:58:08Z

💔 Test failed - status-travis

And fix some typos

topecongiro · 2018-02-12T05:47:33Z

Rebased against master.

I have a problem reproducing the above failures locally... running ./x.py test src/test/run-pass-fulldeps/ does succeed without any failure.

petrochenkov · 2018-02-12T14:35:53Z

@topecongiro
This is actually a pretty-printing test, not run-pass-fulldeps test itself.
This should be reproducible with ./x.py test src/test/run-pass-fulldeps/pretty.

It is possible that this is an expected pretty-printer failure, then you can disable the test with // ignore-pretty if you are sure this is not this PR's fault.

topecongiro · 2018-02-12T15:38:35Z

@petrochenkov Thank you for your advice, I could reproduce this locally.

I cannot tell whether this is an expected pretty-printer failure or not. I looked at the original PR that added this test (#40346). It looks like now arbitrary tokens could appear as arguments to procedural macros. So I guess that this PR is trying to parse random tokens inside procedural macros as some valid paths when it shouldn't? Again, I am not sure, though.

petrochenkov · 2018-02-12T20:59:42Z

src/libsyntax/feature_gate.rs

+                            self.parse_sess.span_diagnostic,
+                            attr.span,
+                            E0693,
+                            "An unknown tool name found in scoped attributes: `{}`.",


Nit: error messages conventionally start with a lower-case letter.
(Same in the feature gate message below.)

petrochenkov · 2018-02-12T21:24:45Z

@topecongiro
Pretty-printed code from run-pass-fulldeps/proc-macro/derive-b is identical before and after this PR, so the only difference is how name resolution treats B in #[cfg_attr(all(), B arbitrary tokens)].

I don't quite see where is the difference in name resolution comes from. Please make sure the logic for single-segment attribute paths is identical to what it was before. For example, fn name previously returned None for multi-segment attribute paths and now it's always successful, maybe it unintendedly makes difference somewhere.

pietroalbini · 2018-02-19T13:42:06Z

@topecongiro can you fix that failing test so the PR can be merged?

bors · 2018-02-25T04:55:12Z

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

shepmaster · 2018-03-02T19:31:57Z

Thank you for your hard work, @topecongiro ! However, we haven't heard from you in a few weeks, so I'm going to close this PR for now to keep the queue tidy. Please feel free to reopen this when you have a chance to fix the merge conflicts and the failing test.

Implement tool_attributes feature (RFC 2103) cc #44690 This is currently just a rebased and compiling (hopefully) version of #47773. Let's see if travis likes this. I will add the implementation for `tool_lints` this week.

rust-highfive assigned nrc Jan 26, 2018

petrochenkov self-assigned this Jan 26, 2018

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

petrochenkov reviewed Jan 27, 2018

View reviewed changes

nrc reviewed Jan 29, 2018

View reviewed changes

rust-highfive unassigned nrc Jan 29, 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 29, 2018

topecongiro force-pushed the rfc2103 branch 2 times, most recently from 54f59e1 to b6a9355 Compare January 30, 2018 10:31

petrochenkov reviewed Jan 30, 2018

View reviewed changes

petrochenkov reviewed Jan 31, 2018

View reviewed changes

kennytm 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 Feb 4, 2018

topecongiro added 7 commits February 12, 2018 13:34

Add tests for a new feature 'tool_attributes'

77ced43

Allow Path for name of MetaItem

b93af01

Remove Option from the return type of Attribute::name()

f2b4234

Gate tool_attributes feature

5146c0c

Add tool_attributes feature to unstable book

9dcb692

Change Attribute::name to return the last segment

bbc0e62

And fix some typos

Fix failures in rustdoc

12b2287

topecongiro force-pushed the rfc2103 branch from 30214c9 to 12b2287 Compare February 12, 2018 05:45

kennytm 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 12, 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 Feb 12, 2018

petrochenkov reviewed Feb 12, 2018

View reviewed changes

shepmaster closed this Mar 2, 2018

shepmaster added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 2, 2018

nrc mentioned this pull request Mar 4, 2018

Tracking issue: skip attributes rust-lang/rustfmt#1978

Closed

6 tasks

flip1995 mentioned this pull request Mar 20, 2018

Tracking issue: RFC 2103 - attributes for tools #44690

Closed

4 tasks

flip1995 mentioned this pull request Apr 17, 2018

Implement tool_attributes feature (RFC 2103) #50030

Merged

flip1995 mentioned this pull request Jun 1, 2018

tool_attributes feature stops working when rust_2018_preview feature is enabled #51277

Closed

Implement tool_attributes feature (RFC 2103) #47773

Implement tool_attributes feature (RFC 2103) #47773

Uh oh!

Conversation

topecongiro commented Jan 26, 2018

Uh oh!

topecongiro commented Jan 26, 2018

Uh oh!

kennytm commented Jan 26, 2018

Uh oh!

topecongiro commented Jan 26, 2018

Uh oh!

petrochenkov Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrc commented Jan 29, 2018

Uh oh!

topecongiro commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Jan 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

topecongiro commented Feb 3, 2018

Uh oh!

petrochenkov commented Feb 4, 2018

Uh oh!

bors commented Feb 4, 2018

Uh oh!

bors commented Feb 4, 2018

Uh oh!

bors commented Feb 4, 2018

Uh oh!

topecongiro commented Feb 12, 2018

Uh oh!

petrochenkov commented Feb 12, 2018

Uh oh!

topecongiro commented Feb 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Feb 12, 2018

Uh oh!

pietroalbini commented Feb 19, 2018

Uh oh!

bors commented Feb 25, 2018

Uh oh!

shepmaster commented Mar 2, 2018

Uh oh!

Uh oh!

petrochenkov Jan 27, 2018 •

edited

Loading

topecongiro commented Jan 29, 2018 •

edited

Loading