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

Treat safe target_feature functions as unsafe by default [less invasive variant] #134353

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 15, 2024

This unblocks

As I stated in #134090 (comment) I think the previous impl was too easy to get wrong, as by default it treated safe target feature functions as safe and had to add additional checks for when they weren't. Now the logic is inverted. By default they are unsafe and you have to explicitly handle safe target feature functions.

This is the less (imo) invasive variant of #134317, as it doesn't require changing the Safety enum, so it only affects FnDefs and nothing else, as it should.

TODO before landing:

  • Replace the Option<Safety> hack with a proper enum around that.

Sorry, something went wrong.

@rustbot
Copy link
Collaborator

rustbot commented Dec 15, 2024

r? @chenyukang

rustbot has assigned @chenyukang.
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 Dec 15, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 15, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 15, 2024
Comment on lines +194 to +195
if sig.target_feature { hir::Safety::Unsafe } else { hir::Safety::Safe },
&[],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit hacky, I'll add a comment

Comment on lines +252 to +255
let Some(sig) = tcx.hir_node_by_def_id(did).fn_sig() else {
tcx.dcx().span_delayed_bug(attr.span, "target_feature applied to non-fn");
continue;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This hides some follow-up errors when applying the attribute to items that shouldn't have it

// Safe `#[target_feature]` functions are not assignable to safe fn pointers (RFC 2396).
// Safe `#[target_feature]` functions are not assignable to safe fn pointers (RFC 2396),
// report a better error than a safety mismatch.
// FIXME(target_feature): do this inside `coerce_from_safe_fn`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the fixme should say that this should be safe, as it can just be emulated by wrapping in a closure, and in contrast to normal unsafe fns, this can't be unsound as you're already executing code that is satisfying the target feature constraints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hold up, is that true? are there maybe ways around this with TAITs or some other shenanigans that never actually execute the code at runtime, but obtain the closure anyway and then execute that

@@ -1112,7 +1120,11 @@ pub(crate) fn check_unsafety(tcx: TyCtxt<'_>, def: LocalDefId) {

let hir_id = tcx.local_def_id_to_hir_id(def);
let safety_context = tcx.hir().fn_sig_by_hir_id(hir_id).map_or(SafetyContext::Safe, |fn_sig| {
if fn_sig.header.safety.is_unsafe() { SafetyContext::UnsafeFn } else { SafetyContext::Safe }
if matches!(fn_sig.header.safety, Some(hir::Safety::Unsafe)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add a comment explaining why this is the way it is (target_feature is the reason, but explain the details)

compiler/rustc_smir/src/rustc_smir/convert/mod.rs Outdated Show resolved Hide resolved
tests/ui/error-emitter/highlighting.svg Outdated Show resolved Hide resolved
|
LL | let foo: fn() = foo as fn();
| ~~~~~~~~~~~
= note: unsafe functions cannot be coerced into safe function pointers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

shuold probably avoid this note

@@ -26,6 +26,9 @@ LL | Quux.avx_bmi2();
error[E0133]: call to function `avx_bmi2` with `#[target_feature]` is unsafe and requires unsafe function or block
--> $DIR/safe-calls.rs:38:5
|
LL | fn bar() {
| -------- items do not inherit unsafety from separate enclosing items
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol what, look into this

Comment on lines +12 to +13
= note: expected signature `fn(&Bar)`
found signature `unsafe fn(&Bar)`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is still wrong... why are we repeating the same logic in 50 3 places... go fix this

@rust-log-analyzer

This comment has been minimized.

@oli-obk oli-obk force-pushed the safe-target-feature-unsafe-by-default branch from 9bd90ce to 273c6c5 Compare December 15, 2024 21:20
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 16, 2024
…ler-errors

Handle fndef rendering together with signature rendering

Pulled out of rust-lang#134353

Changes some highlighting in type mismatch errors around fndefs
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 16, 2024
…ler-errors

Handle fndef rendering together with signature rendering

Pulled out of rust-lang#134353

Changes some highlighting in type mismatch errors around fndefs
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 16, 2024
…ler-errors

Handle fndef rendering together with signature rendering

Pulled out of rust-lang#134353

Changes some highlighting in type mismatch errors around fndefs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants