Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly gate safe keyword in pre-expansion #126757

Merged
merged 1 commit into from
Jun 21, 2024
Merged

Conversation

compiler-errors
Copy link
Member

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

@rustbot 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 Jun 20, 2024
@compiler-errors compiler-errors added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. F-unsafe_extern_blocks `#![feature(unsafe_extern_blocks)]` and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2024
@spastorino
Copy link
Member

r=me with CI green

@spastorino
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

compiler-errors commented Jun 20, 2024

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
Copy link
Member

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
Copy link
Contributor

bors commented Jun 20, 2024

📌 Commit 108b3f2 has been approved by spastorino

It is now in the queue for this repository.

@bors 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 Jun 20, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 20, 2024
Properly gate `safe` keyword in pre-expansion

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

r? `@spastorino` or `@oli-obk`

cc rust-lang#124482 tracking rust-lang#123743
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#125627 (migration lint for `expr2024` for the edition 2024)
 - rust-lang#126481 (Add `powerpc-unknown-openbsd` maintaince status)
 - rust-lang#126613 (Print the tested value in int_log tests)
 - rust-lang#126617 (Expand `avx512_target_feature` to include VEX variants)
 - rust-lang#126686 (Add `#[rustc_dump_{predicates,item_bounds}]`)
 - rust-lang#126700 (Make edition dependent `:expr` macro fragment act like the edition-dependent `:pat` fragment does)
 - rust-lang#126707 (Pass target to inaccessible-temp-dir rmake test)
 - rust-lang#126757 (Properly gate `safe` keyword in pre-expansion)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jun 21, 2024
Properly gate `safe` keyword in pre-expansion

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

r? ``@spastorino`` or ``@oli-obk``

cc rust-lang#124482 tracking rust-lang#123743
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 21, 2024
…kingjubilee

Rollup of 9 pull requests

Successful merges:

 - rust-lang#124101 (Add PidFd::{kill, wait, try_wait})
 - rust-lang#126125 (Improve conflict marker recovery)
 - rust-lang#126481 (Add `powerpc-unknown-openbsd` maintaince status)
 - rust-lang#126613 (Print the tested value in int_log tests)
 - rust-lang#126617 (Expand `avx512_target_feature` to include VEX variants)
 - rust-lang#126707 (Pass target to inaccessible-temp-dir rmake test)
 - rust-lang#126712 (Migrate `relocation-model`, `error-writing-dependencies` and `crate-name-priority` `run-make` tests to rmake)
 - rust-lang#126757 (Properly gate `safe` keyword in pre-expansion)
 - rust-lang#126758 (Do not allow safe/unsafe on static and fn items)

r? `@ghost`
`@rustbot` modify labels: rollup
@workingjubilee
Copy link
Member

@bors p=1 rollup=never

@bors
Copy link
Contributor

bors commented Jun 21, 2024

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

@bors
Copy link
Contributor

bors commented Jun 21, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 21, 2024
@bors bors merged commit 4e6de37 into rust-lang:master Jun 21, 2024
7 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 21, 2024
@rust-timer
Copy link
Collaborator

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%)

@rustbot rustbot added the perf-regression Performance regression. label Jun 21, 2024
@Mark-Simulacrum Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Jun 23, 2024
@Mark-Simulacrum
Copy link
Member

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

@apiraino
Copy link
Contributor

apiraino commented Jul 2, 2024

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle.

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jul 2, 2024
@cuviper cuviper mentioned this pull request Jul 5, 2024
@cuviper cuviper modified the milestones: 1.81.0, 1.80.0 Jul 5, 2024
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 5, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 6, 2024
[beta] backports

- Properly gate `safe` keyword in pre-expansion rust-lang#126757
- Switch back `non_local_definitions` lint to allow-by-default rust-lang#127015
- Stall computing instance for drop shim until it has no unsubstituted const params rust-lang#127068
- Update LLVM submodule rust-lang#127190

r? cuviper
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 6, 2024
[beta] backports

- Properly gate `safe` keyword in pre-expansion rust-lang#126757
- Switch back `non_local_definitions` lint to allow-by-default rust-lang#127015
- Stall computing instance for drop shim until it has no unsubstituted const params rust-lang#127068
- Update LLVM submodule rust-lang#127190
-  Change to the NetBSD archive URL rather than the CDN rust-lang#127232

r? cuviper
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 6, 2024
[beta] backports

- Properly gate `safe` keyword in pre-expansion rust-lang#126757
- Switch back `non_local_definitions` lint to allow-by-default rust-lang#127015
- Stall computing instance for drop shim until it has no unsubstituted const params rust-lang#127068
- Update LLVM submodule rust-lang#127190
-  Change to the NetBSD archive URL rather than the CDN rust-lang#127232

r? cuviper
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 6, 2024
[beta] backports

- Properly gate `safe` keyword in pre-expansion rust-lang#126757
- Switch back `non_local_definitions` lint to allow-by-default rust-lang#127015
- Stall computing instance for drop shim until it has no unsubstituted const params rust-lang#127068
- Update LLVM submodule rust-lang#127190
-  Change to the NetBSD archive URL rather than the CDN rust-lang#127232

r? cuviper
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 3, 2024
…-blocks, r=compiler-errors

Stabilize unsafe extern blocks (RFC 3484)

# Stabilization report

## Summary

This is a tracking issue for the RFC 3484: Unsafe Extern Blocks

We are stabilizing `#![feature(unsafe_extern_blocks)]`, as described in [Unsafe Extern Blocks RFC 3484](rust-lang/rfcs#3484). This feature makes explicit that declaring an extern block is unsafe. Starting in Rust 2024, all extern blocks must be marked as unsafe. In all editions, items within unsafe extern blocks may be marked as safe to use.

RFC: rust-lang/rfcs#3484
Tracking issue: rust-lang#123743

## What is stabilized

### Summary of stabilization

We now need extern blocks to be marked as unsafe and items inside can also have safety modifiers (unsafe or safe), by default items with no modifiers are unsafe to offer easy migration without surprising results.

```rust
unsafe extern {
    // sqrt (from libm) may be called with any `f64`
    pub safe fn sqrt(x: f64) -> f64;

    // strlen (from libc) requires a valid pointer,
    // so we mark it as being an unsafe fn
    pub unsafe fn strlen(p: *const c_char) -> usize;

    // this function doesn't say safe or unsafe, so it defaults to unsafe
    pub fn free(p: *mut core::ffi::c_void);

    pub safe static IMPORTANT_BYTES: [u8; 256];

    pub safe static LINES: SyncUnsafeCell<i32>;
}
```

## Tests

The relevant tests are in `tests/ui/rust-2024/unsafe-extern-blocks`.

## History

- rust-lang#124482
- rust-lang#124455
- rust-lang#125077
- rust-lang#125522
- rust-lang#126738
- rust-lang#126749
- rust-lang#126755
- rust-lang#126757
- rust-lang#126758
- rust-lang#126756
- rust-lang#126973
- rust-lang#127535
- rust-lang/rustfmt#6204

## Unresolved questions

I am not aware of any unresolved questions.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 3, 2024
…-blocks, r=compiler-errors

Stabilize unsafe extern blocks (RFC 3484)

# Stabilization report

## Summary

This is a tracking issue for the RFC 3484: Unsafe Extern Blocks

We are stabilizing `#![feature(unsafe_extern_blocks)]`, as described in [Unsafe Extern Blocks RFC 3484](rust-lang/rfcs#3484). This feature makes explicit that declaring an extern block is unsafe. Starting in Rust 2024, all extern blocks must be marked as unsafe. In all editions, items within unsafe extern blocks may be marked as safe to use.

RFC: rust-lang/rfcs#3484
Tracking issue: rust-lang#123743

## What is stabilized

### Summary of stabilization

We now need extern blocks to be marked as unsafe and items inside can also have safety modifiers (unsafe or safe), by default items with no modifiers are unsafe to offer easy migration without surprising results.

```rust
unsafe extern {
    // sqrt (from libm) may be called with any `f64`
    pub safe fn sqrt(x: f64) -> f64;

    // strlen (from libc) requires a valid pointer,
    // so we mark it as being an unsafe fn
    pub unsafe fn strlen(p: *const c_char) -> usize;

    // this function doesn't say safe or unsafe, so it defaults to unsafe
    pub fn free(p: *mut core::ffi::c_void);

    pub safe static IMPORTANT_BYTES: [u8; 256];

    pub safe static LINES: SyncUnsafeCell<i32>;
}
```

## Tests

The relevant tests are in `tests/ui/rust-2024/unsafe-extern-blocks`.

## History

- rust-lang#124482
- rust-lang#124455
- rust-lang#125077
- rust-lang#125522
- rust-lang#126738
- rust-lang#126749
- rust-lang#126755
- rust-lang#126757
- rust-lang#126758
- rust-lang#126756
- rust-lang#126973
- rust-lang#127535
- rust-lang/rustfmt#6204

## Unresolved questions

I am not aware of any unresolved questions.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 3, 2024
Rollup merge of rust-lang#127921 - spastorino:stabilize-unsafe-extern-blocks, r=compiler-errors

Stabilize unsafe extern blocks (RFC 3484)

# Stabilization report

## Summary

This is a tracking issue for the RFC 3484: Unsafe Extern Blocks

We are stabilizing `#![feature(unsafe_extern_blocks)]`, as described in [Unsafe Extern Blocks RFC 3484](rust-lang/rfcs#3484). This feature makes explicit that declaring an extern block is unsafe. Starting in Rust 2024, all extern blocks must be marked as unsafe. In all editions, items within unsafe extern blocks may be marked as safe to use.

RFC: rust-lang/rfcs#3484
Tracking issue: rust-lang#123743

## What is stabilized

### Summary of stabilization

We now need extern blocks to be marked as unsafe and items inside can also have safety modifiers (unsafe or safe), by default items with no modifiers are unsafe to offer easy migration without surprising results.

```rust
unsafe extern {
    // sqrt (from libm) may be called with any `f64`
    pub safe fn sqrt(x: f64) -> f64;

    // strlen (from libc) requires a valid pointer,
    // so we mark it as being an unsafe fn
    pub unsafe fn strlen(p: *const c_char) -> usize;

    // this function doesn't say safe or unsafe, so it defaults to unsafe
    pub fn free(p: *mut core::ffi::c_void);

    pub safe static IMPORTANT_BYTES: [u8; 256];

    pub safe static LINES: SyncUnsafeCell<i32>;
}
```

## Tests

The relevant tests are in `tests/ui/rust-2024/unsafe-extern-blocks`.

## History

- rust-lang#124482
- rust-lang#124455
- rust-lang#125077
- rust-lang#125522
- rust-lang#126738
- rust-lang#126749
- rust-lang#126755
- rust-lang#126757
- rust-lang#126758
- rust-lang#126756
- rust-lang#126973
- rust-lang#127535
- rust-lang/rustfmt#6204

## Unresolved questions

I am not aware of any unresolved questions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. F-unsafe_extern_blocks `#![feature(unsafe_extern_blocks)]` merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants