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.

Sign up for GitHub

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
Merged

Don't trigger unsafe_op_in_unsafe_fn for deprecated safe fns #125925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 47 additions & 34 deletions compiler/rustc_mir_build/src/check_unsafety.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,33 @@ impl<'tcx> UnsafetyVisitor<'_, 'tcx> {
}
}

fn emit_deprecated_safe_fn_call(&self, span: Span, kind: &UnsafeOpKind) -> bool {
match kind {
// Allow calls to deprecated-safe unsafe functions if the caller is
// from an edition before 2024.
&UnsafeOpKind::CallToUnsafeFunction(Some(id))
if !span.at_least_rust_2024()
&& self.tcx.has_attr(id, sym::rustc_deprecated_safe_2024) =>
Comment on lines +96 to +97
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 •
edited
Loading

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.

{
self.tcx.emit_node_span_lint(
DEPRECATED_SAFE,
self.hir_context,
span,
CallToDeprecatedSafeFnRequiresUnsafe {
span,
function: with_no_trimmed_paths!(self.tcx.def_path_str(id)),
sub: CallToDeprecatedSafeFnRequiresUnsafeSub {
left: span.shrink_to_lo(),
right: span.shrink_to_hi(),
},
},
);
true
}
_ => false,
}
}

fn requires_unsafe(&mut self, span: Span, kind: UnsafeOpKind) {
let unsafe_op_in_unsafe_fn_allowed = self.unsafe_op_in_unsafe_fn_allowed();
match self.safety_context {
Expand All @@ -101,43 +128,29 @@ impl<'tcx> UnsafetyVisitor<'_, 'tcx> {
}
SafetyContext::UnsafeFn if unsafe_op_in_unsafe_fn_allowed => {}
SafetyContext::UnsafeFn => {
// unsafe_op_in_unsafe_fn is disallowed
kind.emit_unsafe_op_in_unsafe_fn_lint(
self.tcx,
self.hir_context,
span,
self.suggest_unsafe_block,
);
self.suggest_unsafe_block = false;
}
SafetyContext::Safe => match kind {
// Allow calls to deprecated-safe unsafe functions if the
// caller is from an edition before 2024.
UnsafeOpKind::CallToUnsafeFunction(Some(id))
if !span.at_least_rust_2024()
&& self.tcx.has_attr(id, sym::rustc_deprecated_safe_2024) =>
{
self.tcx.emit_node_span_lint(
DEPRECATED_SAFE,
let deprecated_safe_fn = self.emit_deprecated_safe_fn_call(span, &kind);
if !deprecated_safe_fn {
// unsafe_op_in_unsafe_fn is disallowed
kind.emit_unsafe_op_in_unsafe_fn_lint(
self.tcx,
self.hir_context,
span,
CallToDeprecatedSafeFnRequiresUnsafe {
span,
function: with_no_trimmed_paths!(self.tcx.def_path_str(id)),
sub: CallToDeprecatedSafeFnRequiresUnsafeSub {
left: span.shrink_to_lo(),
right: span.shrink_to_hi(),
},
},
)
self.suggest_unsafe_block,
);
self.suggest_unsafe_block = false;
}
_ => kind.emit_requires_unsafe_err(
self.tcx,
span,
self.hir_context,
unsafe_op_in_unsafe_fn_allowed,
),
},
}
SafetyContext::Safe => {
let deprecated_safe_fn = self.emit_deprecated_safe_fn_call(span, &kind);
if !deprecated_safe_fn {
kind.emit_requires_unsafe_err(
self.tcx,
span,
self.hir_context,
unsafe_op_in_unsafe_fn_allowed,
);
}
}
}
}

Expand Down
27 changes: 23 additions & 4 deletions tests/ui/rust-2024/unsafe-env.e2021.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,42 @@
error[E0133]: call to unsafe function `unsafe_fn` is unsafe and requires unsafe block
--> $DIR/unsafe-env.rs:15:9
|
LL | unsafe_fn();
| ^^^^^^^^^^^ call to unsafe function
|
= note: for more information, see issue #71668 <https://github.com/rust-lang/rust/issues/71668>
= note: consult the function's documentation for information on how to avoid undefined behavior
note: an unsafe function restricts its caller, but its body is safe by default
--> $DIR/unsafe-env.rs:9:1
|
LL | unsafe fn unsafe_fn() {
| ^^^^^^^^^^^^^^^^^^^^^
note: the lint level is defined here
--> $DIR/unsafe-env.rs:8:8
|
LL | #[deny(unsafe_op_in_unsafe_fn)]
| ^^^^^^^^^^^^^^^^^^^^^^

error[E0133]: call to unsafe function `unsafe_fn` is unsafe and requires unsafe function or block
--> $DIR/unsafe-env.rs:23:5
--> $DIR/unsafe-env.rs:33:5
|
LL | unsafe_fn();
| ^^^^^^^^^^^ call to unsafe function
|
= note: consult the function's documentation for information on how to avoid undefined behavior

error: unnecessary `unsafe` block
--> $DIR/unsafe-env.rs:26:5
--> $DIR/unsafe-env.rs:36:5
|
LL | unsafe {
| ^^^^^^ unnecessary `unsafe` block
|
note: the lint level is defined here
--> $DIR/unsafe-env.rs:11:8
--> $DIR/unsafe-env.rs:21:8
|
LL | #[deny(unused_unsafe)]
| ^^^^^^^^^^^^^

error: aborting due to 2 previous errors
error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0133`.
49 changes: 43 additions & 6 deletions tests/ui/rust-2024/unsafe-env.e2024.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,76 @@
error[E0133]: call to unsafe function `std::env::set_var` is unsafe and requires unsafe block
--> $DIR/unsafe-env.rs:10:5
|
LL | env::set_var("FOO", "BAR");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ call to unsafe function
|
= note: for more information, see issue #71668 <https://github.com/rust-lang/rust/issues/71668>
= note: consult the function's documentation for information on how to avoid undefined behavior
note: an unsafe function restricts its caller, but its body is safe by default
--> $DIR/unsafe-env.rs:9:1
|
LL | unsafe fn unsafe_fn() {
| ^^^^^^^^^^^^^^^^^^^^^
note: the lint level is defined here
--> $DIR/unsafe-env.rs:8:8
|
LL | #[deny(unsafe_op_in_unsafe_fn)]
| ^^^^^^^^^^^^^^^^^^^^^^

error[E0133]: call to unsafe function `std::env::remove_var` is unsafe and requires unsafe block
--> $DIR/unsafe-env.rs:12:5
|
LL | env::remove_var("FOO");
| ^^^^^^^^^^^^^^^^^^^^^^ call to unsafe function
|
= note: for more information, see issue #71668 <https://github.com/rust-lang/rust/issues/71668>
= note: consult the function's documentation for information on how to avoid undefined behavior

error[E0133]: call to unsafe function `unsafe_fn` is unsafe and requires unsafe block
--> $DIR/unsafe-env.rs:15:9
|
LL | unsafe_fn();
| ^^^^^^^^^^^ call to unsafe function
|
= note: for more information, see issue #71668 <https://github.com/rust-lang/rust/issues/71668>
= note: consult the function's documentation for information on how to avoid undefined behavior

error[E0133]: call to unsafe function `set_var` is unsafe and requires unsafe block
--> $DIR/unsafe-env.rs:13:5
--> $DIR/unsafe-env.rs:23:5
|
LL | env::set_var("FOO", "BAR");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ call to unsafe function
|
= note: consult the function's documentation for information on how to avoid undefined behavior

error[E0133]: call to unsafe function `remove_var` is unsafe and requires unsafe block
--> $DIR/unsafe-env.rs:15:5
--> $DIR/unsafe-env.rs:25:5
|
LL | env::remove_var("FOO");
| ^^^^^^^^^^^^^^^^^^^^^^ call to unsafe function
|
= note: consult the function's documentation for information on how to avoid undefined behavior

error[E0133]: call to unsafe function `unsafe_fn` is unsafe and requires unsafe block
--> $DIR/unsafe-env.rs:23:5
--> $DIR/unsafe-env.rs:33:5
|
LL | unsafe_fn();
| ^^^^^^^^^^^ call to unsafe function
|
= note: consult the function's documentation for information on how to avoid undefined behavior

error: unnecessary `unsafe` block
--> $DIR/unsafe-env.rs:26:5
--> $DIR/unsafe-env.rs:36:5
|
LL | unsafe {
| ^^^^^^ unnecessary `unsafe` block
|
note: the lint level is defined here
--> $DIR/unsafe-env.rs:11:8
--> $DIR/unsafe-env.rs:21:8
|
LL | #[deny(unused_unsafe)]
| ^^^^^^^^^^^^^

error: aborting due to 4 previous errors
error: aborting due to 7 previous errors

For more information about this error, try `rustc --explain E0133`.
12 changes: 11 additions & 1 deletion tests/ui/rust-2024/unsafe-env.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,17 @@

use std::env;

unsafe fn unsafe_fn() {}
#[deny(unsafe_op_in_unsafe_fn)]
unsafe fn unsafe_fn() {
env::set_var("FOO", "BAR");
//[e2024]~^ ERROR call to unsafe function `std::env::set_var` is unsafe
env::remove_var("FOO");
//[e2024]~^ ERROR call to unsafe function `std::env::remove_var` is unsafe
if false {
unsafe_fn();
//~^ ERROR call to unsafe function `unsafe_fn` is unsafe
}
}
fn safe_fn() {}

#[deny(unused_unsafe)]
Expand Down
Loading