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

Ban empty features in stability annotations #74537

Closed

Conversation

Mark-Simulacrum
Copy link
Member

This simplifies rustdoc's cleaning and seems generally good -- I don't see a reason to allow empty feature names.

@rust-highfive
Copy link
Collaborator

r? @steveklabnik

(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 19, 2020
@Mark-Simulacrum
Copy link
Member Author

Hm, @steveklabnik does not seem like a good reviewer, perhaps r? @petrochenkov or so?

@petrochenkov
Copy link
Contributor

petrochenkov commented Jul 21, 2020

Seems a bit arbitrary - what about other weird strings that don't look like your typical feature, e.g. " "?
Not sure why rustdoc treated empty string features specially. Perhaps it's simpler to just remove that special treatment? It's an internal feature and nobody is going to use silly feature names like "" or " " anyway.

@Mark-Simulacrum
Copy link
Member Author

Yeah I'm fine removing the support from rustdoc -- at least for unstable features. Going to go ahead and close this then, can revisit in a separate PR that modifies rustdoc (there's a number of similar cases that I'd like to hit at once).

@petrochenkov
Copy link
Contributor

If we want to reject silly feature names, then something like an identifier check would be better.
Non-identifier features won't be usable in #![feature(...)], for example.

tmandry added a commit to tmandry/rust that referenced this pull request Aug 11, 2020
…crum

Introduce `rustc_lexer::is_ident` and use it in couple of places

Implements the suggestion from rust-lang#74537 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants