Check unsafe impls on safe EIIs#159783
Conversation
|
Some changes occurred in compiler/rustc_hir/src/attrs cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
rustbot
added
A-attributes
|
r? @oli-obk rustbot has assigned @oli-obk. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Another approach is to emit the Right now, it is a breaking change to declare a unsafe trait but later remove the unsafe requirement. On the other hand, it is not a breaking change to have a unsafe function and then later remove the unsafe requirement. This PR proposes to go the former way, where both adding and removing the unsafe requirement are breaking changes. I'm undecided on what's best (so please don't go off making changes now), but note that this PR is a change to the semantics of the feature, not merely a "fix". |
|
I think it makes sense to follow the example of traits given that EIIs are kinda traits that can have a single global impl. If we decide to make removing unsafe from traits no longer a breaking change, we can adjust EII to follow this. |
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| #[derive(Diagnostic)] | ||
| #[diag("`#[{$name}]` is not unsafe to implement")] |
There was a problem hiding this comment.
As of #159755 we don't mention the #[/] parts
| #[diag("`#[{$name}]` is not unsafe to implement")] | |
| #[diag("`{$name}` is not unsafe to implement")] |
| #[primary_span] | ||
| #[label("`unsafe` is not allowed here")] | ||
| pub span: Span, |
There was a problem hiding this comment.
For consistency with regular attributes I'd prefer if this message pointed at the entire attribute, and then the label only to the redundant unsafe
| #[primary_span] | |
| #[label("`unsafe` is not allowed here")] | |
| pub span: Span, | |
| #[primary_span] | |
| pub impl_span: Span, | |
| #[label("`unsafe` is not allowed here")] | |
| pub unsafe_span: Span, |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
e8c3cb3 to
bae7e21
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Resolved conflicts and updated according to review comments. @rustbot ready |
rustbot
added
S-waiting-on-review
|
@rustbot label +F-extern_item_impls |
| impl_unsafe_span: match *impl_safety { | ||
| Safety::Unsafe(span) => Some(self.lower_span(span)), | ||
| Safety::Safe(_) | Safety::Default => None, | ||
| }, |
There was a problem hiding this comment.
I'm fine with this change and would prefer to address the concept of how we carry around "safety" in the compiler in a more comprehensive manner (i.e., not here and not today).
@JonathanBrouwer are you okay with this change or would you rather we lower+carry Safety here?
There was a problem hiding this comment.
Using rustc_hir::Safety would be nicer as the type is more descriptive, but it does not contain the span so unless we change that (which is too much for this PR) it's not usable. This is fine for now
rust-bors
Bot
added
S-waiting-on-bors
…r=mejrs,JonathanBrouwer Check unsafe impls on safe EIIs Tracking issue: rust-lang#125418 Unsafe EIIs require implementations to use `#[unsafe(...)]`, but safe EIIs currently accept this form as well. This should be rejected, just like `unsafe impl` on a safe trait. This also fixes existing EII UI tests using this pattern and adds a new regression test.
…r=mejrs,JonathanBrouwer Check unsafe impls on safe EIIs Tracking issue: rust-lang#125418 Unsafe EIIs require implementations to use `#[unsafe(...)]`, but safe EIIs currently accept this form as well. This should be rejected, just like `unsafe impl` on a safe trait. This also fixes existing EII UI tests using this pattern and adds a new regression test.
6 participants
Tracking issue: #125418
Unsafe EIIs require implementations to use
#[unsafe(...)], but safe EIIs currently accept this form as well. This should be rejected, just likeunsafe implon a safe trait.This also fixes existing EII UI tests using this pattern and adds a new regression test.