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

dtolnay · 2023-10-24T23:17:16Z

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:

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.
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?

rustbot · 2023-10-24T23:17:24Z

r? @cjgillot

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

rustbot · 2023-10-25T00:35:46Z

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

The same file already contains another env!("CFG_RELEASE") on line 632, so it's impossible for this crate to compile without CFG_RELEASE set.

rustbot · 2023-10-25T01:09:13Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

cjgillot · 2023-10-25T17:42:05Z

compiler/rustc_attr/src/builtin.rs

@@ -158,6 +158,8 @@ pub enum Since {
    Version(Version),
    /// Stabilized in the upcoming version, whatever number that is.
    Current,
+    /// Failed to parse a stabilization version.
+    Err,


Should this hold an ErrorGuaranteed or not useful?

I based this on the ExprKind::Err from rustc_ast, which does not hold ErrorGuaranteed.

rust/compiler/rustc_ast/src/ast.rs

Lines 1494 to 1495 in 964ff01

/// Placeholder for an expression that wasn't syntactically well formed in some way.

Err,

cjgillot · 2023-10-25T17:46:38Z

compiler/rustc_attr/src/builtin.rs

@@ -25,8 +25,7 @@ use crate::session_diagnostics::{self, IncorrectReprFormatGenericCause};
 pub const VERSION_PLACEHOLDER: &str = "CURRENT_RUSTC_VERSION";

 pub fn rust_version_symbol() -> Symbol {
-    let version = option_env!("CFG_RELEASE").unwrap_or("<current>");
-    Symbol::intern(&version)
+    Symbol::intern(env!("CFG_RELEASE"))


Should this be in rustc_span::symbol, to avoid reinterning this symbol multiple times?

Yes, I'll take care of that.

cjgillot · 2023-10-26T06:58:10Z

@bors r+

bors · 2023-10-26T06:58:12Z

📌 Commit 1a9ea1f has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-10-26T06:59:22Z

⌛ Testing commit 1a9ea1f with merge 104ac7b...

bors · 2023-10-26T08:53:33Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 104ac7b to master...

rust-timer · 2023-10-26T10:35:27Z

Finished benchmarking commit (104ac7b): 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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.1%	[-1.9%, -0.7%]	3
All ❌✅ (primary)	-	-	0

Cycles

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)	10.2%	[9.6%, 10.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: missing data
Artifact size: 304.45 MiB -> 304.42 MiB (-0.01%)

Avoid repeated interning of `env!("CFG_RELEASE")` Implements `@cjgillot's` suggestion from rust-lang#117148 (comment).

Rollup merge of rust-lang#117188 - dtolnay:symbolenv, r=cjgillot Avoid repeated interning of `env!("CFG_RELEASE")` Implements `@cjgillot's` suggestion from rust-lang#117148 (comment).

Store #[deprecated] attribute's `since` value in parsed form This PR implements the first followup bullet listed in rust-lang#117148 (comment). We centralize error handling to the attribute parsing code in `compiler/rustc_attr/src/builtin.rs`, and thereby remove some awkward error codepaths from later phases of compilation that had to make sense of these #\[deprecated\] attributes, namely `compiler/rustc_passes/src/stability.rs` and `compiler/rustc_middle/src/middle/stability.rs`.

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?

dtolnay added 3 commits October 24, 2023 15:50

Store 'since' attribute as parsed Version

f2ae7b5

Keep track of #[stable] attribute even if version cannot be parsed

ddcb183

Update stability attribute sanity UI test to delete superfluous errors

7b0e315

rustbot assigned cjgillot Oct 24, 2023

dtolnay mentioned this pull request Oct 24, 2023

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

Merged

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. labels Oct 24, 2023

This comment has been minimized.

Sign in to view

Handle structured stable attribute 'since' version in rustdoc

6933a67

This comment has been minimized.

Sign in to view

dtolnay added 2 commits October 24, 2023 18:00

Handle structured stable attribute 'since' version in clippy

fd8907f

CFG_RELEASE is mandatory, no need for option_env

51f7fba

The same file already contains another env!("CFG_RELEASE") on line 632, so it's impossible for this crate to compile without CFG_RELEASE set.

Expose a non-Symbol way to access current rustc version string

1a9ea1f

dtolnay force-pushed the sinceversion branch from bedf004 to 1a9ea1f Compare October 25, 2023 01:11

cjgillot reviewed Oct 25, 2023

View reviewed changes

dtolnay added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 25, 2023

dtolnay mentioned this pull request Oct 25, 2023

Avoid repeated interning of env!("CFG_RELEASE") #117188

Merged

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 26, 2023

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

bors merged commit 104ac7b into rust-lang:master Oct 26, 2023
12 checks passed

rustbot added this to the 1.75.0 milestone Oct 26, 2023

dtolnay deleted the sinceversion branch October 26, 2023 09:03

bors mentioned this pull request Oct 26, 2023

rustdoc: redesign [+]/[−] controls #113074

Closed

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 26, 2023

Rollup merge of rust-lang#117188 - dtolnay:symbolenv, r=cjgillot

2656c98

Avoid repeated interning of `env!("CFG_RELEASE")` Implements `@cjgillot's` suggestion from rust-lang#117148 (comment).

dtolnay mentioned this pull request Oct 30, 2023

Store #[deprecated] attribute's since value in parsed form #117377

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

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

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

dtolnay commented Oct 24, 2023

rustbot commented Oct 24, 2023

This comment has been minimized.

rustbot commented Oct 25, 2023

This comment has been minimized.

rustbot commented Oct 25, 2023

cjgillot Oct 25, 2023

dtolnay Oct 25, 2023

cjgillot Oct 25, 2023

dtolnay Oct 25, 2023

dtolnay Oct 25, 2023

cjgillot commented Oct 26, 2023

bors commented Oct 26, 2023

bors commented Oct 26, 2023

bors commented Oct 26, 2023

rust-timer commented Oct 26, 2023

	/// Placeholder for an expression that wasn't syntactically well formed in some way.
	Err,

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

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

Conversation

dtolnay commented Oct 24, 2023

rustbot commented Oct 24, 2023

This comment has been minimized.

rustbot commented Oct 25, 2023

This comment has been minimized.

rustbot commented Oct 25, 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

cjgillot commented Oct 26, 2023

bors commented Oct 26, 2023

bors commented Oct 26, 2023

bors commented Oct 26, 2023

rust-timer commented Oct 26, 2023

Overall result: no relevant changes - no action needed

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

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