resolve: Support custom attributes when macro modularization is enabled #53053

petrochenkov · 2018-08-04T02:24:50Z

Basically, if resolution of a single-segment attribute is a determined error, then we interpret it as a custom attribute.

Since custom attributes are integrated into general macro resolution, feature(custom_attribute) now requires and implicitly enables macro modularization (feature(use_extern_macros)).
Actually, a few other "advanced" macro features now implicitly enable macro modularization too (and one bug was found and fixed in process of enabling it).

The first two commits are preliminary cleanups/refactorings.

rust-highfive · 2018-08-04T02:24:53Z

r? @varkor

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

petrochenkov · 2018-08-04T02:25:04Z

r? @alexcrichton

alexcrichton

Looks great to me, thanks! Just one suspicious thing I'd like to clarify before r+'ing, and otherwise I got tripped up in a few places due to my own lack of in-depth understanding of resolve, so a few minor requests for comments!

alexcrichton · 2018-08-04T03:19:29Z

src/libsyntax/ext/expand.rs

@@ -1504,7 +1489,8 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
        };

        if attr.is_some() || !traits.is_empty()  {
-            if !self.cx.ecfg.macros_in_extern_enabled() {
+            if !self.cx.ecfg.macros_in_extern_enabled() &&
+               !self.cx.ecfg.custom_attribute_enabled() {


I wasn't quite able to discern why this change was needed, you can probably help me out though?

Macros on foreign items are feature gated, and in this place custom attributes look like macro attrs, so without this condition they start requiring feature(macros_in_extern) as well.
I should probably move feature gate checking for feature(macros_in_extern) to some other place in which attribute resolution is already known and we can gate macros, but not other attributes.

Ok sounds good to me!

alexcrichton · 2018-08-04T03:20:27Z

src/libsyntax/ext/expand.rs

@@ -565,7 +548,9 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
            _ => unreachable!(),
        };

-        attr::mark_used(&attr);
+        if let NonMacroAttr { mark_used: false } = *ext {} else {


Could a small comment be added here as to why everything else other than this one case gets a mark_used?

Because everything except for NonMacroAttr is a macro that's actually expanded and thus used in the process and NonMacroAttr { mark_used: true } gets mark_used because, well, it says so.

alexcrichton · 2018-08-04T03:20:46Z

src/libsyntax/ext/expand.rs

-                    // under us. If this is the case we're making progress so we
-                    // want to flag it as such, and we test this by looking if
-                    // the `attr_id()` method has been changing over time.
-                    if invoc.attr_id() != attr_id_before {


Definitely glad to be rid of this, nice refactoring!

alexcrichton · 2018-08-04T03:26:05Z

src/librustc_resolve/macros.rs

+            let path = invoc.path().expect("no path for non-macro attr");
+            match attr_kind {
+                NonMacroAttrKind::Tool | NonMacroAttrKind::DeriveHelper |
+                NonMacroAttrKind::Custom if is_attr_invoc => {


I'm not personally 100% familiar with resolve, so I'm a little lost here as to why if is_attr_invoc is needed here. Could a comment be added about why it's here and why we'd have a non-attr invocation get resolved to a NonMacroAttr def still?

Anything in macro namespace can resolve to Def::NonMacroAttr!

let x = inline!(); // ERROR expected a macro, found built-in attribute

alexcrichton · 2018-08-04T03:29:23Z

src/librustc_resolve/macros.rs

+            let binding = (Def::NonMacroAttr(NonMacroAttrKind::Custom),
+                           ty::Visibility::Public, ident.span, Mark::root())
+                           .to_name_binding(self.arenas);
+            Ok(MacroBinding::Global(binding))


To confirm, this basically means that it's impossible for an attribute to be "unresolved" now, right? All attributes that otherwise fail to get resolved simply get classified as "this is a custom attribute, it needs #![feature(custom_attributes)]"?

it's impossible for an attribute to be "unresolved" now

It's possible in some rare cases when import resolution/expansion is stuck due to indeterminacy, but otherwise yes, for a single-segment attribute you always get a custom attribute instead of "no resolution" (i.e. the old pre macro modularization behavior is restored).

alexcrichton · 2018-08-04T03:33:04Z

src/test/ui/tool-attributes-misplaced-1.rs

@@ -8,21 +8,21 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.

-#![feature(tool_attributes)]
+#![feature(tool_attributes, custom_attribute)]


Oh one thing I was wondering, this feature was just added to test that #[rustfmt], typically illegal under #![feature(tool_attributes)], is in fact actually allowed with #![feature(custom_attributes)]?

Yes, rustfmt resolves to a tool module in type namespace, but it resolves to nothing in macro namespace and is thus interpreted as a custom attribute.

Unfortunately, feature gate errors are fatal and stop compilation for some reason, so I had to enable the feature to get other errors, which are reported later.
I should fix this some day.

alexcrichton · 2018-08-04T03:33:53Z

if resolution of a single-segment attribute is a determined error, then we interpret it as a custom attribute.

I wasn't quite able to pintpoint where the single-segment clause was here in the resolver, could a test perhaps be added showing that the error for #![unknown] is different than #![unknown::another]?

petrochenkov · 2018-08-04T11:06:53Z

I wasn't quite able to pintpoint where the single-segment clause was here in the resolver

"lexical" in resolve_lexical_macro_path_segment means it's the first segment and is_attr == true should mean it's the last segment, so it's we get to custom attribute fallback only for single-segment attributes.

I'll add comments/asserts/tests for all the review stuff.

alexcrichton · 2018-08-04T15:28:02Z

Ok this all sounds great to me!

r=me w/ the extra comments/tests

petrochenkov · 2018-08-05T01:37:13Z

@bors r=alexcrichton

bors · 2018-08-05T01:37:13Z

📌 Commit 3456bec178b159fc03e064dcb9cebef5384fdd84 has been approved by alexcrichton

bors · 2018-08-06T04:54:30Z

⌛ Testing commit 3456bec178b159fc03e064dcb9cebef5384fdd84 with merge 0da4190b512e0ff0610f0c0893f77be11c8ce7e6...

bors · 2018-08-06T05:52:39Z

💔 Test failed - status-appveyor

petrochenkov · 2018-08-06T09:19:29Z

One pretty-printing test is failing, will fix.

…tures is enabled Do not mark all builtin attributes as used when macro modularization is enabled

Adjust a few fulldeps and pretty-printing tests Fix rebase

bors · 2018-08-06T21:52:20Z

📌 Commit 5088611 has been approved by alexcrichton

bors · 2018-08-08T05:25:24Z

⌛ Testing commit 5088611 with merge 7a6e62234d1d22af99e101c0aa45148e9abb5aa4...

bors · 2018-08-08T06:38:09Z

💔 Test failed - status-travis

kennytm · 2018-08-08T07:26:04Z

@bors retry travis-ci/travis-ci#9696

[01:09:38] fatal: unable to access 'https://github.com/rust-lang-nursery/rust-toolstate.git/': Could not resolve host: github.com

bors · 2018-08-08T08:38:03Z

⌛ Testing commit 5088611 with merge ffb09df...

resolve: Support custom attributes when macro modularization is enabled Basically, if resolution of a single-segment attribute is a determined error, then we interpret it as a custom attribute. Since custom attributes are integrated into general macro resolution, `feature(custom_attribute)` now requires and implicitly enables macro modularization (`feature(use_extern_macros)`). Actually, a few other "advanced" macro features now implicitly enable macro modularization too (and one bug was found and fixed in process of enabling it). The first two commits are preliminary cleanups/refactorings.

bors · 2018-08-08T10:41:59Z

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

rust-highfive · 2018-08-08T10:43:15Z

📣 Toolstate changed by #53053!

Tested on commit ffb09df.
Direct link to PR: #53053

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra).
💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).

@Manishearth

Tested on commit rust-lang/rust@ffb09df. Direct link to PR: <rust-lang/rust#53053> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra). 💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).

oli-obk · 2018-08-08T10:54:53Z

on it

@kennytm

Update clippy submodule r? @kennytm fixes breakage from #53053 (comment)

kennytm · 2018-08-08T17:45:36Z

Note RLS's failure is different from clippy.

[01:17:05] error[E0432]: unresolved import `crate::config::Config`
[01:17:05]   --> tools/rls/src/actions/mod.rs:17:5
[01:17:05]    |
[01:17:05] 17 | use crate::config::Config;
[01:17:05]    |     ^^^^^^^^^^^^^^^^^^^^^ no `Config` in `config`
[01:17:05]
[01:17:05] error[E0432]: unresolved import `crate::config::Config`
[01:17:05]   --> tools/rls/src/actions/notifications.rs:15:5
[01:17:05]    |
[01:17:05] 15 | use crate::config::Config;
[01:17:05]    |     ^^^^^^^^^^^^^^^^^^^^^ no `Config` in `config`
[01:17:05]
[01:17:05] error[E0432]: unresolved import `crate::config::Config`
[01:17:05]   --> tools/rls/src/build/mod.rs:18:5
[01:17:05]    |
[01:17:05] 18 | use crate::config::Config;
[01:17:05]    |     ^^^^^^^^^^^^^^^^^^^^^ no `Config` in `config`
[01:17:05]
[01:17:05] error[E0432]: unresolved import `crate::config::Config`
[01:17:05]   --> tools/rls/src/build/cargo.rs:24:5
[01:17:05]    |
[01:17:05] 24 | use crate::config::Config;
[01:17:05]    |     ^^^^^^^^^^^^^^^^^^^^^ no `Config` in `config`
[01:17:05]
[01:17:05] error[E0432]: unresolved import `crate::config::Config`
[01:17:05]   --> tools/rls/src/build/rustc.rs:27:39
[01:17:05]    |
[01:17:05] 27 | use crate::config::{ClippyPreference, Config};
[01:17:05]    |                                       ^^^^^^ no `Config` in `config`
[01:17:05]
[01:17:05] error[E0432]: unresolved import `crate::config::Config`
[01:17:05]   --> tools/rls/src/cmd.rs:17:5
[01:17:05]    |
[01:17:05] 17 | use crate::config::Config;
[01:17:05]    |     ^^^^^^^^^^^^^^^^^^^^^ no `Config` in `config`
[01:17:05]
[01:17:05] error[E0432]: unresolved import `crate::config::Config`
[01:17:05]   --> tools/rls/src/server/mod.rs:17:5
[01:17:05]    |
[01:17:05] 17 | use crate::config::Config;
[01:17:05]    |     ^^^^^^^^^^^^^^^^^^^^^ no `Config` in `config`
[01:17:05]
[01:17:05] error: cannot determine resolution for the attribute macro `serde`
[01:17:05]    --> tools/rls/src/config.rs:122:3
[01:17:05]     |
[01:17:05] 122 | #[serde(default)]
[01:17:05]     |   ^^^^^
[01:17:05]     |
[01:17:05]     = note: import resolution is stuck, try simplifying macro imports
[01:17:05]
[01:17:05] error[E0412]: cannot find type `Config` in this scope
[01:17:05]    --> tools/rls/src/config.rs:163:18
[01:17:05]     |
[01:17:05] 163 | impl Default for Config {
[01:17:05]     |                  ^^^^^^ not found in this scope
[01:17:05]
[01:17:05] error[E0412]: cannot find type `Config` in this scope
[01:17:05]    --> tools/rls/src/config.rs:164:21
[01:17:05]     |
[01:17:05] 164 |     fn default() -> Config {
[01:17:05]     |                     ^^^^^^ not found in this scope
[01:17:05]
[01:17:05] error[E0422]: cannot find struct, variant or union type `Config` in this scope
[01:17:05]    --> tools/rls/src/config.rs:165:26
[01:17:05]     |
[01:17:05] 165 |         let mut result = Config {
[01:17:05]     |                          ^^^^^^ not found in this scope
[01:17:05]
[01:17:05] error[E0412]: cannot find type `Config` in this scope
[01:17:05]    --> tools/rls/src/config.rs:195:6
[01:17:05]     |
[01:17:05] 195 | impl Config {
[01:17:05]     |      ^^^^^^ not found in this scope
[01:17:05]
[01:17:05] error[E0412]: cannot find type `Config` in this scope
[01:17:05]    --> tools/rls/src/config.rs:197:39
[01:17:05]     |
[01:17:05] 197 |     pub fn update(&mut self, mut new: Config) {
[01:17:05]     |                                       ^^^^^^ not found in this scope
[01:17:05]
[01:17:05] error: aborting due to 13 previous errors
[01:17:05]
[01:17:05] Some errors occurred: E0412, E0422, E0432.
[01:17:05] For more information about an error, try `rustc --explain E0412`.
[01:17:05] [RUSTC-TIMING] rls test:false 1.585
[01:17:05] error: Could not compile `rls`.

nrc · 2018-08-08T21:53:47Z

With this change I get:

error: cannot determine resolution for the attribute macro `serde`
   --> src/config.rs:122:3
    |
122 | #[serde(default)]
    |   ^^^^^
    |
    = note: import resolution is stuck, try simplifying macro imports

the relevant imports seem to be:

use serde;
use serde::de::{Deserialize, Deserializer, Visitor};
use serde_derive::{Serialize, Deserialize};

and some more context for the error:

#[derive(Clone, Debug, Deserialize, Serialize)]
#[allow(missing_docs)]
#[serde(default)]
pub struct Config {
    ...

Given this is an issue using Serde, I fear this may affect many users once it hits nightly. Is this intentional? Any idea how to fix?

nrc · 2018-08-08T22:08:35Z

cc #53144

petrochenkov · 2018-08-08T22:15:02Z

@nrc
This is an unfortunate consequence of #52234 and appears if macro modularization is enabled (this PR could enable it implicitly if some other features are used - cb64672).

It should be fixable and it must be fixed before #50911 is landed, so I'll send a PR in a few days.

The workaround is to avoid use serde; (that's what causes resolution to stuck) and use ::serde::stuff explicitly, see 6c78d3b for examples.

nrc · 2018-08-09T02:42:58Z

Thanks @petrochenkov !

rust-highfive assigned varkor Aug 4, 2018

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

rust-highfive assigned alexcrichton and unassigned varkor Aug 4, 2018

alexcrichton reviewed Aug 4, 2018

View reviewed changes

petrochenkov force-pushed the custattr branch from 46150b8 to 3456bec Compare August 5, 2018 01:36

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 Aug 5, 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 Aug 6, 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 Aug 6, 2018

petrochenkov added 5 commits August 6, 2018 23:20

Discern between various kinds of non-macro attributes

467a7ab

Avoid modifying invocations in place for derive helper attributes

c84562e

Support custom attributes when macro modularization is enabled

f60d96a

Enable macro modularization implicitly if one of "advanced" macro fea…

cb64672

…tures is enabled Do not mark all builtin attributes as used when macro modularization is enabled

Address review comments

5088611

Adjust a few fulldeps and pretty-printing tests Fix rebase

petrochenkov force-pushed the custattr branch from 3456bec to 5088611 Compare August 6, 2018 21:49

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 Aug 6, 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 Aug 8, 2018

This comment has been minimized.

Sign in to view

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 Aug 8, 2018

bors merged commit 5088611 into rust-lang:master Aug 8, 2018

oli-obk mentioned this pull request Aug 8, 2018

Update clippy submodule #53192

Merged

bors added a commit that referenced this pull request Aug 8, 2018

Auto merge of #53192 - oli-obk:clippyup, r=kennytm

376b60d

Update clippy submodule r? @kennytm fixes breakage from #53053 (comment)

petrochenkov mentioned this pull request Aug 9, 2018

Stabilize use_extern_macros #50911

Merged

petrochenkov mentioned this pull request Aug 19, 2018

html5ever failing to compile on perf #53495

Closed

pnkfelix mentioned this pull request Oct 8, 2018

[1.30 beta] Test suite of the jemalloc-ctl crate is failing #54478

Closed

petrochenkov deleted the custattr branch June 5, 2019 16:10

resolve: Support custom attributes when macro modularization is enabled #53053

resolve: Support custom attributes when macro modularization is enabled #53053

Conversation

petrochenkov commented Aug 4, 2018

rust-highfive commented Aug 4, 2018

petrochenkov commented Aug 4, 2018

alexcrichton 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Aug 4, 2018

petrochenkov commented Aug 4, 2018

alexcrichton commented Aug 4, 2018

petrochenkov commented Aug 5, 2018

bors commented Aug 5, 2018

bors commented Aug 6, 2018

bors commented Aug 6, 2018

petrochenkov commented Aug 6, 2018

bors commented Aug 6, 2018

bors commented Aug 8, 2018

bors commented Aug 8, 2018

This comment has been minimized.

kennytm commented Aug 8, 2018

bors commented Aug 8, 2018

bors commented Aug 8, 2018

rust-highfive commented Aug 8, 2018

oli-obk commented Aug 8, 2018

kennytm commented Aug 8, 2018

nrc commented Aug 8, 2018

nrc commented Aug 8, 2018

petrochenkov commented Aug 8, 2018

nrc commented Aug 9, 2018