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

RFC: #[used] static variables #2386

Merged
merged 2 commits into from
May 27, 2018
Merged

RFC: #[used] static variables #2386

merged 2 commits into from
May 27, 2018

Conversation

japaric
Copy link
Member

@japaric japaric commented Apr 3, 2018

RFC to stabilize the experimental #[used] feature added in rust-lang/rust#39987.

This feature has been in the compiler for a bit over a year; it's being widely used in embedded /
no-std context; and it has had no bugs filled against it (other than rust-lang/rust#41628 which was
filled shortly after the feature was added).

Tracking issue: rust-lang/rust#40289.

The embedded WG would like to see this feature stabilized in time for the edition release as it appears often in core components of the embedded ecosystem; it's not a blocker for the edition release though as it can be worked around.

Rendered

cc @aturon

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the RFC. label Apr 3, 2018
@clarfonthey
Copy link
Contributor

clarfonthey commented Apr 4, 2018

While I know that C using used is a huge precedent for going with the name used, I'd like to see a bit more discussion of alternative names just so that we're clear that we've exhausted our options.

I personally think that #[no_remove] may be a bit cleaner, as it goes along nicely with #[no_mangle]. Also, #[preserve] was suggested in the past and I didn't 100% read up on why that name wasn't chosen.

@phil-opp
Copy link

phil-opp commented Apr 5, 2018

There is an issue that a static is optimized away if it's in an otherwise unused module, even with the #[used] attribute: rust-lang/rust#47384 (comment). This was resolved.

@nikomatsakis
Copy link
Contributor

I'm inclined to merge -- this seems pretty low risk. If we come up with some other attribute that can serve this purpose, we can always deprecate #[used].

@Centril Centril self-assigned this Apr 12, 2018
@jminer
Copy link

jminer commented Apr 13, 2018

I think the attribute should be renamed. I think a verb would fit Rust's naming conventions better instead of an adjective. My day job is mainly embedded development, and IAR's linker uses a keep option to prevent unused symbols from being removed. (IAR may be the most popular embedded compiler.) I think it would be easier to remember what either #[no_remove] or #[keep] do than to remember #[used]. They are much more self-describing.

@mjbshaw
Copy link
Contributor

mjbshaw commented Apr 13, 2018

Yes, "used" isn't the most straight-forward name. I personally prefer something like #[preserve(linker)] (with #[keep] as second(ish) choice; I prefer to avoid negation where possible (e.g. #[no_remove])). The primary benefit of the name #[used] is that it matches LLVM's and GCC's __attribute__((used)). People familiar with LLVM's and GCC's attribute should easily understand what #[used] does.

I'd rather have #[used] (as-is, with its current name) sooner rather than bikeshed the name and delay its stabilization. I've got some code that's raring to use it. A follow-up RFC could always introduce a new name or attribute (and deprecate the "used" name).

@nagisa
Copy link
Member

nagisa commented Apr 13, 2018 via email

@clarfonthey
Copy link
Contributor

So, collecting what's proposed:

  • #[used]
  • #[no_remove]
  • #[preserve] or #[preserve(linker)]
  • #[keep]

I would also propose #[link_keep], for consistency with the existing link_* attributes.

@Centril
Copy link
Contributor

Centril commented Apr 19, 2018

@japaric We discussed this on the latest lang team meeting and the feeling was that #[used] was too vague. The general sense on the meeting was that #[link_keep] is more descriptive and that a longer attribute name is OK since the use case is niche. What do you think?

@ssokolow
Copy link

ssokolow commented Apr 20, 2018

I'm not part of the target user base, but I don't see a 9-character name like link_keep being excessive (eg. it's only one character more than derive()) and the link_ prefix is certainly worth it for the improved ability to guess what it means. (I also agree that having a verb like keep helps to bolster intuitiveness.)

@jcsoo
Copy link

jcsoo commented Apr 24, 2018

Discussed at the embedded-wg meeting, and #[link_keep] sounds fine.

@nagisa
Copy link
Member

nagisa commented Apr 25, 2018 via email

@japaric
Copy link
Member Author

japaric commented Apr 25, 2018

Discussed at the embedded-wg meeting, and #[link_keep] sounds fine.

To further elaborate: we are fine with any name.


Personally, I'd rather not spend much time trying to find the "perfect" name. Very few people are going to directly deal with this feature; most people will depend on it because some crate in their dependency graph is using it. Some other people will indirectly use this feature behind a macro that ensures that's not misused (#[used] is often paired with #[link_section]; the later is much more of a footgun and easier to get wrong -- the macro is to prevent you from misusing the later).

Ultimately, I think the documentation is more important than the name of the feature and that's already covered by the RFC. All the suggested names sound fine to me; anyone will do, IMO.

@aturon
Copy link
Member

aturon commented Apr 26, 2018

We briefly discussed this RFC in the last lang team meeting to generally positive response. We need to resolve the naming, but I'm going to go ahead and push toward FCP/full review:

@rfcbot fcp merge

@rfcbot rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Apr 26, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 26, 2018

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Centril
Copy link
Contributor

Centril commented Apr 26, 2018

@nagisa While that reading is possible, I think in this case, #[link_keep] works well with #[link_section(..)] and associates them closer together, which is good. We could go with #[keep_until_link] but I think as @japaric that the documentation is the most important bit here since it is quite a niche feature. I think link_section is sufficiently specific so that it is easy to google.

@japaric Mind updating the RFC to use #[link_keep]?

@joshtriplett
Copy link
Member

joshtriplett commented Apr 26, 2018

I'm fine with #[link_keep]; that said, I'd also like to request that the documentation specifically mention that this works similarly to the C __attribute__((used)), to make it easy for people to find and understand this.

(Edit: no longer fine with #[link_keep]; see below.

@nagisa
Copy link
Member

nagisa commented Apr 26, 2018

Let me elaborate on my comment. I’m a target audience for #[used] and I find #[link_keep] one of the worst options for the name.

Even with good documentation, there’s a lot of value in concept having a self-descriptive name for people who are already familiar with said concept. link_keep self-describes as something that would retain items even through linking (cf. the .KEEP directive in the linker script), which is not true – it only does that until the linker is invoked.

So, even if somebody duckduckcame upon this attribute, without knowing about it a priori, it would require them to read through the whole documentation on the attribute to make sure it does what they want it to do. Lest they miss that extremely subtle distinction between attribute keeping items until or after linking (and they wanted the latter), they would spend a good few hours pondering why the attribute is “not working”. The name is outright confusing.

I do agree that the attribute doesn’t need to be very terse, though, and thus #[keep_(un)till_link] is way better in my books.

@joshtriplett
Copy link
Member

joshtriplett commented Apr 26, 2018

@rfcbot concern attribute-name

@nagisa That's a very good point! I hadn't thought about the potential implication there, but you're right, this doesn't affect the linker, it affects the compiler. That does make #[link_keep] the wrong name for this.

Given that, I'd personally favor #[used], for compatibility with existing C code and knowledge. I'd hesitate to introduce a new name for this.

I think this goes beyond bikeshedding, because there's a serious question here about compatibility (with widespread existing understanding) and descriptiveness (can we make something sufficiently more descriptive that it's worth not using the name "used" with all the understanding attached to that name).

@nikomatsakis
Copy link
Contributor

My 2 cents: While I don't have a strong opinion on the name, I tend to think that using a "familiar" name is a decent thing, in the absence of a strong alternative -- that is, I expect that most people who want this feature will go looking for it by asking "what is the equivalent to __attribute(used)__, and hence if the name is "used" they are more likely to successfully find it. But really I could go any which way.

@Centril
Copy link
Contributor

Centril commented Apr 26, 2018

OK; So I was writing this comment about how #[used] is would be too hard to find on google. Then I actually tried searching for "rust used" and the first search result was the right one. So I take back everything I said around the vagueness of #[used] in terms of being able to search for easily.

That said, if #[keep_until_link] satisfies @nagisa's concerns, then that would be my preference but #[used] seems fine too.


Beyond this RFC, and on a tangent, @joshtriplett raised the idea on the lang meeting today of some more general mechanism by which you can search for an attribute via https://doc.rust-lang.org/nightly/std/?search=%23%5Bused%5D. cc @rust-lang/docs on that.

@joshtriplett
Copy link
Member

@Centril Thanks for capturing that!

I do think that'd be generally useful for things like "what do you call ? so I can search for it", or "what's the trait associated with this operator", or "how do I find #[used] given that search engines ignore the punctuation" (or, for that matter, "what's the name of things that start with #[").

@QuietMisdreavus
Copy link
Member

@joshtriplett At least for the "what's the trait associated with this operator", we've just gotten those added in thanks to rust-lang/rust#49757 and rust-lang/rust#50231.

The issue for searching for specific attributes in the standard library docs is that most often there's no page for it to land on. The Reference is the better place for that.

@Centril
Copy link
Contributor

Centril commented Apr 26, 2018

@QuietMisdreavus The reference is not searchable tho (can we change that?); Couldn't the standard library documentation simply take all attributes and link to the reference?

@QuietMisdreavus
Copy link
Member

@Centril

  1. It is now! mdbook got this change a while back. There's a faint magnifying glass icon in the header now.
  2. To add this documentation to the standard library docs, it would need to be done as an extension to rustdoc, in the end. Rustdoc needs an item to add something to the search index. Sometimes this lands as a weird hack - for example, since the primitives don't show up in libcore as actual items, libstd has a bunch of dummy modules with special attributes that rustdoc specifically looks for. Something similar needs to happen for "the list of all known attributes". Rustdoc needs to be made aware of this list so it can create pages for them.

@joshtriplett
Copy link
Member

@petrochenkov

In addition to linking-related consequences it makes #[unused] lints not fire on items marked as #[used], which seems pretty consistent to me.

Exactly. That's also consistent with GCC/LLVM: __attribute__((unused)) and __attribute__((used)) both suppress warnings about unused code, but unused still lets the compiler throw it away, while used forces the compiler to keep it.

@repax
Copy link

repax commented May 8, 2018

I have three issues with #[used]:

  • It is ungracefully similar to the already present #[must_use].
  • A clique of embedded devs would know when to use it, yes, but anyone else might encounter it and be confused -- because:
  • Calling it "used" says that it is used, yes, but that's rather vague and it does not entail what the linker is supposed to do.

Please consider my library example above. An extern fn is exported by a library in Rust. This library can be used by other languages, such as C. The extern keyword is to the point, descriptive and precise. The linker knows what to do. The reader knows that this item is meant to be available from outside of the code at hand. It's not merely used.

I'm not against anything here in particular. I just see this moment as an opportunity to choose a better name.

@joshtriplett
Copy link
Member

@rfcbot resolved attribute-name

We discussed this in the lang team meeting, and ended up deciding to paint this bikeshed used-colored. :)

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels May 17, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented May 17, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@frehberg
Copy link

I would use this annotation to prevent the optimizer from eliminating unused variables, so my proposal would be

#[keep_unused]

@liigo
Copy link
Contributor

liigo commented May 25, 2018

#[link_used]

@anirudhb
Copy link

@frehberg That seems more like it would keep all unused variables. That might make Rust's 'unused variable' lint useless on statics.
I propose #[preserve]

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this RFC. label May 27, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented May 27, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@rfcbot rfcbot removed the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label May 27, 2018
@Centril Centril merged commit 5d2beda into rust-lang:master May 27, 2018
@Centril
Copy link
Contributor

Centril commented May 27, 2018

Huzzah! This RFC is merged!

Tracking issue: rust-lang/rust#40289

@euclio
Copy link

euclio commented May 30, 2018

I'm probably too late to chime in, but was #[link(used)] considered? That keeps the familiarity of the C attribute while clearly marking that it's an attribute that concerns the linker.

@joshtriplett
Copy link
Member

@euclio Not a bad idea, but the #[link(...)] attribute already exists for linking libraries, and in that capacity only applies to an extern block. Repurposing it for this, and changing it to apply to all symbols, seems problematic.

@euclio
Copy link

euclio commented May 30, 2018

Ah, I should've taken a look at the list of attributes. #[linker(used)] might work, or some other form of "namespacing".

@liigo
Copy link
Contributor

liigo commented Jun 6, 2018

#[used] is misleading. It's used until linking, but not used at writing or compiling code.
should be #[to_be_used] or #[maybe_useful], or #[useful] for short?
useful also implies: compiler, don't dismiss this item, it's useful for later use, i.e. linking.

@CryZe
Copy link

CryZe commented Jun 6, 2018 via email

bors added a commit to rust-lang/rust 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
@Centril Centril added A-attributes Proposals relating to attributes A-ffi FFI related proposals. A-linkage Proposals relating to the linking step. labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Proposals relating to attributes A-ffi FFI related proposals. A-linkage Proposals relating to the linking step. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.