Skip to content

Fix missing block for unsafe code #10809

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

Merged
merged 5 commits into from
May 23, 2023
Merged

Fix missing block for unsafe code #10809

merged 5 commits into from
May 23, 2023

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented May 22, 2023

If a block is declared as unsafe, it needs an extra layer of curly braces around it.

Fixes #10808

This code adds handling for UnsafeSource::UserProvided block, i.e. unsafe { ... }. Note that we do not handle the UnsafeSource::CompilerGenerated as it seems to not be possible to generate that with the user code (?), or at least doesn't seem to be needed to be handled explicitly.

There is an issue with this code: it does not add an extra indentation for the unsafe blocks. I think this is a relatively minor concern for such an edge case, and should probably be done by a separate PR (fixing compile bug is more important than getting styling perfect especially when rustfmt will fix it anyway)

// original code
unsafe {
  ...
}

// code that is now generated by this PR
{ unsafe {
  ...
} }

// what we would ideally like to get
{
  unsafe {
    ...
  }
}

changelog: single_match: Fix suggestion for unsafe blocks

@rustbot
Copy link
Collaborator

rustbot commented May 22, 2023

r? @Jarcho

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 22, 2023
These unit tests generate non-compilable code.  I did NOT `bless` them on purpose because the stderr output is not good.

I'm surprised we don't auto-compile the suggestions here - is this something that can be easily enabled?

See rust-lang#10808
@nyurik nyurik changed the title Unit tests highlighting unsafe match issue Fix missing block for unsafe code May 22, 2023
@nyurik nyurik marked this pull request as ready for review May 22, 2023 08:10
@Jarcho
Copy link
Contributor

Jarcho commented May 23, 2023

I wouldn't worry too much about formatting since most people will be running a formatter anyways. As long as it works and isn't atrocious.

P.S. I'm surprised we don't auto-compile the suggestions here - is this something that can be easily enabled? Should this be done as a separate fix test somehow?

Adding //@run-rustfix to the start of the file will run rustfix tests. Is that what you're referring too? This does require that the fixed code builds without errors/warnings.

@nyurik
Copy link
Contributor Author

nyurik commented May 23, 2023

@Jarcho thx, fixed

@Jarcho
Copy link
Contributor

Jarcho commented May 23, 2023

Thank you. @bors r+

@bors
Copy link
Contributor

bors commented May 23, 2023

📌 Commit ed935de has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented May 23, 2023

⌛ Testing commit ed935de with merge 6385676...

bors added a commit that referenced this pull request May 23, 2023
Fix missing block for unsafe code

If a block is declared as unsafe, it needs an extra layer of curly braces around it.

Fixes #10808

This code adds handling for `UnsafeSource::UserProvided` block, i.e. `unsafe { ... }`. Note that we do not handle the `UnsafeSource::CompilerGenerated` as it seems to not be possible to generate that with the user code (?), or at least doesn't seem to be needed to be handled explicitly.

There is an issue with this code: it does not add an extra indentation for the unsafe blocks. I think this is a relatively minor concern for such an edge case, and should probably be done by a separate PR (fixing compile bug is more important than getting styling perfect especially when `rustfmt` will fix it anyway)

```rust
// original code
unsafe {
  ...
}

// code that is now generated by this PR
{ unsafe {
  ...
} }

// what we would ideally like to get
{
  unsafe {
    ...
  }
}
```
@bors
Copy link
Contributor

bors commented May 23, 2023

💔 Test failed - checks-action_test

@Jarcho
Copy link
Contributor

Jarcho commented May 23, 2023

@bors retry

@bors
Copy link
Contributor

bors commented May 23, 2023

⌛ Testing commit ed935de with merge fe792d9...

@bors
Copy link
Contributor

bors commented May 23, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing fe792d9 to master...

@bors bors merged commit fe792d9 into rust-lang:master May 23, 2023
@nyurik nyurik deleted the match-unsafe branch May 23, 2023 19:56
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

single-match makes incorrect suggestion for unsafe code
4 participants