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

Add the -Zcrate-attr=foo unstable rustc option #52355

Merged
merged 1 commit into from
Jul 29, 2018

Conversation

pietroalbini
Copy link
Member

@pietroalbini pietroalbini commented Jul 13, 2018

This PR adds a new unstable option to rustc: -Zcrate-attr=foo. The option can be used to inject crate-level attributes from the CLI, and it's meant to be used by tools like Crater that needs to add their own attributes to a crate without changing the source code.

The exact reason I need this is to implement "edition runs" in Crater: we need to add the preview feature flag to every crate, and editing the crates' source code on the fly might produce unexpected results, while a compiler flag is more reliable.

cc rust-lang/crater#282 @Mark-Simulacrum

@rust-highfive
Copy link
Collaborator

r? @eddyb

(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 Jul 13, 2018
@nagisa
Copy link
Member

nagisa commented Jul 13, 2018

Would it be possible to generalize to a -Zattr='any attr'? That would be useful for more tools and testing scenarios.

@pietroalbini
Copy link
Member Author

Hmm, what other attributes did you have in mind? I can't think of many of them that apply to the whole crate and can be useful to dynamically add.

@eddyb
Copy link
Member

eddyb commented Jul 14, 2018

@pietroalbini I was discussing a while back "custom test frameworks" with @Manishearth, and this came up: the usecases I had in mind were injecting #![test] at the crate level, from --test (to trigger the test harness transformation), and maybe even e.g. -A foo -> #![allow(foo)].

Also there are some things like crate name, type, recursion limits, etc. where it'd be nice if there was a common interface (e.g. -a 'foo(bar)'), with existing flags explicitly marked as shorthands.

EDIT: @michaelwoerister might have some opinions regarding incremental, but I think such a flag would simplify things, by modifying the input source code itself, rather than being a side-channel.

@@ -1351,6 +1351,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"generate build artifacts that are compatible with linker-based LTO."),
no_parallel_llvm: bool = (false, parse_bool, [UNTRACKED],
"don't run LLVM in parallel (while keeping codegen-units and ThinLTO)"),
feature: Vec<String> = (Vec::new(), parse_string_push, [UNTRACKED],
Copy link
Member

Choose a reason for hiding this comment

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

cc @michaelwoerister on the UNTRACKED here - I think you can only do this if you actually modify the AST to include explicit attributes (which I think we should do).
Also, it'd be easier to handle e.g. the "unused feature" lint, if we have the attributes in the AST.

Copy link
Contributor

Choose a reason for hiding this comment

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

Modifying the AST would probably work for incremental, yes, but it also seems .. like overkill? In particular, the reason for this change is to unblock our ability to run a crater run and test the Edition migration, so I'd hate to just stall out on possible future improvements. We could just make the attribute [TRACKED].

Where would you propose doing the AST modification exactly?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was working on converting this PR to -Zcrate-attr=feature(foo) (wip branch), but I don't have time to finish it now (maybe in a few days). If you want to merge this as soon as possible I can do a quick commit changing the flag to [TRACKED], and then I can open a new PR implementing -Zcrate-attr and reverting this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh @nikomatsakis, now that I think about it, if we want to do a crater run as soon as possible we could also @bors try this and use this PR as the second crater toolchain.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point.

@pietroalbini pietroalbini 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 Jul 15, 2018
@pietroalbini
Copy link
Member Author

Let's get a toolchain built for the edition crater run.

@bors try

@bors
Copy link
Contributor

bors commented Jul 19, 2018

⌛ Trying commit bce31ea with merge 37c9dc8...

bors added a commit that referenced this pull request Jul 19, 2018
Add the -Zfeature=foo unstable rustc option

This PR adds a new unstable option to `rustc`: `-Zfeature=foo`. The option can be used to enable nightly features from the CLI, and it's meant to be used by tools like Crater to try enabling features without changing the crate's source code.

The exact reason I need this is to implement "edition runs" in Crater: we need to add the preview feature flag to every crate, and editing the crates' source code on the fly might produce unexpected results, while a compiler flag is more reliable. Obviously this won't be stabilized in the future.

cc rust-lang/crater#282 @Mark-Simulacrum
@bors
Copy link
Contributor

bors commented Jul 19, 2018

☀️ Test successful - status-travis
State: approved= try=True

@pietroalbini pietroalbini force-pushed the zfeature branch 2 times, most recently from 92fb368 to 277fbf3 Compare July 27, 2018 16:49
@pietroalbini pietroalbini changed the title Add the -Zfeature=foo unstable rustc option Add the -Zcrate-attr=foo unstable rustc option Jul 27, 2018
@pietroalbini
Copy link
Member Author

pietroalbini commented Jul 27, 2018

Ok, implemented -Zcrate-attr instead of -Zfeature, and while it works fine, some diagnostics are really bad. When there is a parse error the correct message is displayed:

$ rustc foo.rs -Zcrate-attr=10
error: expected identifier, found `10`
 --> <crate attribute>:1:1
  |
1 | 10
  | ^^ expected identifier

But when another part of rustc raises an error about that attribute the spans are messed up (showing the crate code instead of the attribute):

$ rustc foo.rs "-Zcrate-attr=feature(10)"
error[E0556]: malformed feature, expected just one word
 --> foo.rs:1:9
  |
1 | #![allow(dead_code)]
  |         ^^

error[E0658]: non-string literals in attributes, or string literals in top-level positions, are experimental (see issue #34981)
 --> foo.rs:1:1
  |
1 | #![allow(dead_code)]
  | ^^^^^^^^^^^
  |
  = help: add #![feature(attr_literals)] to the crate attributes to enable

@pietroalbini
Copy link
Member Author

Diagnostics fixed. This should be ready for a review now.

@pietroalbini 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 Jul 27, 2018
@pietroalbini
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Jul 28, 2018

⌛ Trying commit 71276c6 with merge 52fa4de...

bors added a commit that referenced this pull request Jul 28, 2018
Add the -Zcrate-attr=foo unstable rustc option

This PR adds a new unstable option to `rustc`: `-Zcrate-attr=foo`. The option can be used to inject crate-level attributes from the CLI, and it's meant to be used by tools like Crater that needs to add their own attributes to a crate without changing the source code.

The exact reason I need this is to implement "edition runs" in Crater: we need to add the preview feature flag to every crate, and editing the crates' source code on the fly might produce unexpected results, while a compiler flag is more reliable.

cc rust-lang/crater#282 @Mark-Simulacrum
@bors
Copy link
Contributor

bors commented Jul 28, 2018

☀️ Test successful - status-travis
State: approved= try=True

@eddyb
Copy link
Member

eddyb commented Jul 28, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Jul 28, 2018

📌 Commit 71276c6 has been approved by eddyb

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2018
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 28, 2018
@Manishearth
Copy link
Member

cc @djrenren this may be useful for custom test frameworks

@bors
Copy link
Contributor

bors commented Jul 28, 2018

⌛ Testing commit 71276c6 with merge 6323d9a...

bors added a commit that referenced this pull request Jul 28, 2018
Add the -Zcrate-attr=foo unstable rustc option

This PR adds a new unstable option to `rustc`: `-Zcrate-attr=foo`. The option can be used to inject crate-level attributes from the CLI, and it's meant to be used by tools like Crater that needs to add their own attributes to a crate without changing the source code.

The exact reason I need this is to implement "edition runs" in Crater: we need to add the preview feature flag to every crate, and editing the crates' source code on the fly might produce unexpected results, while a compiler flag is more reliable.

cc rust-lang/crater#282 @Mark-Simulacrum
@bors
Copy link
Contributor

bors commented Jul 29, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 6323d9a to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

7 participants