Unknown feature flags should trigger a warning or error

[shepmaster]

#![feature(wuzzy)]
fn main() {}

This code compiles with no warnings or errors on 1.28.0-nightly (2018-06-28 e3bf634). As of 2017-08-11, it prints a warning ("unused or unknown feature").

@Mark-Simulacrum ran a bisection and found:

bisection claims 2017-09-09 is the first nightly that regressed, but I'm still trying to narrow down a specific commit

I suspect #44142 (the commit range if I'm reading it right is d93036a to dead08c).

/cc @alexcrichton

[shepmaster]

I think this is slightly more important as we are encouraging people to try out the 2018 edition via a feature flag, but I'd find it useful to have back in any case.

[varkor]

This is #44232, though there it only talks about unused features, rather than invalid, features. However, that's the issue that the code points to for this:
https://github.com/rust-lang/rust/blob/08ccf2e59da8e49772ff74b0dab3140d45d7c7dc/src/librustc/middle/stability.rs#L827-L841

[nikomatsakis]

I this is not, I believe, a duplicate of #44232 — this is not about features that are not used but rather about features that are totally invalid.

[nikomatsakis]

Marking as P-high just because it's bad. I'm going to try and write up some mentoring instructions. "How hard can it be"

[varkor]

To clarify: in the past, the unused or invalid feature lint was the same lint — which is why #44232 only seems to mention unused features (hence the unused or unknown feature message).

The difficult part here is detecting invalid lib features, because we don't have a complete list of all the lib features (lang features are easy to check, on the other hand).

We could do the validation here if we could check against a set of all known library features:
https://github.com/rust-lang/rust/blob/08ccf2e59da8e49772ff74b0dab3140d45d7c7dc/src/libsyntax/feature_gate.rs#L1948

However, previously it was conflated into this check:
https://github.com/rust-lang/rust/blob/08ccf2e59da8e49772ff74b0dab3140d45d7c7dc/src/librustc/middle/stability.rs#L827-L841

From what I understand at the moment, this isn't going to be straightforward. Nowhere are all the library features gathered. Instead, we collect all the potential library features that have been declared, and then for each use of an item, we check whether it requires a library feature and then whether that library feature was declared.

To actually detect invalid features (i.e. to distinguish them from unused features), I think we are going to need to build up a set of valid library features. I'm not sure how awkward this would be yet.

---

This would probably be functionally achievable by calling find_stability for every set of item attributes:
https://github.com/rust-lang/rust/blob/08ccf2e59da8e49772ff74b0dab3140d45d7c7dc/src/libsyntax/attr.rs#L973-L977
And storing the feature names in a table. It seems likely this is not efficient and also might not mesh well with incremental. I think this has essentially the same technical difficulties as #44232, even if it's not merged again unused feature detection.

[pnkfelix]

visited for triage.

@varkor are you waiting for more guidance on potential ways to proceed? I cannot tell from your comment whether you are implicitly asking for this bug to be downgraded in priority, due to potentially-inherent inefficiencies in ways to resolve it?

[varkor]

That first comment was just clarifying what the problem was, and why it's slightly more difficult than we thought it might be. I still need to give it some more thought as the best way to proceed; hopefully I'll have something by next week.

[varkor]

I have a fix for this, which essentially involves giving lib features the same treatment as language features. However, it adds a massive list of features (~800 lines' worth) to feature_gate.rs and has the additional disadvantage that changing stability of a lib feature requires recompiling rustc.

I'm not sure of a way to detect whether a lib feature is valid without either storing them in a list, or checking every single method, etc. in core, alloc, std, etc. (which seems to be why the previous lint was "feature unused or unknown").

My question is whether we want to take this approach, or whether this is overkill, and there's a better way?

[Mark-Simulacrum]

I believe we do successfully lint on all library features when they're unused, so perhaps somehow that logic can be reused to avoid introducing a list anywhere? That is, we'd wait until we have that information and now emit the unused/unknown warning on any features that we don't recognize as a lang feature. Ideally of course we'd so of independent of lang-ness but since we don't properly track lang features yet we can just skip over them in the warning.

Does that make sense?

[varkor]

Just summarising a few points from an IRC discussion here:

• We used to lint on unused library features, but since Re-enable detection of unused library #![feature] directives #44232, the lint is gone, so it can't be repurposed.
• It'd be nice if we could somehow gather all the stability information in the crate metadata somehow. Some promising leads:
  • https://github.com/rust-lang/rust/blob/master/src/librustc_metadata/encoder.rs#L397-L403
  • https://github.com/rust-lang/rust/blob/master/src/librustc/ty/context.rs#L2813-L2824
  • https://github.com/rust-lang/rust/blob/master/src/librustc/middle/lang_items.rs#L198-L209

[pnkfelix]

visited for triage; PR #52644 looks like some great work to address this, and it seems like the discussion has essentially migrated there.

[shepmaster]

Thanks, @varkor ! I know that was a slog to get merged!