Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support registering inert attributes and attribute tools using crate-level attributes #66070

Merged
merged 5 commits into from
Nov 10, 2019

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Nov 3, 2019

And remove #[feature(custom_attribute)].
(rustc_plugin::Registry::register_attribute is not removed yet, I'll do it in a follow up PR.)

#![register_attr(my_attr)]
#![register_tool(my_tool)]

#[my_attr] // OK
#[my_tool::anything] // OK
fn main() {}

Some tools (rustfmt and clippy) used in tool attributes are hardcoded in the compiler.
We need some way to introduce them without hardcoding as well.

This PR introduces a way to do it with a crate level attribute.
The previous attempt to introduce them through command line (#57921) met some resistance.

This probably needs to go through an RFC before stabilization.
However, I'd prefer to land this PR without an RFC to able to remove #[feature(custom_attribute)] and Registry::register_attribute while also providing a replacement.


register_attr is a direct replacement for #![feature(custom_attribute)] (#29642), except it doesn't rely on implicit fallback from unresolved attributes to custom attributes (which was always hacky and is the primary reason for the removal of custom_attribute) and requires registering the attribute explicitly.
It's not clear whether it should go through stabilization or not.
It's quite possible that all the uses should migrate to #![register_tool] (#66079) instead.


Details:

  • The naming is register_attr/register_tool rather than some register_attributes (plural, no abbreviation) for consistency with already existing attributes like cfg_attr, or feature, etc.

Previous attempt: #57921
cc #44690
Tracking issues: #66079 (register_tool), #66080 (register_attr)
Closes #29642

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2019
src/librustc_resolve/macros.rs Show resolved Hide resolved
src/librustc_resolve/macros.rs Outdated Show resolved Hide resolved
src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved
src/libsyntax/feature_gate/active.rs Outdated Show resolved Hide resolved

#![register_attr(attr, attr)] //~ ERROR attribute `attr` was already registered
#![register_tool(tool, tool)] //~ ERROR tool `tool` was already registered

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the result of attr vs tool collision?

#![register_attr(something)]
#![register_tool(something)]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They do not collide because the attribute is in macro namespace and the tool is in type namespace.

Copy link
Contributor

@matthewjasper matthewjasper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly looks good. There are a few things that I would like to see tests for:

  • #[no_implicit_prelude] should hide these attributes.
  • Using the register* attributes on something other than a crate should be a warning/error.
  • Whether registed attributes can be imported with use.

src/libsyntax/feature_gate/builtin_attrs.rs Outdated Show resolved Hide resolved
@petrochenkov 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 Nov 5, 2019
@JohnCSimon
Copy link
Member

Ping from triage -
@petrochenkov - this PR hasn't been updated in a few days
cc: @matthewjasper
thanks!

@petrochenkov
Copy link
Contributor Author

Updated with the comments addressed.

@petrochenkov petrochenkov 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 Nov 9, 2019
@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 9, 2019

📌 Commit 83f553c has been approved by matthewjasper

@bors 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 Nov 9, 2019
@bors
Copy link
Contributor

bors commented Nov 10, 2019

⌛ Testing commit 83f553c with merge b3f59a942069b71a14d6eb5991f582c054864de2...

@rust-highfive
Copy link
Collaborator

The job x86_64-msvc-cargo of your PR failed (pretty log, 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.
2019-11-10T05:34:52.6300872Z 
2019-11-10T05:34:52.6300903Z 
2019-11-10T05:34:52.6300950Z 
2019-11-10T05:34:52.6300982Z 
2019-11-10T05:34:52.6301056Z #![register_attr(my_attr)]
2019-11-10T05:34:52.6301127Z #![register_tool(my_tool)]
2019-11-10T05:34:52.6301211Z #[my_attr] // OK
2019-11-10T05:34:52.6301278Z #[my_tool::anything] // OK
2019-11-10T05:34:52.6301389Z (`rustc_plugin::Registry::register_attribute` is not removed yet, I'll do it in a follow up PR.)
2019-11-10T05:34:52.6301543Z - The naming is `register_attr`/`register_tool` rather than some `register_attributes` (plural, no abbreviation) for consistency with already existing attributes like `cfg_attr`, or `feature`, etc.
2019-11-10T05:34:52.6301753Z ---
2019-11-10T05:34:52.6301807Z ---
2019-11-10T05:34:52.6301874Z ---
2019-11-10T05:34:52.6301932Z AGENT_BUILDDIRECTORY=D:\a\1
---
2019-11-10T05:34:52.6304252Z ANT_HOME=C:\ProgramData\chocolatey\lib\ant\apache-ant-1.10.5
2019-11-10T05:34:52.6304329Z APPDATA=C:\Users\VssAdministrator\AppData\Roaming
2019-11-10T05:34:52.6304424Z AZURE_EXTENSION_DIR=C:\Program Files\Common Files\AzureCliExtensionDirectory
2019-11-10T05:34:52.6305400Z AZURE_HTTP_USER_AGENT=VSTS_d439fc94-e01f-4249-b63e-d8392bc0247c_build_10_0
2019-11-10T05:34:52.6305559Z And remove `#[feature(custom_attribute)]`.
2019-11-10T05:34:52.6305723Z BOOST_ROOT_1_69_0=C:\Program Files\Boost\1.69.0
2019-11-10T05:34:52.6305788Z BUILD_ARTIFACTSTAGINGDIRECTORY=D:\a\1\a
2019-11-10T05:34:52.6305867Z BUILD_BINARIESDIRECTORY=D:\a\1\b
2019-11-10T05:34:52.6305943Z BUILD_BUILDID=13149
---
2019-11-10T05:34:52.6310810Z GeckoWebDriver=C:\SeleniumWebDrivers\GeckoDriver
2019-11-10T05:34:52.6310901Z HOME=/c/Users/VssAdministrator
2019-11-10T05:34:52.6310987Z HOMEDRIVE=C:
2019-11-10T05:34:52.6311050Z HOMEPATH=\Users\VssAdministrator
2019-11-10T05:34:52.6311574Z However, I'd prefer to land *this* PR without an RFC to able to remove `#[feature(custom_attribute)]` and `Registry::register_attribute` while also providing a replacement.
2019-11-10T05:34:52.6311817Z INPUT_ARGUMENTS=
2019-11-10T05:34:52.6311881Z ImageVersion=20191028.1
2019-11-10T05:34:52.6311881Z ImageVersion=20191028.1
2019-11-10T05:34:52.6311981Z It's not clear whether it should go through stabilization or not.
2019-11-10T05:34:52.6312115Z It's quite possible that all the uses should migrate to `#![register_tool]` (https://github.com/rust-lang/rust/issues/66079) instead.
2019-11-10T05:34:52.6312346Z JAVA_HOME_11_X64=C:\Program Files\Java\zulu-11-azure-jdk_11.33.15-11.0.4-win_x64
2019-11-10T05:34:52.6312448Z JAVA_HOME_7_X64=C:\Program Files\Java\zulu-7-azure-jdk_7.31.0.5-7.0.232-win_x64
2019-11-10T05:34:52.6312567Z JAVA_HOME_8_X64=C:\Program Files\Java\zulu-8-azure-jdk_8.40.0.25-8.0.222-win_x64
2019-11-10T05:34:52.6312669Z LOCALAPPDATA=C:\Users\VssAdministrator\AppData\Local
---
2019-11-10T05:34:52.6321560Z SYSTEM_TEAMPROJECTID=e71b0ddf-dd27-435a-873c-e30f86eea377
2019-11-10T05:34:52.6321663Z SYSTEM_TIMELINEID=e35078b8-1d30-49c3-8229-28167d9adf69
2019-11-10T05:34:52.6321737Z SYSTEM_TOTALJOBSINPHASE=16
2019-11-10T05:34:52.6321822Z SYSTEM_WORKFOLDER=D:\a
2019-11-10T05:34:52.6321929Z Some tools (`rustfmt` and `clippy`) used in tool attributes are hardcoded in the compiler.
2019-11-10T05:34:52.6322036Z Support registering inert attributes and attribute tools using crate-level attributes
2019-11-10T05:34:52.6322211Z TEMP=/tmp
2019-11-10T05:34:52.6322283Z TERM=cygwin
2019-11-10T05:34:52.6322343Z TF_BUILD=True
2019-11-10T05:34:52.6322425Z TMP=/tmp
2019-11-10T05:34:52.6322425Z TMP=/tmp
2019-11-10T05:34:52.6322503Z TOOLSTATE_ISSUES_API_URL=https://api.github.com/repos/rust-lang/rust/issues
2019-11-10T05:34:52.6322598Z TOOLSTATE_PUBLISH=1
2019-11-10T05:34:52.6322703Z TOOLSTATE_REPO=https://github.com/rust-lang-nursery/rust-toolstate
2019-11-10T05:34:52.6322818Z The previous attempt to introduce them through command line (https://github.com/rust-lang/rust/pull/57921) met some resistance.
2019-11-10T05:34:52.6322945Z This PR introduces a way to do it with a crate level attribute.
2019-11-10T05:34:52.6323155Z This probably needs to go through an RFC before stabilization.
2019-11-10T05:34:52.6323252Z Tracking issues: #66079 (`register_tool`), #66080 (`register_attr`)
2019-11-10T05:34:52.6323403Z USERDOMAIN=fv-az363
2019-11-10T05:34:52.6323475Z USERDOMAIN_ROAMINGPROFILE=fv-az363
2019-11-10T05:34:52.6323536Z USERNAME=VssAdministrator
2019-11-10T05:34:52.6323616Z USERPROFILE=C:\Users\VssAdministrator
2019-11-10T05:34:52.6323616Z USERPROFILE=C:\Users\VssAdministrator
2019-11-10T05:34:52.6323679Z VCPKG_INSTALLATION_ROOT=C:\vcpkg
2019-11-10T05:34:52.6323753Z VCVARS_BAT=vcvars64.bat
2019-11-10T05:34:52.6323830Z VS140COMNTOOLS=C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\
2019-11-10T05:34:52.6323924Z VSTS_AGENT_PERFLOG=c:\vsts\perflog
2019-11-10T05:34:52.6324012Z VSTS_PROCESS_LOOKUP_ID=vsts_a793f582-7718-41fe-a734-0b3bb3114a89
2019-11-10T05:34:52.6324079Z WINDIR=C:\windows
2019-11-10T05:34:52.6324156Z WIX=C:\Program Files (x86)\WiX Toolset v3.11\
2019-11-10T05:34:52.6324231Z We need some way to introduce them without hardcoding as well.
2019-11-10T05:34:52.6324367Z ```
2019-11-10T05:34:52.6324429Z ```rust
2019-11-10T05:34:52.6324429Z ```rust
2019-11-10T05:34:52.6324587Z `register_attr` is a direct replacement for `#![feature(custom_attribute)]` (https://github.com/rust-lang/rust/issues/29642), except it doesn't rely on implicit fallback from unresolved attributes to custom attributes (which was always hacky and is the primary reason for the removal of `custom_attribute`) and requires registering the attribute explicitly.
2019-11-10T05:34:52.6324827Z cc https://github.com/rust-lang/rust/issues/44690
2019-11-10T05:34:52.6324960Z fn main() {}
2019-11-10T05:34:52.6328746Z 
2019-11-10T05:34:52.6329094Z disk usage:
---
2019-11-10T07:45:33.4077821Z [RUSTC-TIMING] lazycell test:false 0.139
2019-11-10T07:45:33.4103237Z    Compiling hex v0.4.0
2019-11-10T07:45:36.8057313Z [RUSTC-TIMING] hex test:false 3.386
2019-11-10T07:45:36.8155967Z    Compiling glob v0.3.0
2019-11-10T07:45:37.4832023Z memory allocation of 2147483656 bytes failed[RUSTC-TIMING] hex test:false 4.275
2019-11-10T07:45:37.4943635Z error: could not compile `hex`.
2019-11-10T07:45:38.7754599Z [RUSTC-TIMING] glob test:false 1.953
2019-11-10T07:45:38.8158391Z error: build failed
2019-11-10T07:45:38.8225517Z command did not execute successfully: "D:\\a\\1\\s\\build\\x86_64-pc-windows-msvc\\stage0\\bin\\cargo.exe" "build" "-Zconfig-profile" "--target" "x86_64-pc-windows-msvc" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--locked" "--color" "always" "--manifest-path" "D:\\a\\1\\s\\src/tools/cargo\\Cargo.toml" "--features" "rustc-workspace-hack/all-static" "--message-format" "json-render-diagnostics"
2019-11-10T07:45:38.8225922Z expected success, got: exit code: 101
2019-11-10T07:45:38.8225922Z expected success, got: exit code: 101
2019-11-10T07:45:38.9372893Z failed to run: D:\a\1\s\build\bootstrap\debug\bootstrap test src/tools/cargotest src/tools/cargo
2019-11-10T07:45:38.9373036Z Build completed unsuccessfully in 1:59:28
2019-11-10T07:45:39.0125968Z == clock drift check ==
2019-11-10T07:45:39.1137962Z   local time: Sun Nov 10 07:45:39 CUT 2019
2019-11-10T07:45:39.5337009Z   network time: Sun, 10 Nov 2019 07:45:39 GMT
2019-11-10T07:45:39.5355087Z == end clock drift check ==
2019-11-10T07:45:39.6089271Z 
2019-11-10T07:45:40.0425355Z ##[error]Bash exited with code '1'.
2019-11-10T07:45:40.1031801Z ##[section]Starting: Checkout
2019-11-10T07:45:40.1902148Z ==============================================================================
2019-11-10T07:45:40.1902295Z Task         : Get sources
2019-11-10T07:45:40.1902380Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

@bors
Copy link
Contributor

bors commented Nov 10, 2019

💔 Test failed - checks-azure

@bors 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 Nov 10, 2019
@petrochenkov
Copy link
Contributor Author

2019-11-10T07:45:37.4832023Z memory allocation of 2147483656 bytes failed[RUSTC-TIMING] hex test:false 4.275
2019-11-10T07:45:37.4943635Z error: could not compile `hex`.

Wat.
@bors retry

@bors 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 Nov 10, 2019
@bors
Copy link
Contributor

bors commented Nov 10, 2019

⌛ Testing commit 83f553c with merge 3fc30d8...

bors added a commit that referenced this pull request Nov 10, 2019
Support registering inert attributes and attribute tools using crate-level attributes

And remove `#[feature(custom_attribute)]`.
(`rustc_plugin::Registry::register_attribute` is not removed yet, I'll do it in a follow up PR.)

```rust
#![register_attr(my_attr)]
#![register_tool(my_tool)]

#[my_attr] // OK
#[my_tool::anything] // OK
fn main() {}
```

---
Some tools (`rustfmt` and `clippy`) used in tool attributes are hardcoded in the compiler.
We need some way to introduce them without hardcoding as well.

This PR introduces a way to do it with a crate level attribute.
The previous attempt to introduce them through command line (#57921) met some resistance.

This probably needs to go through an RFC before stabilization.
However, I'd prefer to land *this* PR without an RFC to able to remove `#[feature(custom_attribute)]` and `Registry::register_attribute` while also providing a replacement.

---
`register_attr` is a direct replacement for `#![feature(custom_attribute)]` (#29642), except it doesn't rely on implicit fallback from unresolved attributes to custom attributes (which was always hacky and is the primary reason for the removal of `custom_attribute`) and requires registering the attribute explicitly.
It's not clear whether it should go through stabilization or not.
It's quite possible that all the uses should migrate to `#![register_tool]` (#66079) instead.

---

Details:
- The naming is `register_attr`/`register_tool` rather than some `register_attributes` (plural, no abbreviation) for consistency with already existing attributes like `cfg_attr`, or `feature`, etc.
---
Previous attempt: #57921
cc #44690
Tracking issues: #66079 (`register_tool`), #66080 (`register_attr`)
Closes #29642
@bors
Copy link
Contributor

bors commented Nov 10, 2019

☀️ Test successful - checks-azure
Approved by: matthewjasper
Pushing 3fc30d8 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for custom_attribute features
7 participants