audit: ensure every gated feature has a compile-fail test

[pnkfelix]

I was sloppy when I implemented #22173 and did not include a test. So, surprise, a follow-on commit a few days later broke it.

As self-punishment for neglecting to include a regression test, I am assigning myself the task of reviewing our test suite to make sure that every feature-gate has a test.

---

Here is a transcribed list of feature gates, based on feature_gate.rs, that I have annotated according to whether the feature is Accepted/Removed (and thus needs no tests), Active/Deprecated but has tests already (marked with an x, or Active/Deprecated but has no tests that I saw via a cursory grep/skim.

• - ("globs", "1.0.0", Accepted),
• - ("macro_rules", "1.0.0", Accepted),
• - ("struct_variant", "1.0.0", Accepted),
• - ("asm", "1.0.0", Active),
  • asm-gated.rs
  • asm-gated2.rs
• - ("managed_boxes", "1.0.0", Removed),
• - ("non_ascii_idents", "1.0.0", Active),
  • gated-non-ascii-idents.rs
• - ("thread_local", "1.0.0", Active),
  • Add tests checking that a number of feature gates are gating their features #23226
• - ("link_args", "1.0.0", Active),
  • Add tests checking that a number of feature gates are gating their features #23226
• - ("phase", "1.0.0", Removed),
• - ("plugin_registrar", "1.0.0", Active),
  • gated-plugin_registrar.rs but it is not clear whether this is testing the plugin gate or the plugin_registrar gate. From the error message it seems like the plugin_registrar gate is firing.
• - ("log_syntax", "1.0.0", Active),
  • log-syntax-gate.rs
  • log-syntax-gate2.rs
• - ("trace_macros", "1.0.0", Active),
  • gated-trace_macros.rs
• - ("concat_idents", "1.0.0", Active),
  • concat_idents-gate.rs
  • concat_idents-gate2.rs
• - ("unsafe_destructor", "1.0.0", Active),
  • Add tests checking that a number of feature gates are gating their features #23226
• - ("intrinsics", "1.0.0", Active),
  • feature-gate-intrinsics.rs
• - ("lang_items", "1.0.0", Active),
  • feature-gate-lang-items.rs
• - ("simd", "1.0.0", Active),
  • gated-simd.rs
• - ("default_type_params", "1.0.0", Accepted),
• - ("quote", "1.0.0", Active),
  • Add tests checking that a number of feature gates are gating their features #23226
• - ("link_llvm_intrinsics", "1.0.0", Active),
  • Add tests checking that a number of feature gates are gating their features #23226
• - ("linkage", "1.0.0", Active),
  • linkage1.rs
• - ("struct_inherit", "1.0.0", Removed),
• - ("quad_precision_float", "1.0.0", Removed),
• - ("rustc_diagnostic_macros", "1.0.0", Active),
• - ("unboxed_closures", "1.0.0", Active),
• - ("import_shadowing", "1.0.0", Removed),
• - ("advanced_slice_patterns", "1.0.0", Active),
• - ("tuple_indexing", "1.0.0", Accepted),
• - ("associated_types", "1.0.0", Accepted),
• - ("visible_private_types", "1.0.0", Active),
• - ("slicing_syntax", "1.0.0", Accepted),
• - ("box_syntax", "1.0.0", Active), gated-box-syntax.rs
• - ("on_unimplemented", "1.0.0", Active),
• - ("simd_ffi", "1.0.0", Active), gated-simd-ffi.rs
• - ("if_let", "1.0.0", Accepted),
• - ("while_let", "1.0.0", Accepted),
• - ("plugin", "1.0.0", Active),
  • gated-plugin_registrar.rs but it is not clear whether this is testing the plugin gate or the plugin_registrar gate
• - ("start", "1.0.0", Active),
  • feature-gate-start.rs
• - ("main", "1.0.0", Active),
  • feature-gate-main.rs
• - ("issue_5723_bootstrap", "1.0.0", Accepted),
• - ("opt_out_copy", "1.0.0", Removed),
• - ("old_orphan_check", "1.0.0", Deprecated),
• - ("old_impl_check", "1.0.0", Deprecated),
• - ("optin_builtin_traits", "1.0.0", Active),
• - ("int_uint", "1.0.0", Active),
  • feature-gate-int-uint.rs
• - ("macro_reexport", "1.0.0", Active), gated-macro-reexports.rs
• - ("test_accepted_feature", "1.0.0", Accepted),
• - ("test_removed_feature", "1.0.0", Removed),
• - ("staged_api", "1.0.0", Active), staged_api.rs
• - ("unmarked_api", "1.0.0", Active),
• - ("no_std", "1.0.0", Active),
  • gated-no-std.rs
• - ("box_patterns", "1.0.0", Active), gated-box-patterns.rs
• - ("unsafe_no_drop_flag", "1.0.0", Active),
  • unsafe_no_drop_flag-gate.rs
• - ("custom_attribute", "1.0.0", Active),
• - ("custom_derive", "1.0.0", Active), single-derive-attr.rs
• - ("rustc_attrs", "1.0.0", Active),
• - ("static_assert", "1.0.0", Active),
  • feature-gate-static-assert.rs
• - ("allow_internal_unstable", "1.0.0", Active),
  • feature-gate-allow-internal-unstable.rs
  • feature-gate-allow-internal-unstable-nested-macro.rs

[pnkfelix]

(but hey, if other people want to add such missing tests in the meantime, while I am asleep, I will not mind. :)

[pnkfelix]

In case its not clear: If you want to help, just find a case above that does not have a check-mark and also does not have a pull request number attached beneath it. (And I guess write your name so that other people do not waste time adding a test for the same feature gate).

[lgrz]

The test for custom_attribute is located in custom_attribute.rs

[pnkfelix]

I am not surprised that I "overlooked" that ... the expected error message prefix does not say anything about a feature gate. Let me just confirm that the whole message does ...

Update: Okay, the help output for that test (custom_attribute.rs) does mention the feature gate:

help: add #![feature(custom_attribute)] to the crate attributes to enable

So I have now checked off that box, though I wonder if I should add this help message to the expected test output there.

[lgrz]

I was wondering the same thing about the help message. It is not very clear what that custom_attribute test is doing, the only hint was the filename.

From reading #23974 some help messages were removed, would there be any concern for adding help messages to the expected test output in this case?

It could be worth adding a comment in there similar to the other gated tests:

// Test that `#[...]` attribute is gated by `...` feature gate.

In addition, would it be worth going as far as to rename custom_attribute.rs to gated-custom_attribute.rs (and doing the same for any other gated tests that do not have a gated- prefix in their filename)?

[lgrz]

In relation to the feature gate list above I'm going to start off by looking into the following:

update: removed items that had tests, and added optin_builtin_traits, advanced_slice_patterns

• rustc_attrs
• on_unimplemented
• custom_derive
• box_patterns
• optin_builtin_traits
• advanced_slice_patterns

[pnkfelix]

@Lstat ah yes, #23974 is (correctly) why the help mag is missing .... hmm .... that complicates things

[lgrz]

I've done a pass through of the unchecked feature gates and have found tests for the following:

• box_patterns (gated-box-patterns.rs)
• box_syntax (gated-box-syntax.rs)
• custom_derive (single-derive-attr.rs)
• staged_api (staged_api.rs)
• simd_ffi (gated-simd-ffi.rs)
• macro_reexports (gated-macro-reexports.rs)

@pnkfelix Could these be marked as complete?

[pnkfelix]

@Lstat thanks!

[lgrz]

Ok, so I've had a look through everything, here's my findings:

@pnkfelix I have some questions below for the three outstanding feature gates (at the very bottom of this comment) and was wondering if you could help confirm the status of those feature gate tests?

My previous comment listed feature gates that had existing tests. I've since found a few more to add to that list.

Here is the full list of feature gates with existing tests. New findings are in bold:

• custom_derive: single-derive-attr.rs
• staged_api: staged_api.rs
• box_patterns: gated-box-patterns.rs
• macro_reexport: gated-macro-reexports.rs
• box_syntax: gated-box-syntax.rs
• simd_ffi: gated-simd-ffi.rs
• unboxed_closures: unboxed-closure-feature-gate.rs
• plugin_registrar: gated-plugin_registrar.rs
• advanced_slice_patterns: feature-gate-advanced-slice-features.rs

The following feature gates were removed after this issue was initially created. Since they are no longer around they can be considered complete or crossed out of the task list above:

• old_impl_check: see rustc: Remove support for old_impl_check #23749
• old_orphan_check: see std: Remove #[old_orphan_check] from PartialEq #23288

The following feature gates appear to have duplicate tests introduced in #23578, the initial tests were added in #20723 and #21233:

• box_patterns
• simd_ffi

The following are not in the above task list as they have been added since the issue was initially created:

• slice_patterns: see Feature gate *all* slice patterns. #23121 #23794
• negate_unsigned: see Gate unsigned unary negate #23945

The following are remaining feature gates needing tests (excluding the feature gates I had questions for, see below). I'm cooking up a PR for these:

• on_unimplemented
• rust_attr
• optin_builtin_traits
• plugin
• slice_patterns
• negate_unsigned

The following items are ones that I had trouble completing because I have questions and need some feedback:

• unmarked_api
• visible_private_types
• rustc_diagnostic_macros

unmarked_api

The unmarked_api was introduced in #21910, and tests for unmarked_api were replaced with tests for missing stability attributes: missing-stability.rs.

In addition, it seems that there is no way to trigger this code path unless there is a bug in a library or the compiler itself.

Are the tests in missing-stability.rs enough to consider unmarked_api complete?

visible_private_types

These appear to be the tests we're looking for relating to visible_private_types, could someone confirm this?

rustc_diagnostic_macros

This appears to be the rustc_diagnostic_macros feature gate test, but was removed in #19362 (14f9127) rustc-diagnostics-3.rs

Should we reinstate the test in this issue or let it be handled by #19624 which is in the works?

[abhijeetbhagat]

For visible_private_types, i found src/test/run-pass/visible-private-types-feature-gate.rs
Not sure if item this should be ticked as well.

[brson]

There appear to be a lot of new unstable features since this was touched. It seems hopeless to ever catch up on this issue without an automatic tidy script verifying the tests exist. @pnkfelix do you want this issue to continue? Should we try to augment tidy to check the existence of compile-fail tests mentioning unstable features?

[pnkfelix]

@brson I agree that it seems like we'll never be able to close this without having some sort of automation (e.g. a tidy script, as you say) to verify that the tests exist.

I don't have any immediate idea about the best way to write such a script. I guess it could just be based on some naming convention for the test itself; i.e. it would be just a heuristic check, not a true verification of it.

[brson]

I think it could be written like so: after featureck, search everything in src/compile-fail for #![deny($feature_name)]. If this makes sense I can mentor.

[pnkfelix]

@brson but requiring the test to explicitly deny the feature defeats the point, doesn't it?

That is, the goal here is to catch code that makes use of a feature without explicitly opting into that feature. ..

Though I suppose you might be thinking that if there is enough infrastructure present to support some kind of deny(feature) attribute, then that is a sign that the necessary gating is in place, regardless of what the default lint settings actually are?

[brson]

@pnkfelix Good point. I think yes, I was considering that just confirming that the compiler has code that correctly can deny the feature is good enough, but there is some room for error by making the deny explicit. The deny attribute also wouldn't actually confirm that the compile failed for the correct reason. We could also just require tests to annotate in some other way that yes, it is testing that a particular feature is denied.

As an extension to my previous suggestion, the test runner could take the test case that is annotated to be a 'deny feature' test, run it once with allow(feature) then run it again with no feature attribute, and compare the results.

[brson]

Similarly to the above, but to avoid creating another custom test runner. The featureck lint could require that compile-test and run-pass contain pairs of feature tests that are identical, one with #[allow(feature)] and one without.

[est31]

With #39059 fixed, maybe this issue can be closed as resolved?

[brson]

Thanks @est31