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: Feature gate unused attributes #572

Merged
merged 2 commits into from
Feb 12, 2015
Merged

Conversation

Manishearth
Copy link
Member

@sfackler
Copy link
Member

I would personally prefer the use of rustc:: instead of rustc_ or rustc(). Allowing paths instead of just idents in attributes seems like it could be potentially useful in other contexts as well.

I unfortunately don't think that the unused attribute alternative would work. You could define a syntax extension called #[foo] that uses #[bar] attributes to do something. The #[bar] attributes would be marked as used, but if the compiler started doing something with #[bar] attributes, you'd be hosed.

@Manishearth
Copy link
Member Author

I would personally prefer the use of rustc:: instead of rustc_ or rustc().

Me too, but we don't support that yet (and probably won't till post-1.0).

I unfortunately don't think that the unused attribute alternative would work. You could define a syntax extension called #[foo] that uses #[bar] attributes to do something. The #[bar] attributes would be marked as used, but if the compiler started doing something with #[bar] attributes, you'd be hosed.

We make no guarantees about the stability of syntax extensions and other plugins in nightly builds. We can switch unused_attribute to Warn on non-release builds (or allow enabling a feature gate that lets us #[allow()] it where needed). In the context of syntax extension stability name clashes are a relatively small issue, the entire API is unstable and probably will undergo some major changes before stability.

@sfackler
Copy link
Member

I would personally prefer the use of rustc:: instead of rustc_ or rustc().

Me too, but we don't support that yet (and probably won't till post-1.0).

But this RFC is dealing with an issue that will only matter after 1.0. We can take that solution, and won't need to worry about changing the attribute syntax until we decide we want a new compiler-used attribute after 1.0. In any case, it's a pretty trivial change to make.

We make no guarantees about the stability of syntax extensions and other plugins in nightly builds. We can switch unused_attribute to Warn on non-release builds (or allow enabling a feature gate that lets us #[allow()] it where needed). In the context of syntax extension stability name clashes are a relatively small issue, the entire API is unstable and probably will undergo some major changes before stability.

I think we still need a plausible story of how the solution to this RFC will interact with stabilized syntax extensions, since this RFC will have an impact on them.

@nrc
Copy link
Member

nrc commented Jan 11, 2015

I'm tempted to say, lets take the alternative of making unused attributes error by default (I want that anyway) and deal with the question of syntax extensions when we stabilise those, since that won't be for a while.

Otherwise, do nothing, and rely on :: in attributes for when we want to add rustc::* (and, bikeshed alert, I prefer rust::* to rustc::* for 'official' language attributes, I would reserve rustc for compiler specific things).

@sfackler
Copy link
Member

@nick29581 Agreed about rust:: over rustc::. std:: might be an option as well.

Okay, so I've been thinking about this a bit more, and I think this is a reasonable path forward:

  • Before 1.0, forbid unused attributes. Note that there are some interactions here with the current discussion over the use of stability attributes in third party libraries. This solves the immediate issue.
  • When stabilizing syntax extensions, alter the meta item grammar to allow paths instead of just idents. This is backwards compatible, since idents are paths.
  • Redefine all existing attributes used by the compiler and rustdoc under the rust:: (or std::) namespace. That is, these would be equivalent: #[derive(Eq)] and #[rust::derive(Eq)]. Any new compiler/rustdoc attributes will need to be defined under only the rust namespace.
  • Decorator syntax extensions are all namespaced under the module they're imported as (which is not allowed to be called rust for obvious reasons). Syntax extensions are forbidden from marking attributes in the rust or global namespace as used.

Does this seem reasonable? We could push to make the grammar change and redefine all of the current compiler attributes as rust::foo before 1.0, but that seems like totally unnecessary churn.

@Manishearth
Copy link
Member Author

@sfackler #[rustc::foo] is not supported right now. Metaitems have a very limited structure at the moment. If someone wants to add a feature that needs a new attr, it's unreasonable to expect them to add a whole new set of attribute parsing rules, syntax, and AST representation and update everything.

I think we still need a plausible story of how the solution to this RFC will interact with stabilized syntax extensions, since this RFC will have an impact on them.

You mean in the future? The RfC isn't meant to be binding about rustc_ or the Forbid, if we ever get stuff like rustc:: we can switch to using that and remove the Forbid on unused attributes. In the meantime, the Forbid will only exist on release, which is great because syntexts don't work there anyway.

@brson
Copy link
Contributor

brson commented Jan 12, 2015

Regardless of the merits of doing the reservation I may prefer not using the word rustc for anything that must be specced. I consider rustc to be the merely the name of the reference implementation of the compiler, otherwise unconnected to the language. Perhaps rust_ would be more palatable to me.

@Manishearth
Copy link
Member Author

  • Before 1.0, forbid unused attributes. Note that there are some interactions here with the current discussion over the use of stability attributes in third party libraries. This solves the immediate issue.
  • When stabilizing syntax extensions, alter the meta item grammar to allow paths instead of just idents. This is backwards compatible, since idents are paths.

Yeah, that deny-now fix-later is basically what I was thinking of when talking about the alternative (sorry I wasn't clear)

  • Redefine all existing attributes used by the compiler and rustdoc under the rust:: (or std::) namespace. That is, these would be equivalent: #[derive(Eq)] and #[rust::derive(Eq)]. Any new compiler/rustdoc attributes will need to be defined under only the rust namespace.
  • Decorator syntax extensions are all namespaced under the module they're imported as (which is not allowed to be called rust for obvious reasons). Syntax extensions are forbidden from marking attributes in the rust namespace as used.

I don't think we should figure out our final attribute namespacing story in this RfC, the idea is to do something so that compiler features can be added without any backcompat hassle now, and we can decide how to namespace attributes properly in the future. There may be some unforseen reasons as to why we can't or won't support the path-based namespacing, so I consider that to be a whole different problem for when syntext stabilization comes along. Which should be in the far future, not something we should decide on now.

(So it might end up that we have to use #[rust_foo_bar] in the future post syntext-stabilization, but both solutions here give us the freedom to stabilize on that without requiring a whole new feature which may or may not be possible to implement easily)

@Manishearth
Copy link
Member Author

@brson I like this idea.

@sfackler
Copy link
Member

I am well aware of the exact structure of meta items. Compiler-inspected attributes are not added all that often, and the switch from idents to paths will be straightforward.

We have to live with the language decisions we make now forever. We should not be optimizing on the convenience of the small number of developers working on the compiler. I would very much prefer to not have some weird mismash of #[rust_foo] and #[rust::bar] in 6 months.

@Manishearth
Copy link
Member Author

@sfackler Either way, the point of this RfC is not to make that decision; it is to put a safety net so that the actual attribute namespacing story can be figured out later.

So in your opinion should we do nothing (it works, but we need to get paths in metaitems before doing anything) or forbid unused attributes? I personally still prefer the latter (and prefer both over rustc_).

@sfackler
Copy link
Member

We have to forbid unused attributes, at the very least unused attributes that start with rustc_ at which point we may as well forbid all of them.

EDIT: *we have to assuming we go with the rust_ solution.

@flaper87
Copy link

I think the plan described here sounds reasonable. As far as using rust_ or rust:: goes, I'd personally prefer the later. One thing to consider, though, is that going with :: is already inconsistent with the way we do things right now.

@nikomatsakis
Copy link
Contributor

However we do it, I would like a space reserved for attributes that are internal to rustc itself. For example, there are some attributes used now for writing tests (e.g., rustc_variance). I consider the (recently implemented) rustc_error_message to be a grey area: it will never be part of the language semantics, since it concerns only error reporting, but it is something that seems like it should nonetheless be portable across compilers. If the answer is "ban all attributes except the whitelisted set for 1.0", then so be it. (I think everyone (probably?) agrees that namespace'd attributes seem preferable all around, but we've never gotten around to making it happen.)

@nrc nrc self-assigned this Jan 22, 2015
@Manishearth Manishearth changed the title RFC: Reserve #[rustc_*] for future language features RFC: Feature gate unused attributes Jan 31, 2015
@Manishearth
Copy link
Member Author

Since there was a lot of support for just feature gating unused attributes, I've rewritten the RfC to use that as the main suggestion along the lines of @sfackler's comment.

Thoughts?

@nrc nrc merged commit ac452e4 into rust-lang:master Feb 12, 2015
@nrc
Copy link
Member

nrc commented Feb 12, 2015

Merged.

Tracking issue

@Manishearth
Copy link
Member Author

\o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stability Proposals relating to policy and changes about stability of features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants