-
Notifications
You must be signed in to change notification settings - Fork 14.1k
forbid #[target_feature] in traits #141465
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
Conversation
|
rustbot has assigned @compiler-errors. Use |
|
Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
|
@bors try |
forbid #[target_feature] in traits `target_feature` in trait *implementations* makes basically no sense -- it's almost always unsound since a generic caller has no way of knowing the required target features! In particular, now that we can have *safe* target feature functions, that's *definitely* unsound in combination with `dyn`, see #139368. As sound version of this would have to check that the same target features are already set in the trait declaration. But only functions with bodies can even have target features in the trait declaration! So, this was never actually designed in a way that makes any sense, it just kinda did something. Interestingly there were tests for `#[target_feature]` in traits and trait impls... not sure how that didn't set off alarm bells. Oh well. I propose we just entirely forbid target features in trait impls and trait declarations. This will need an FCW, but before we do that let's get an idea for how big the fallout is by doing a crater run. The diagnostics that result from PR are terrible but it's good enough for crater.
This comment has been minimized.
This comment has been minimized.
3b43b6a to
7523e1d
Compare
|
Hm, so apparently aho-corasick actually has target_feature in trait impls or so? Damn.^^ |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
|
|
@hanna-kruppe I think that's just an unfortunate case of a macro expansion span and some lines being omitted. I was confused the same way. Anyway this PR isn't going to happen, we won't break aho-corasick.^^ See here for further discussion. |
target_featurein trait implementations makes basically no sense -- it's almost always unsound since a generic caller has no way of knowing the required target features! Strangely, we do have tests checking for this. I don't understand what the intended reasoning principles for soundness here would look like. At least someone thought of blocking this for safe functions / target-feature 1.1 so there's always some unsafe code somewhere before this causes UB.As sound version of this would have to check that the same target features are already set in the trait declaration. But only functions with bodies can even have target features in the trait declaration!
I propose we just entirely forbid target features in trait impls and trait declarations. This will need an FCW, but before we do that let's get an idea for how big the fallout is by doing a crater run. The diagnostics that result from PR are terrible but it's good enough for crater.