Skip to content

Update check_unsafety.rs Change UnsafetyViolationKind::General to UnsafetyViolationKind::GeneralAndConstFn #75425

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

Closed
wants to merge 54 commits into from

Conversation

CloudNClock
Copy link

@CloudNClock CloudNClock commented Aug 11, 2020

Reflect on #75340

Change UnsafetyViolationKind::General to UnsafetyViolationKind::GeneralAndConstFn

Due to my mistake, PR has been merged from #75578.

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 11, 2020
@RalfJung
Copy link
Member

r? @oli-obk

@oli-obk
Copy link
Contributor

oli-obk commented Aug 12, 2020

Ah, didn't realize there was a PR. So, as mentioned in the issue, you need to bless the tests (and add a new test for the case in the issue). The fastest way to do that is ./x.py test --stage 1 --bless src/test/ui --test-args min_const_fn. That's a mouth full, but all of these flags actually do help:

  • --stage 1 just builds a compiler and runs the tests, without first checking whether you can still build a compiler with that new compiler
  • --bless updates .stderr files, you may still need to touch the source files (e.g. the //~ is unsafe comment in
    const fn bad_const_fn_deref_raw(x: *mut usize) -> &'static usize { unsafe { &*x } } //~ is unsafe
    should now probably be removed)
  • --test-args min_const_fn only runs tests with min_const_fn in their name

@CloudNClock
Copy link
Author

CloudNClock commented Aug 13, 2020

  1. Where should I put the test case from the issue?
    I added the new test case from the issue insrc/test/ui/consts/min_const_fn/min_const_fn_unsafe_ok.rs for now.
  2. Do I import
#![feature(const_raw_ptr_deref)]
#![feature(raw_ref_macros)]
use std::ptr;

in the same file or other places?

Sorry, this is my first time getting into a big open source project. Thank you all for your explanations.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 13, 2020

1. Where should I put the test case from the issue?
   I added the new test case from the issue in`src/test/ui/consts/min_const_fn/min_const_fn_unsafe_ok.rs` for now.

2. Do I import
#![feature(const_raw_ptr_deref)]
#![feature(raw_ref_macros)]
use std::ptr;

in the same file or other places?

While I personally am fine with the changes you made, I don't think the original driver behind min_const_fn_unsafe_ok.rs would be, so let's put it into a separate file in the same directory. Since the file will compile successfully, we'll need a // check-pass comment in that new file.

Sorry, this is my first time getting into a big open source project. Thank you all for your explanations.

No worries! These are all good questions, please never stop asking.

@CloudNClock
Copy link
Author

What should I do now with all the commits? I try to squash the commits and then unexpected merge seems to be pushed. And previous commits are still shown. Did I do it in the wrong way? I think I should revert the merge?
If I commit, will the assignees receive a notification? Generally speaking, should I leave a comment after a stage of commits?
Thanks.

@RalfJung
Copy link
Member

You already have a merge commit in here, so please rebase on top of latest origin/master while also squashing:

git fetch origin
git rebase --interactive origin/master

That should not create any merge commits. See for example this explanation of rebasing.

The reviewer will be notified when you do a regular push, but force-pushes unfortunately do not reliably trigger notifications (GitHub is acting rather strange there), so generally it is a good idea to ask the reviewer to have another look when the PR is ready again.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 14, 2020

Jup, so the PR lgtm. While doing the rebase, please squash all commits into the first one

…R_CASTS handling after the match

The reason I did this in the first place was to try and figure out why I don't see my expected 7 error messages
This seemed to overdo it a bit, affecting multiple submodules, and changing a file I didn't touch, so I didn't commit those changes
RalfJung and others added 27 commits August 15, 2020 21:23
Fixes #75361
Fixes #74918

Previously, we were creating a new `InferCtxt`, which caused an ICE when
used with type variables from the existing `InferCtxt`
Co-authored-by: Yuki Okushi <[email protected]>
Co-authored-by: Lonami <[email protected]>
Renamed remaining references to "undef" to "uninit" when referring to Miri.

Impacted directories are:

- src/librustc_codegen_llvm/consts.rs
- src/librustc_middle/mir/interpret/
- src/librustc_middle/ty/print/pretty.rs
- src/librustc_mir/
- src/tools/clippy/clippy_lints/src/consts.rs

Upon building Miri based on the new changes it was verified that no changes needed to be made with the Miri project.

Related issue #71193
This commit constrains the support added for handling unevaluated consts
in polymorphization (introduced in #75260) by:

- Skipping associated constants as this causes cycle errors.
- Skipping promoted constants when they contain `Self` as this ensures
  `T` is used in constants of the form `<Self as Foo<T>>`.

Signed-off-by: David Wood <[email protected]>
The old implementation only looks at numbers at the end, but not in
other places in a name: "u8" and "u16" got sorted properly, but "u8_bla"
and "u16_bla" did not.
When we have a tuple struct used with struct we don't want to suggest using
the (valid) struct syntax with numeric field names. Instead we want to
suggest the expected syntax.

Given

```rust
fn main() {
    match MyOption::MySome(42) {
        MyOption::MySome { x: 42 } => (),
        _ => (),
    }
}
```

We now emit E0769 "tuple variant `MyOption::MySome` written as struct variant"
instead of E0026 "variant `MyOption::MySome` does not have a field named `x`".
Remove unnecessary `unwrap`.
Even though the description is clear but the solution may not be as straightforward.
Adding a suggested fix.
author Matthew Y <[email protected]> 1597179585 -0400
committer 5M1Sec <[email protected]> 1597541078 -0400

Allowing raw ptr dereference in const fn

Change ``UnsafetyViolationKind::General` to `UnsafetyViolationKind::GeneralAndConstFn` in check_unsafety.rs

Bless min_const_fn

Add test case from issue 75340 into a new file
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 18, 2020
Allowing raw ptr dereference in const fn

Reflect on issue rust-lang#75340
Discussion in previous PR  rust-lang#75425

## Updates
Change `UnsafetyViolationKind::General` to `UnsafetyViolationKind::GeneralAndConstFn` in check_unsafety.rs

Remove `unsafe` in min_const_fn_unsafe_bad.rs

Bless min_const_fn

Add the test case from issue 75340
***
Sorry for the chaos. I messed up and ended up deleting the repo in the last PR. I have to create a new PR for the new repo. I will make a feature branch next time. I will edit the old PR once I receive the commends.

@RalfJung Thank you all for your replies. They are helpful!

r? @oli-obk
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.