Validate `feature` and `since` values inside `#[stable(…)]` #116773

dtolnay · 2023-10-15T19:36:28Z

Previously the string passed to #[unstable(feature = "...")] would be validated as an identifier, but not #[stable(feature = "...")]. In the standard library there were stable attributes containing the empty string, and kebab-case string, neither of which should be allowed.

Pre-existing validation of unstable:

// src/lib.rs

#![allow(internal_features)]
#![feature(staged_api)]
#![unstable(feature = "kebab-case", issue = "none")]

#[unstable(feature = "kebab-case", issue = "none")]
pub struct Struct;

error[E0546]: 'feature' is not an identifier
 --> src/lib.rs:5:1
  |
5 | #![unstable(feature = "kebab-case", issue = "none")]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

For an unstable attribute, the need for an identifier is obvious because the downstream code needs to write a #![feature(...)] attribute containing that identifier. #![feature(kebab-case)] is not valid syntax and #![feature(kebab_case)] would not work if that is not the name of the feature.

Having a valid identifier even in stable is less essential but still useful because it allows for informative diagnostic about the stabilization of a feature. Compare:

// src/lib.rs

#![allow(internal_features)]
#![feature(staged_api)]
#![stable(feature = "kebab-case", since = "1.0.0")]

#[stable(feature = "kebab-case", since = "1.0.0")]
pub struct Struct;

// src/main.rs

#![feature(kebab_case)]

use repro::Struct;

fn main() {}

error[E0635]: unknown feature `kebab_case`
 --> src/main.rs:3:12
  |
3 | #![feature(kebab_case)]
  |            ^^^^^^^^^^

vs the situation if we correctly use feature = "snake_case" and #![feature(snake_case)], as enforced by this PR:

warning: the feature `snake_case` has been stable since 1.0.0 and no longer requires an attribute to enable
 --> src/main.rs:3:12
  |
3 | #![feature(snake_case)]
  |            ^^^^^^^^^^
  |
  = note: `#[warn(stable_features)]` on by default

rustbot · 2023-10-15T19:36:37Z

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

library/std/src/process.rs

rustbot · 2023-10-15T20:41:01Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

compiler-errors · 2023-10-16T16:23:56Z

tests/ui/repr/16-bit-repr-c-enum.rs

@@ -8,7 +8,7 @@
 #![feature(no_core, lang_items, intrinsics, staged_api, rustc_attrs)]
 #![no_core]
 #![crate_type = "lib"]
-#![stable(feature = "", since = "")]
+#![stable(feature = "intrinsics_for_test", since = "")]


Do we also want to validate the since?

Yes, good call. Would you want it in this PR or separately?

Yeah this PR is fine, just stack on another commit or two

compiler-errors · 2023-10-16T16:29:43Z

r? compiler-errors

dtolnay · 2023-10-16T20:34:52Z

tests/ui/stability-attribute/stability-attribute-sanity.stderr

   |
 LL | #[rustc_const_unstable(feature = "d", issue = "none")]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

-error: invalid stability version found


This older error comes from:

rust/compiler/rustc_passes/src/stability.rs

Line 235 in 4af886f

self.tcx.sess.emit_err(errors::InvalidStability { span, item_sp });

It only kicked in if there was also a deprecated attribute on the same item.

I might follow up with a separate PR to store a more structured representation for the since value than what's currently present in StabilityLevel::Stable. We can eliminate the reparsing done by rustc_passes and make that error unreachable.

rust/compiler/rustc_attr/src/builtin.rs

Lines 185 to 187 in 4af886f

Stable {

/// Rust release which stabilized this feature.

since: Symbol,

It would also be good if since could store an ExprKind::Err-like variant, because currently if since is invalid, rustc goes forward pretending the whole stable attribute does not exist, giving the bogus extra error "function has missing stability attribute".

dtolnay · 2023-10-16T20:36:21Z

tests/ui/stability-attribute/stability-attribute-sanity.rs

    #[deprecated(note = "a")] //~ ERROR missing 'since' [E0542]
    fn f2() { }

-    #[stable(feature = "a", since = "b")]
+    #[stable(feature = "a", since = "4.4.4")]
    #[deprecated(since = "a")] //~ ERROR missing 'note' [E0543]


(In a separate PR) here's another thing to check.

cjgillot · 2023-10-22T10:44:46Z

compiler/rustc_attr/src/builtin.rs

@@ -48,33 +50,28 @@ pub(crate) enum UnsupportedLiteralReason {
    DeprecatedKvPair,
 }

-fn handle_errors(sess: &ParseSess, span: Span, error: AttrError) {
+fn handle_errors(sess: &ParseSess, span: Span, error: AttrError) -> ErrorGuaranteed {


Should we get rid of this function, and emit the errors directly?

Yes, good call. #117064

Eliminate rustc_attrs::builtin::handle_errors in favor of emitting errors directly Suggested in rust-lang#116773 (review). This `handle_errors` function is originally from rust-lang#34531, in which it was useful because it allowed error messages and error codes (`E0542`) for multiple occurrences of the same error to be centralized in one place. For example rather than repeating this diagnostic in 2 places: ```rust span_err!(diagnostic, attr.span, E0542, "missing 'since'"); ``` one could repeat this instead: ```rust handle_errors(diagnostic, attr.span, AttrError::MissingSince); ``` ensuring that all "missing 'since'" errors always remained consistent in message and error code. Over time as error messages and error codes got factored to fluent diagnostics (rust-lang#100836), this rationale no longer applies. The new code has the same benefit while being less verbose (+73, -128). ```rust sess.emit_err(session_diagnostics::MissingSince { span: attr.span }); ``` r? `@cjgillot`

Rollup merge of rust-lang#117064 - dtolnay:handleerrors, r=cjgillot Eliminate rustc_attrs::builtin::handle_errors in favor of emitting errors directly Suggested in rust-lang#116773 (review). This `handle_errors` function is originally from rust-lang#34531, in which it was useful because it allowed error messages and error codes (`E0542`) for multiple occurrences of the same error to be centralized in one place. For example rather than repeating this diagnostic in 2 places: ```rust span_err!(diagnostic, attr.span, E0542, "missing 'since'"); ``` one could repeat this instead: ```rust handle_errors(diagnostic, attr.span, AttrError::MissingSince); ``` ensuring that all "missing 'since'" errors always remained consistent in message and error code. Over time as error messages and error codes got factored to fluent diagnostics (rust-lang#100836), this rationale no longer applies. The new code has the same benefit while being less verbose (+73, -128). ```rust sess.emit_err(session_diagnostics::MissingSince { span: attr.span }); ``` r? `@cjgillot`

bors · 2023-10-23T18:57:45Z

☔ The latest upstream changes (presumably #117087) made this pull request unmergeable. Please resolve the merge conflicts.

dtolnay · 2023-10-23T20:08:10Z

Rebased to resolve conflict with Eliminate rustc_attrs::builtin::handle_errors in favor of emitting errors directly #117064

compiler-errors · 2023-10-23T20:09:57Z

@bors r+

bors · 2023-10-23T20:09:58Z

📌 Commit 6a02e20 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-10-24T15:06:23Z

⌛ Testing commit 6a02e20 with merge 07a4b7e...

bors · 2023-10-24T17:04:42Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 07a4b7e to master...

rust-timer · 2023-10-24T18:14:43Z

Finished benchmarking commit (07a4b7e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.8%	[2.0%, 7.6%]	2
Improvements ✅ (primary)	-3.2%	[-3.2%, -3.2%]	1
Improvements ✅ (secondary)	-1.0%	[-1.0%, -1.0%]	1
All ❌✅ (primary)	-3.2%	[-3.2%, -3.2%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: missing data
Artifact size: 304.29 MiB -> 304.38 MiB (0.03%)

Store #[stable] attribute's `since` value in structured form Followup to rust-lang#116773 (review). Prior to this PR, if you wrote an improper `since` version in a `stable` attribute, such as `#[stable(feature = "foo", since = "wat.0")]`, rustc would emit a diagnostic saying **_'since' must be a Rust version number, such as "1.31.0"_** and then throw out the whole `stable` attribute as if it weren't there. This strategy had 2 problems, both fixed in this PR: 1. If there was also a `#[deprecated]` attribute on the same item, rustc would want to enforce that the stabilization version is older than the deprecation version. This involved reparsing the `stable` attribute's `since` version, with a diagnostic **_invalid stability version found_** if it failed to parse. Of course this diagnostic was unreachable because an invalid `since` version would have already caused the `stable` attribute to be thrown out. This PR deletes that unreachable diagnostic. 2. By throwing out the `stable` attribute when `since` is invalid, you'd end up with a second diagnostic saying **_function has missing stability attribute_** even though your function is not missing a stability attribute. This PR preserves the `stable` attribute even when `since` cannot be parsed, avoiding the misleading second diagnostic. Followups I plan to try next: - Do the same for the `since` value of `#[deprecated]`. - See whether it makes sense to also preserve `stable` and/or `unstable` attributes when they contain an invalid `feature`. What redundant/misleading diagnostics can this eliminate? What problems arise from not having a usable feature name for some API, in the situation that we're already failing compilation, so not concerned about anything that happens in downstream code?

Store #[stable] attribute's `since` value in structured form Followup to rust-lang/rust#116773 (review). Prior to this PR, if you wrote an improper `since` version in a `stable` attribute, such as `#[stable(feature = "foo", since = "wat.0")]`, rustc would emit a diagnostic saying **_'since' must be a Rust version number, such as "1.31.0"_** and then throw out the whole `stable` attribute as if it weren't there. This strategy had 2 problems, both fixed in this PR: 1. If there was also a `#[deprecated]` attribute on the same item, rustc would want to enforce that the stabilization version is older than the deprecation version. This involved reparsing the `stable` attribute's `since` version, with a diagnostic **_invalid stability version found_** if it failed to parse. Of course this diagnostic was unreachable because an invalid `since` version would have already caused the `stable` attribute to be thrown out. This PR deletes that unreachable diagnostic. 2. By throwing out the `stable` attribute when `since` is invalid, you'd end up with a second diagnostic saying **_function has missing stability attribute_** even though your function is not missing a stability attribute. This PR preserves the `stable` attribute even when `since` cannot be parsed, avoiding the misleading second diagnostic. Followups I plan to try next: - Do the same for the `since` value of `#[deprecated]`. - See whether it makes sense to also preserve `stable` and/or `unstable` attributes when they contain an invalid `feature`. What redundant/misleading diagnostics can this eliminate? What problems arise from not having a usable feature name for some API, in the situation that we're already failing compilation, so not concerned about anything that happens in downstream code?

Store #[stable] attribute's `since` value in structured form Followup to rust-lang/rust#116773 (review). Prior to this PR, if you wrote an improper `since` version in a `stable` attribute, such as `#[stable(feature = "foo", since = "wat.0")]`, rustc would emit a diagnostic saying **_'since' must be a Rust version number, such as "1.31.0"_** and then throw out the whole `stable` attribute as if it weren't there. This strategy had 2 problems, both fixed in this PR: 1. If there was also a `#[deprecated]` attribute on the same item, rustc would want to enforce that the stabilization version is older than the deprecation version. This involved reparsing the `stable` attribute's `since` version, with a diagnostic **_invalid stability version found_** if it failed to parse. Of course this diagnostic was unreachable because an invalid `since` version would have already caused the `stable` attribute to be thrown out. This PR deletes that unreachable diagnostic. 2. By throwing out the `stable` attribute when `since` is invalid, you'd end up with a second diagnostic saying **_function has missing stability attribute_** even though your function is not missing a stability attribute. This PR preserves the `stable` attribute even when `since` cannot be parsed, avoiding the misleading second diagnostic. Followups I plan to try next: - Do the same for the `since` value of `#[deprecated]`. - See whether it makes sense to also preserve `stable` and/or `unstable` attributes when they contain an invalid `feature`. What redundant/misleading diagnostics can this eliminate? What problems arise from not having a usable feature name for some API, in the situation that we're already failing compilation, so not concerned about anything that happens in downstream code?

…rrors Validate `feature` and `since` values inside `#[stable(…)]` Previously the string passed to `#[unstable(feature = "...")]` would be validated as an identifier, but not `#[stable(feature = "...")]`. In the standard library there were `stable` attributes containing the empty string, and kebab-case string, neither of which should be allowed. Pre-existing validation of `unstable`: ```rust // src/lib.rs #![allow(internal_features)] #![feature(staged_api)] #![unstable(feature = "kebab-case", issue = "none")] #[unstable(feature = "kebab-case", issue = "none")] pub struct Struct; ``` ```console error[E0546]: 'feature' is not an identifier --> src/lib.rs:5:1 | 5 | #![unstable(feature = "kebab-case", issue = "none")] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ``` For an `unstable` attribute, the need for an identifier is obvious because the downstream code needs to write a `#![feature(...)]` attribute containing that identifier. `#![feature(kebab-case)]` is not valid syntax and `#![feature(kebab_case)]` would not work if that is not the name of the feature. Having a valid identifier even in `stable` is less essential but still useful because it allows for informative diagnostic about the stabilization of a feature. Compare: ```rust // src/lib.rs #![allow(internal_features)] #![feature(staged_api)] #![stable(feature = "kebab-case", since = "1.0.0")] #[stable(feature = "kebab-case", since = "1.0.0")] pub struct Struct; ``` ```rust // src/main.rs #![feature(kebab_case)] use repro::Struct; fn main() {} ``` ```console error[E0635]: unknown feature `kebab_case` --> src/main.rs:3:12 | 3 | #![feature(kebab_case)] | ^^^^^^^^^^ ``` vs the situation if we correctly use `feature = "snake_case"` and `#![feature(snake_case)]`, as enforced by this PR: ```console warning: the feature `snake_case` has been stable since 1.0.0 and no longer requires an attribute to enable --> src/main.rs:3:12 | 3 | #![feature(snake_case)] | ^^^^^^^^^^ | = note: `#[warn(stable_features)]` on by default ```

rustbot assigned davidtwco Oct 15, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 15, 2023

dtolnay commented Oct 15, 2023

View reviewed changes

library/std/src/process.rs Outdated Show resolved Hide resolved

dtolnay removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 15, 2023

This comment has been minimized.

Sign in to view

dtolnay force-pushed the validatestable branch from 64fddb6 to 67b1755 Compare October 15, 2023 20:15

This comment has been minimized.

Sign in to view

dtolnay force-pushed the validatestable branch from 67b1755 to 920d8c8 Compare October 15, 2023 20:40

dtolnay force-pushed the validatestable branch from 920d8c8 to 303475f Compare October 16, 2023 04:31

compiler-errors reviewed Oct 16, 2023

View reviewed changes

rustbot assigned compiler-errors and unassigned davidtwco Oct 16, 2023

dtolnay changed the title ~~Validate feature name identifier inside #[stable(feature = "...")]~~ Validate feature and since values inside #[stable(…)] Oct 16, 2023

dtolnay commented Oct 16, 2023

View reviewed changes

cjgillot reviewed Oct 22, 2023

View reviewed changes

dtolnay mentioned this pull request Oct 22, 2023

Eliminate rustc_attrs::builtin::handle_errors in favor of emitting errors directly #117064

Merged

dtolnay added 4 commits October 23, 2023 13:02

Validate #[stable(feature = "…")] identifier

9853f61

Fix invalid stability attribute features in standard library

67ea798

Fix stable feature names in tests

01b9091

Validate since value in stable attribute

82ed3f5

Update since stability attributes in tests

6a02e20

dtolnay force-pushed the validatestable branch from 0ea5e8f to 6a02e20 Compare October 23, 2023 20:07

compiler-errors approved these changes Oct 23, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 24, 2023

bors merged commit 07a4b7e into rust-lang:master Oct 24, 2023
12 checks passed

rustbot added this to the 1.75.0 milestone Oct 24, 2023

dtolnay deleted the validatestable branch October 24, 2023 22:50

dtolnay mentioned this pull request Oct 24, 2023

Store #[stable] attribute's since value in structured form #117148

Merged

github-actions bot mentioned this pull request Oct 31, 2023

Automated pull from upstream master ferrocene/ferrocene#74

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate `feature` and `since` values inside `#[stable(…)]` #116773

Validate `feature` and `since` values inside `#[stable(…)]` #116773

dtolnay commented Oct 15, 2023

rustbot commented Oct 15, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented Oct 15, 2023

compiler-errors Oct 16, 2023

dtolnay Oct 16, 2023

compiler-errors Oct 16, 2023

compiler-errors commented Oct 16, 2023

dtolnay Oct 16, 2023

dtolnay Oct 24, 2023

dtolnay Oct 16, 2023

dtolnay Oct 30, 2023

cjgillot Oct 22, 2023

dtolnay Oct 22, 2023

bors commented Oct 23, 2023

dtolnay commented Oct 23, 2023

compiler-errors commented Oct 23, 2023

bors commented Oct 23, 2023

bors commented Oct 24, 2023

bors commented Oct 24, 2023

rust-timer commented Oct 24, 2023

	Stable {
	/// Rust release which stabilized this feature.
	since: Symbol,

Validate feature and since values inside #[stable(…)] #116773

Validate feature and since values inside #[stable(…)] #116773

Conversation

dtolnay commented Oct 15, 2023

rustbot commented Oct 15, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented Oct 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Oct 23, 2023

dtolnay commented Oct 23, 2023

compiler-errors commented Oct 23, 2023

bors commented Oct 23, 2023

bors commented Oct 24, 2023

bors commented Oct 24, 2023

rust-timer commented Oct 24, 2023

Overall result: no relevant changes - no action needed

Validate `feature` and `since` values inside `#[stable(…)]` #116773

Validate `feature` and `since` values inside `#[stable(…)]` #116773