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

stabilize #[used] #51363

Merged
merged 4 commits into from
Sep 11, 2018
Merged

stabilize #[used] #51363

merged 4 commits into from
Sep 11, 2018

Conversation

japaric
Copy link
Member

@japaric japaric commented Jun 5, 2018

closes #40289

RFC for stabilization: rust-lang/rfcs#2386

r? @Centril

Where should this be documented? Currently the documentation is in the unstable book

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2018
@@ -609,6 +606,8 @@ declare_features! (
(accepted, fn_must_use, "1.27.0", Some(43302), None),
// Allows use of the :lifetime macro fragment specifier
(accepted, macro_lifetime_matcher, "1.27.0", Some(34303), None),
// Used to preserve symbols (see llvm.used)
(accepted, used, "1.28.0", Some(40289), None),
);

// If you change this, please modify src/doc/unstable-book as well. You must
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we even have an entry in the unstable-book? If yes, apply this comment, if no, we should probably document it together with stabilization.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we even have an entry in the unstable-book?

Yes.

If yes, apply this comment

I asked in the PR description where the documentation should be moved to as there are a few options (reference, book, RBE, etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would have assumed to add it to the book, because that would have been where I'd have searched for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@steveklabnik thinks otherwise (cf. #51366 (comment) and #51366 (comment)). I'll prep a PR to add documentation to the reference and the nomicon.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (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.
tidy check
[00:04:50] * 547 error codes
[00:04:50] * highest error code: E0911
[00:04:50] * 205 features
[00:04:50] tidy error: The Unstable Book has a 'language feature' section 'used' which doesn't correspond to an unstable language feature
[00:04:51] some tidy checks failed
[00:04:51] 
[00:04:51] 
[00:04:51] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:51] 
[00:04:51] 
[00:04:51] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:51] Build completed unsuccessfully in 0:01:51
[00:04:51] Build completed unsuccessfully in 0:01:51
[00:04:51] Makefile:79: recipe for target 'tidy' failed
[00:04:51] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:26ed04ba
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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)

@Centril
Copy link
Contributor

Centril commented Jun 5, 2018

r? @oli-obk

@bors
Copy link
Contributor

bors commented Jun 9, 2018

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

@fitzgen
Copy link
Member

fitzgen commented Jun 15, 2018

Can someone explain to me why this can only be applied to static items? It would be useful for exporting "unused" extern "C" fns from nested crates so that they appear in the final artifact (dylib/wasm/whatever).

@japaric
Copy link
Member Author

japaric commented Jun 20, 2018

Can someone explain to me why this can only be applied to static items?

A few reasons:

  • #[used] on functions is backward compatible to add / implement.

  • #[used] on functions needs a design (RFC) that defines where it is allowed: for instance,e is #[used] valid on generic functions? what would that even mean? how would that be implemented?

  • Keeping a unused function can be achieved using the current implementation of #[used] like this:

extern "C" fn keep_this() {}

#[used]
static KEEP: extern "C" fn() = keep_this;

@japaric
Copy link
Member Author

japaric commented Jun 20, 2018

Documentation PRs: rust-lang/reference#361 and rust-lang/nomicon#74

bradjc added a commit to tock/tock that referenced this pull request Jun 24, 2018
We had been using `#[no_mangle]` which is a workaround to ensure that
the compiler doesn't drop important symbols. `#[used]` is designed
explicitly for this purpose.

rust-lang/rust#51363
@pietroalbini
Copy link
Member

Now that the docs PRs are sent should we start the FCP process? cc @rust-lang/lang

@pietroalbini pietroalbini added T-lang Relevant to the language team, which will review and decide on the PR/issue. relnotes Marks issues that should be documented in the release notes of the next release. labels Jun 25, 2018
@eddyb
Copy link
Member

eddyb commented Jun 25, 2018

Wasn't the name quite controversial? Are we going with #[used] for sure?

@Centril
Copy link
Contributor

Centril commented Jun 25, 2018

@eddyb we accepted an RFC to stabilize with that name after a lot of bikeshed, so I'd say yes :)

@eddyb
Copy link
Member

eddyb commented Jun 25, 2018

Ah, I see, rust-lang/rfcs#2386 (comment). That's sad but I won't block the stabilization (#40289).

@@ -609,6 +606,8 @@ declare_features! (
(accepted, fn_must_use, "1.27.0", Some(43302), None),
// Allows use of the :lifetime macro fragment specifier
(accepted, macro_lifetime_matcher, "1.27.0", Some(34303), None),
// Used to preserve symbols (see llvm.used)
(accepted, used, "1.28.0", Some(40289), None),
Copy link
Member

Choose a reason for hiding this comment

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

This should be changed to 1.29.0 before merging.

@Centril
Copy link
Contributor

Centril commented Jun 25, 2018

@eddyb Hah :P You even ticked your own box on the RFC ;)

@eddyb
Copy link
Member

eddyb commented Jun 27, 2018

@Centril I ticked my box during the discussion about the name, which I was hoping would result in a better alternative being chosen. Acceptance of the RFC, with #[used], was done afterwards.

bradjc added a commit to tock/tock that referenced this pull request Jun 27, 2018
We had been using `#[no_mangle]` which is a workaround to ensure that
the compiler doesn't drop important symbols. `#[used]` is designed
explicitly for this purpose.

rust-lang/rust#51363
@TimNN
Copy link
Contributor

TimNN commented Jul 3, 2018

Ping from triage, @oli-obk / @rust-lang/lang: IIUC, this PR is waiting for one of you to trigger an FCP.

@cramertj
Copy link
Member

cramertj commented Jul 3, 2018

@TimNN #[used] has already been proposed for stabilization on the tracking issue. The FCP has not yet started-- still waiting for checkboxes.

@TimNN
Copy link
Contributor

TimNN commented Jul 3, 2018

Ah, thank you @cramertj, I hadn't noticed that. Marking as blocked on #40289.

@TimNN TimNN added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jul 3, 2018
@TimNN TimNN removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2018
@japaric
Copy link
Member Author

japaric commented Jul 3, 2018

To clear the PR queue, I'm going to close this until the FCP period is over.

@japaric japaric closed this Jul 3, 2018
@Centril Centril reopened this Sep 8, 2018
@Centril
Copy link
Contributor

Centril commented Sep 8, 2018

FCP period is now over.

@japaric can you resolve the conflicts / rebase? =P

@japaric
Copy link
Member Author

japaric commented Sep 9, 2018

@Centril I have rebased the PR. I have also pushed a commit that removes the chapter from the unstable book since there is a PR to document this in the reference (which I still need to update).

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (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.
[00:46:30] ....................................................................................................
[00:46:32] ....................................................................................................
[00:46:35] ....................................................................................................
[00:46:37] ....................................................................................................
[00:46:39] .....................................................F..............................................
[00:46:43] .................F.................i................................................................
[00:46:46] ..i.................................................................................................
[00:46:52] .......................................................................................i............
[00:46:54] ....................................................................................................
[00:46:57] ....................................................................................................
[00:47:00] ....................................................................................................
---
[00:52:31] 
[00:52:31] ---- [ui] ui/feature-gates/feature-gate-linker-flavor.rs stdout ----
[00:52:31] diff of stderr:
[00:52:31] 
[00:52:31] - error[E0658]: the `#[used]` attribute is an experimental feature (see issue #40289)
[00:52:31] + error: attribute must be applied to a `static` variable
[00:52:31] 2   --> $DIR/feature-gate-linker-flavor.rs:16:1
[00:52:31] 3    |
[00:52:31] 4 LL | #[used]
[00:52:31] 5    | ^^^^^^^
[00:52:31] -    |
[00:52:31] -    = help: add #![feature(used)] to the crate attributes to enable
[00:52:31] 8 
---
[00:52:31] 12 
[00:52:31] 
[00:52:31] 
[00:52:31] The actual stderr differed from the expected stderr.
[00:52:31] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-linker-flavor/feature-gate-linker-flavor.stderr
[00:52:31] To update references, rerun the tests and pass the `--bless` flag
[00:52:31] To only update this specific test, also pass `--test-args feature-gates/feature-gate-linker-flavor.rs`
[00:52:31] error: 1 errors occurred comparing output.
[00:52:31] status: exit code: 1
[00:52:31] status: exit code: 1
[00:52:31] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/feature-gates/feature-gate-linker-flavor.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-linker-flavor/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-linker-flavor/auxiliary" "-A" "unused"
[00:52:31] ------------------------------------------
[00:52:31] 
[00:52:31] ------------------------------------------
[00:52:31] stderr:
[00:52:31] stderr:
[00:52:31] ------------------------------------------
[00:52:31] {"message":"attribute must be applied to a `static` variable","code":null,"level":"error","spans":[{"file_name":"/checkout/src/test/ui/feature-gates/feature-gate-linker-flavor.rs","byte_start":702,"byte_end":709,"line_start":16,"line_end":16,"column_start":1,"column_end":8,"is_primary":true,"text":[{"text":"#[used]","highlight_start":1,"highlight_end":8}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"render

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)

@@ -674,6 +671,9 @@ declare_features! (
// Allows all literals in attribute lists and values of key-value pairs.
(accepted, attr_literals, "1.30.0", Some(34981), None),
(accepted, panic_handler, "1.30.0", Some(44489), None),
// Used to preserve symbols (see llvm.used)
(accepted, used, "1.29.0", Some(40289), None),
Copy link
Member

Choose a reason for hiding this comment

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

Time flies, this should be 1.30.0 now (or 1.31.0 if it can't be merged this week).

@cramertj
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 10, 2018

📌 Commit d37658a has been approved by cramertj

@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-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Sep 10, 2018
@bors
Copy link
Contributor

bors commented Sep 11, 2018

⌛ Testing commit d37658a with merge 7ee7207...

bors added a commit that referenced this pull request Sep 11, 2018
stabilize #[used]

closes #40289

RFC for stabilization: rust-lang/rfcs#2386

r? @Centril

Where should this be documented? Currently the documentation is in the unstable book
@bors
Copy link
Contributor

bors commented Sep 11, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: cramertj
Pushing 7ee7207 to master...

@bors bors merged commit d37658a into rust-lang:master Sep 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for the #[used] attribute