Properly gate safe keyword in pre-expansion

[compiler-errors]

This PR gates safe keyword in pre-expansion contexts. Should mitigate the fallout of #126755, which is that safe is now usable on beta lol.

r? @spastorino or @oli-obk

cc #124482 tracking #123743

[spastorino]

r=me with CI green

[spastorino]

Well ... maybe before merging this let me see if I can quickly fix the issue. We can have this to approve it and backport quickly but if I can do a right fix is going to be better as the backport can apply cleanly.

[compiler-errors]

What change do you have in mind, @spastorino?

I'd kinda rather we backport something simple like this rather than a more invasive change, like splitting out Safety into Unsafe/Safe/Default which will require changing feature gating further.

My understanding is that after this PR lands, nothing needs to be futher backported since this is the only thing that has regressed for users not using stable features.

[compiler-errors]

Specifically, how is this fix not sufficient? Usage of a new unstable keyword should be gated in the parser regardless of if we need to do further validation later on in ast_validation or lowering.

[spastorino]

Michael this #126758 is the fix I was talking about. Yes, you're right that safe should be gated, so we should probably also apply your patch but remember that unsafe extern blocks just need a stabilization report and the feature flag is going away. As soon as the feature flag goes away, this fix is also going to be lost.

[spastorino]

I meant, it's true though that in terms of backport we can apply yours but if we don't apply #126758 we risk reintroducing the regression (that's what I meant)

[compiler-errors]

if we don't apply #126758 we risk reintroducing the regression (that's what I meant)

I think the regression you mean is #126749, which is a separate issue from #126755.

The fact that we don't validate where a safe keyword is allowed is also a problem, and should be fixed in #126758, but I intentionally split out the issue #126755 to recognize that we are accidentally unintentionally stabilizing the safe keyword in function signatures in macro/cfg-out'd code on beta currently.

I still think we should land this ASAP. After that, I don't see why we need to beta backport #126758 since after this PR it only affects nightly code.

[spastorino]

Agreed on everything you've said. The relationship I was talking about was exactly what you said, the important bug to address quickly is the regression on stable and your PR addressed this while we have the feature flag, my PR addresses it when the thing is stabilized.

Agreed that we should apply both PRs and backport yours.

@bors r+ rollup

[bors]

📌 Commit 108b3f2 has been approved by spastorino

It is now in the queue for this repository.

[workingjubilee]

@bors p=1 rollup=never

[bors]

⌛ Testing commit 108b3f2 with merge 4e6de37...

[bors]

☀️ Test successful - checks-actions
Approved by: spastorino
Pushing 4e6de37 to master...

[rust-timer]

Finished benchmarking commit (4e6de37): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.3%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.2%, 0.3%]	4

Max RSS (memory usage)

Results (secondary 3.5%)

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

Cycles

Results (secondary 0.8%)

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.8%	[0.7%, 0.9%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 695.451s -> 698.008s (0.37%)
Artifact size: 326.69 MiB -> 326.58 MiB (-0.03%)

[Mark-Simulacrum]

Looks like a minimal regression (maybe spurious/bimodality) and the change is clearly needed. Marking as triaged.