terminology: #[feature] enables a feature (instead of "declaring" or "activating" it) #131321

RalfJung · 2024-10-06T09:30:24Z

Mostly, we currently call a feature that has a corresponding #[feature(name)] attribute in the current crate a "declared" feature. I think that is confusing as it does not align with what "declaring" usually means. Furthermore, we also refer to #[stable]/#[unstable] as declaring a feature (e.g. in these diagnostics), which aligns better with what "declaring" usually means. To make things worse, the functions tcx.features().active(...) and tcx.features().declared(...) both exist and they are doing almost the same thing (testing whether a corresponding #[feature(name)] exists) except that active would ICE if the feature is not an unstable lang feature. On top of this, the callback when a feature is activated/declared is called set_enabled, and many comments also talk about "enabling" a feature.

So really, our terminology is just a mess.

I would suggest we use "declaring a feature" for saying that something is/was guarded by a feature (e.g. #[stable]/#[unstable]), and "enabling a feature" for #[feature(name)]. This PR implements that.

rustbot · 2024-10-06T09:30:32Z

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-10-06T09:55:34Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

est31 · 2024-10-08T01:23:34Z

r? @est31

@bors r+

bors · 2024-10-08T01:23:37Z

📌 Commit 1df170c has been approved by est31

It is now in the queue for this repository.

compiler/rustc_expand/src/config.rs

nnethercote · 2024-10-08T06:32:22Z

This is an improvement over "declared", but I was going to suggest enable/enabled over activate/active. Several of the existing comments already used enable/enabled.

RalfJung · 2024-10-08T06:49:23Z

Hm... both "active" and "enabled" are used then, it seems.

@rust-lang/compiler any preference?

@bors r-

dianne · 2024-10-08T07:16:06Z

If you're looking to make comments use consistent terminology too, the doc comments in rustc_middle/src/middle/stability.rs use yet another term: features being "provided".

nnethercote · 2024-10-08T08:34:37Z

It's not a perfect analogy, but cargo features are "enabled".

RalfJung · 2024-10-08T08:40:28Z

Thinking about it, I think I like "enabled" more.

rustbot · 2024-10-08T12:07:23Z

These commits modify compiler targets.
(See the Target Tier Policy.)

RalfJung · 2024-10-08T12:08:55Z

I now updated this to "enabling" a feature.

@rustbot ready

rust-timer · 2024-10-09T11:34:11Z

Finished benchmarking commit (ded5475): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary -0.2%)

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)	3.4%	[3.4%, 3.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.8%	[-3.8%, -3.8%]	1
All ❌✅ (primary)	-	-	0

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: 775.847s -> 775.025s (-0.11%)
Artifact size: 329.70 MiB -> 329.61 MiB (-0.03%)

RalfJung · 2024-10-09T11:37:23Z

Oh wow, it basically makes no difference. That's surprising. I guess the hash set is usually empty and so checking it is pretty fast, or so?

I can still bring back the bools as private fields if you prefer, to avoid all these hash set tests.

RalfJung · 2024-10-16T05:56:58Z

@nnethercote ehuss is not on the compiler team, so it seems odd to ask them for review here? This is basically entirely an internal compiler change. The term shows up in one error message, but it's not like we super carefully review those.

ehuss · 2024-10-21T14:41:47Z

Yea, I have nothing to say here (other than the perf experiment commit will likely cause a lot of conflicts for other PRs). I'll reassign to the original reviewer.

r? @est31

nnethercote · 2024-10-22T05:14:33Z

compiler/rustc_query_system/src/ich/impls_syntax.rs


-        self.all_features()[..].hash_stable(hcx, hasher);
+        self.all_lang_features()[..].hash_stable(hcx, hasher);


Why the change from all_features to all_lang_features?

Oh, you renamed the function above.

nnethercote · 2024-10-22T05:20:25Z

Ok, r=me for the first two commits. I like the idea of the third one -- good to avoid duplicating the data -- but I think it's worth putting that in a separate PR because it touches so many lines.

RalfJung · 2024-10-22T06:37:05Z

Okay I will move 6c38ef0 to a separate PR.

…r "activating" it)

… debug assertions

RalfJung · 2024-10-22T06:39:57Z

@bors r=nnethercote

bors · 2024-10-22T06:40:00Z

📌 Commit 1381773 has been approved by nnethercote

It is now in the queue for this repository.

bors · 2024-10-22T11:02:39Z

⌛ Testing commit 1381773 with merge bca5fde...

bors · 2024-10-22T14:15:29Z

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing bca5fde to master...

rust-timer · 2024-10-22T15:32:35Z

Finished benchmarking commit (bca5fde): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.1%	[-3.1%, -3.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.1%	[-3.1%, -3.1%]	1

Max RSS (memory usage)

Results (secondary -3.1%)

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)	-3.1%	[-3.1%, -3.1%]	1
All ❌✅ (primary)	-	-	0

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: 780.605s -> 781.36s (0.10%)
Artifact size: 333.69 MiB -> 333.62 MiB (-0.02%)

…=<try> nightly feature tracking: get rid of the per-feature bool fields The `struct Features` that tracks which features are enabled has a ton of public `bool`-typed fields that are basically caching the result of looking up the corresponding feature in `enabled_lang_features`. Having public fields with an invariant is not great, so at least they should be made private. However, it turns out caching these lookups is actually [not worth it](rust-lang#131321 (comment)), so this PR just entirely gets rid of these fields. (The alternative would be to make them private and have a method for each of them to expose them in a read-only way. Most of the diff of this PR would be the same in that case.) r? `@nnethercote`

…=nnethercote nightly feature tracking: get rid of the per-feature bool fields The `struct Features` that tracks which features are enabled has a ton of public `bool`-typed fields that are basically caching the result of looking up the corresponding feature in `enabled_lang_features`. Having public fields with an invariant is not great, so at least they should be made private. However, it turns out caching these lookups is actually [not worth it](rust-lang#131321 (comment)), so this PR just entirely gets rid of these fields. (The alternative would be to make them private and have a method for each of them to expose them in a read-only way. Most of the diff of this PR would be the same in that case.) r? `@nnethercote`

…ercote terminology: #[feature] *enables* a feature (instead of "declaring" or "activating" it) Mostly, we currently call a feature that has a corresponding `#[feature(name)]` attribute in the current crate a "declared" feature. I think that is confusing as it does not align with what "declaring" usually means. Furthermore, we *also* refer to `#[stable]`/`#[unstable]` as *declaring* a feature (e.g. in [these diagnostics](https://github.com/rust-lang/rust/blob/f25e5abea229a6b6aa77b45e21cb784e785c6040/compiler/rustc_passes/messages.ftl#L297-L301)), which aligns better with what "declaring" usually means. To make things worse, the functions `tcx.features().active(...)` and `tcx.features().declared(...)` both exist and they are doing almost the same thing (testing whether a corresponding `#[feature(name)]` exists) except that `active` would ICE if the feature is not an unstable lang feature. On top of this, the callback when a feature is activated/declared is called `set_enabled`, and many comments also talk about "enabling" a feature. So really, our terminology is just a mess. I would suggest we use "declaring a feature" for saying that something is/was guarded by a feature (e.g. `#[stable]`/`#[unstable]`), and "enabling a feature" for `#[feature(name)]`. This PR implements that.

rustbot assigned nnethercote Oct 6, 2024

This comment has been minimized.

Sign in to view

RalfJung force-pushed the feature-activation branch from 8c5bf45 to 2c37795 Compare October 6, 2024 09:55

This comment has been minimized.

Sign in to view

RalfJung force-pushed the feature-activation branch from 2c37795 to 1df170c Compare October 6, 2024 11:31

rustbot assigned est31 Oct 8, 2024

rustbot unassigned nnethercote Oct 8, 2024

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 8, 2024

nnethercote reviewed Oct 8, 2024

View reviewed changes

compiler/rustc_expand/src/config.rs Outdated Show resolved Hide resolved

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 8, 2024

RalfJung force-pushed the feature-activation branch from 1df170c to 3b7ed10 Compare October 8, 2024 12:07

RalfJung changed the title ~~terminology: #[feature] *activates* a feature (instead of "declaring" it)~~ terminology: #[feature] *enables* a feature (instead of "declaring" or "activating" it) Oct 8, 2024

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

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 9, 2024

rustbot assigned est31 and unassigned ehuss Oct 21, 2024

nnethercote reviewed Oct 22, 2024

View reviewed changes

nnethercote assigned nnethercote and unassigned est31 Oct 22, 2024

RalfJung added 2 commits October 22, 2024 07:37

terminology: #[feature] *enables* a feature (instead of "declaring" o…

46ce5cb

…r "activating" it)

make some rustc_feature internals private, and ensure invariants with…

1381773

… debug assertions

RalfJung force-pushed the feature-activation branch from 6c38ef0 to 1381773 Compare October 22, 2024 06:38

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 22, 2024

RalfJung mentioned this pull request Oct 22, 2024

nightly feature tracking: get rid of the per-feature bool fields #132027

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 22, 2024

bors merged commit bca5fde into rust-lang:master Oct 22, 2024
7 checks passed

rustbot added this to the 1.84.0 milestone Oct 22, 2024

RalfJung deleted the feature-activation branch October 22, 2024 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

terminology: #[feature] enables a feature (instead of "declaring" or "activating" it) #131321

terminology: #[feature] enables a feature (instead of "declaring" or "activating" it) #131321

RalfJung commented Oct 6, 2024 •

edited

Loading

rustbot commented Oct 6, 2024

This comment has been minimized.

rustbot commented Oct 6, 2024

This comment has been minimized.

est31 commented Oct 8, 2024

bors commented Oct 8, 2024

nnethercote commented Oct 8, 2024

RalfJung commented Oct 8, 2024

dianne commented Oct 8, 2024

nnethercote commented Oct 8, 2024

RalfJung commented Oct 8, 2024 via email

rustbot commented Oct 8, 2024

RalfJung commented Oct 8, 2024

rust-timer commented Oct 9, 2024

RalfJung commented Oct 9, 2024

RalfJung commented Oct 16, 2024 •

edited

Loading

ehuss commented Oct 21, 2024

nnethercote Oct 22, 2024

nnethercote Oct 22, 2024

nnethercote commented Oct 22, 2024

RalfJung commented Oct 22, 2024

RalfJung commented Oct 22, 2024

bors commented Oct 22, 2024

bors commented Oct 22, 2024

bors commented Oct 22, 2024

rust-timer commented Oct 22, 2024


		self.all_features()[..].hash_stable(hcx, hasher);
		self.all_lang_features()[..].hash_stable(hcx, hasher);

terminology: #[feature] *enables* a feature (instead of "declaring" or "activating" it) #131321

terminology: #[feature] *enables* a feature (instead of "declaring" or "activating" it) #131321

Conversation

RalfJung commented Oct 6, 2024 • edited Loading

rustbot commented Oct 6, 2024

This comment has been minimized.

rustbot commented Oct 6, 2024

This comment has been minimized.

est31 commented Oct 8, 2024

bors commented Oct 8, 2024

nnethercote commented Oct 8, 2024

RalfJung commented Oct 8, 2024

dianne commented Oct 8, 2024

nnethercote commented Oct 8, 2024

RalfJung commented Oct 8, 2024 via email

rustbot commented Oct 8, 2024

RalfJung commented Oct 8, 2024

rust-timer commented Oct 9, 2024

Overall result: ❌ regressions - no action needed

RalfJung commented Oct 9, 2024

RalfJung commented Oct 16, 2024 • edited Loading

ehuss commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnethercote commented Oct 22, 2024

RalfJung commented Oct 22, 2024

RalfJung commented Oct 22, 2024

bors commented Oct 22, 2024

bors commented Oct 22, 2024

bors commented Oct 22, 2024

rust-timer commented Oct 22, 2024

Overall result: ✅ improvements - no action needed

terminology: #[feature] enables a feature (instead of "declaring" or "activating" it) #131321

terminology: #[feature] enables a feature (instead of "declaring" or "activating" it) #131321

RalfJung commented Oct 6, 2024 •

edited

Loading

RalfJung commented Oct 16, 2024 •

edited

Loading