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

Don't trigger unsafe_op_in_unsafe_fn for deprecated safe fns #125925

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Jun 3, 2024

@rustbot
Copy link
Collaborator

rustbot commented Jun 3, 2024

r? @Nadrieril

rustbot has assigned @Nadrieril.
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 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 3, 2024
@rust-log-analyzer

This comment has been minimized.

@tbu- tbu- force-pushed the pr_unsafe_env_unsafe_op_in_unsafe_fn branch from e605228 to 42b7153 Compare June 3, 2024 11:17
Comment on lines +96 to +97
if !span.at_least_rust_2024()
&& self.tcx.has_attr(id, sym::rustc_deprecated_safe_2024) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this PR should probably go through as-is to get the fix in as soon as possible, I feel like this system is somewhat brittle, relying on specifically the symbol rustc_deprecated_safe_2024 and the edition being hard coded to check at least 2024. I wonder if it would be a good idea for a follow up PR to somehow make this general in that the attribute could specify an edition in which a function starts becoming unsafe, and check that edition?

However, such a followup assumes that we will make more functions unsafe in the future due to oversight. Ideally we don't have to do that, and I think that the current state of things is such that just about everything has been audited? So perhaps it's not worth such a generalization, since it ideally doesn't happen again.

Copy link
Contributor Author

@tbu- tbu- Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this PR should probably go through as-is to get the fix in as soon as possible, I feel like this system is somewhat brittle, relying on specifically the symbol rustc_deprecated_safe_2024 and the edition being hard coded to check at least 2024. I wonder if it would be a good idea for a follow up PR to somehow make this general in that the attribute could specify an edition in which a function starts becoming unsafe, and check that edition?

Such a more general has been proposed and can probably be implemented. #[rustc_deprecated_safe_2024] was intentionally limited in scope in order to make std::env::{set_var, remove_var} unsafe in Rust 2024. See e.g. #94978.

@Nadrieril
Copy link
Member

Nadrieril commented Jun 5, 2024

I have one style comment, feel free to ignore it or fix it now or fix it in a later PR. r=me if you decide not to fix it now.

@bors delegate+

@bors
Copy link
Contributor

bors commented Jun 5, 2024

✌️ @tbu-, you can now approve this pull request!

If @Nadrieril told you to "r=me" after making some further change, please make that change, then do @bors r=@Nadrieril

@tbu- tbu- force-pushed the pr_unsafe_env_unsafe_op_in_unsafe_fn branch from 42b7153 to bb901a1 Compare June 5, 2024 21:45
@tbu-
Copy link
Contributor Author

tbu- commented Jun 5, 2024

@bors r=Nadrieril

@bors
Copy link
Contributor

bors commented Jun 5, 2024

📌 Commit bb901a1 has been approved by Nadrieril

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 5, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 5, 2024
…safe_fn, r=Nadrieril

Don't trigger `unsafe_op_in_unsafe_fn` for deprecated safe fns

Fixes rust-lang#125875.

Tracking:

- rust-lang#124866
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 6, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#124731 (Add translation support by mdbook-i18n-helpers to bootstrap)
 - rust-lang#125168 (Match ergonomics 2024: align implementation with RFC)
 - rust-lang#125925 (Don't trigger `unsafe_op_in_unsafe_fn` for deprecated safe fns)
 - rust-lang#125966 (Implement `os_string_pathbuf_leak`)
 - rust-lang#125987 (When `derive`ing, account for HRTB on `BareFn` fields)
 - rust-lang#126045 (check_expr_struct_fields: taint context with errors if struct definit…)
 - rust-lang#126048 (Fix typos in cargo-specifics.md)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 6, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#124731 (Add translation support by mdbook-i18n-helpers to bootstrap)
 - rust-lang#125168 (Match ergonomics 2024: align implementation with RFC)
 - rust-lang#125925 (Don't trigger `unsafe_op_in_unsafe_fn` for deprecated safe fns)
 - rust-lang#125987 (When `derive`ing, account for HRTB on `BareFn` fields)
 - rust-lang#126045 (check_expr_struct_fields: taint context with errors if struct definit…)
 - rust-lang#126048 (Fix typos in cargo-specifics.md)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8f04878 into rust-lang:master Jun 6, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 6, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 6, 2024
Rollup merge of rust-lang#125925 - tbu-:pr_unsafe_env_unsafe_op_in_unsafe_fn, r=Nadrieril

Don't trigger `unsafe_op_in_unsafe_fn` for deprecated safe fns

Fixes rust-lang#125875.

Tracking:

- rust-lang#124866
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

std::env::remove_var() becomes unsafe even in Rust 2021
6 participants