Clippy pointer cast lint experiment #75098

Ryan1729 · 2020-08-03T09:43:24Z

This PR is an experiment about exposing more parts of rustc_typeck for use in clippy. In particular, the code that checks where a cast is valid or not was exposed, which necessitated exposing FnCtxt, and figuring out how to create an instance of that type inside clippy.

This was prompted by this clippy issue.

r? @oli-obk

rust-highfive · 2020-08-03T09:43:27Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @oli-obk (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.

src/tools/clippy/clippy_lints/src/transmute.rs

oli-obk · 2020-08-03T10:02:35Z

src/tools/clippy/clippy_lints/src/transmute.rs

+                .ok()
+                // do_check's documentation says that it might return Ok and create
+                // errors in the fcx instead of returing Err in some cases.
+                .filter(|_| !fn_ctxt.errors_reported_since_creation())


This seems bad... won't we start emitting errors for weird transmutes?

My intention was that if errors happened, we would return None. As far as I understand, that filter achieves that since I negated the boolean. So if error happen in the FnCtxt, this code should be treating it the same as an Err result, unless I've missed something.

I assumed that if any errors happened in the FnCtxt, some other check, probably from rustc itself, would catch the problem and report it. Does that seem like a reasonable assumption, and does that fit with your expectations?

My worry is that the errors have already been emitted. I haven't checked how errors could be emitted, so... I guess look for these errors and try to use the arguments on which they'd error for a cast on a transmute and see how the tests fare.

Of all the functions called from inside do_check, check_ref_cast is the one that most clearly has a case where errors are emitted. This is because it calls demand_eqtype which clearly can emit an error.

The following functions clearly do not emit errors:

CheckCast::check_ptr_ptr_cast

CheckCast::check_ptr_addr_cast

CheckCast::check_addr_ptr_cast

CheckCast::check_fptr_ptr_cast

I haven't confirmed whether or not the other functions called in do_check emit errors though.

I also had a look at the commit the introduced do_check and the doc comment on it I mentioned above, and check_ref_cast was already called within do_check at that time, so I suspect that is what the doc comment originally referred to.

So, my next step is to add code to the tests that triggers demand_eqtype to emit an error, and see what happens.

I tried adding a dbg! call into demand_eqtype so that if the error case was called, the tests would definitely fail, (due to the extra stdout output.) That way, we would know that the new tests would actually hit that case, so we would know whether or not we got an extra error message.

The existing tests for the new lint did not hit that case, but from looking at the code leading from do_check to demand_eqtype, it seems like a transmute from an array reference to a pointer to a different type should trigger the lint to call do_check in a way that should trigger it. For example, &[i32; 1] to *const u8. So I added such a transmute to the tests, but I got an error from a different transmute lint.

While the other lint seems to prevent any issues at the moment, if that lint, or the ordering was changed in the future that could lead to a surprising bug, so it seems prudent to understand whether the do_check call will cause an extra error or not.

So, I temporarily placed the new lint at the top of the big match inside the check_expr implementation inside transmute.rs. Doing that lead to an ICE with the following error message:

thread 'rustc' panicked at 'assertion failed: self.find(parent).map_or(false, |n| is_body_owner(n, hir_id))', src/librustc_middle/hir/map/mod.rs:352:9

So without digging into what the failed assert means exactly, I would guess that something is wrong with the way that the FnCtxt was created inside clippy in this PR. But I'm not really sure. Does anyone else have more context around what that assert failing suggests?

(In case anyone cares, here's a link to the commit that had the ICE, on a different branch)

but I got an error from a different transmute lint.

If that other lint is allowed, would the error occur? Or is it part of this big match, so we never reach your new lint if the other lint is emitted? I'd be fine to just leave sufficient comments (and add a test that will fail horribly if the order is changed).

is it part of this big match, so we never reach your new lint if the other lint is emitted?

That is the case.

Some other tests were already failing whenever I observed the ICE, but better safe than sorry. I've added a the transmute from &[i32; 1] to *const u8 case to the lint-specific test.

Ok great! I think you can now just make this an assert instead of filtering

Sounds good. I added a not about the panics to the comments too. All that is in this commit

oli-obk · 2020-08-06T07:38:46Z

@rust-lang/clippy we're doing a clippy PR against rustc, because it also requires rustc changes. Now that the rustc changes work, we could also split it and move the clippy changes to a clippy PR, or we review it here and then sync afterwards. Opinions?

@Ryan1729 please rebase and ensure that none of your commits contain submodule changes

…into transmute module

There are 5 errors, when there should be 7.

…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

…that we still get an ICE

…re covering the same ground

…ainst lint ordering changing

…ible_as_ptr_casts test

oli-obk · 2020-08-06T10:46:08Z

src/tools/clippy/tests/ui/transmutes_expressible_as_ptr_casts.rs

+// fall through into `do_check`.
+fn trigger_do_check_to_emit_error(in_param: &[i32; 1]) -> *const u8 {
+    unsafe { transmute::<&[i32; 1], *const u8>(in_param) }
+}


missing a newline at the end of the file

oli-obk · 2020-08-06T11:16:44Z

You need to run ./x.py fmt before CI even starts building the PR

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

flip1995 · 2020-08-06T13:09:31Z

I can review the Clippy changes in this PR and we can sync afterwards. I don't think it is worth to go back and forth with syncs for changes that affect rustc and clippy (to avoid this we switched to subtree in the first place)

Ryan1729 · 2020-08-08T02:59:08Z

Here’s a snippet from the bottom of the raw log linked above that suggests the failure was caused by a network issue.

2020-08-07T17:55:39.4818196Z Caused by:
2020-08-07T17:55:39.4818413Z   failed to get 200 response from `https://crates.io/api/v1/crates/maplit/1.0.1/download`, got 502

oli-obk · 2020-08-08T08:48:40Z

@bors retry

bors · 2020-08-08T15:19:41Z

⌛ Testing commit c04c4cb with merge d72af5f4695cbd0c3d565eb53c4cdb50a5370e6b...

bors · 2020-08-08T15:57:08Z

💔 Test failed - checks-actions

oli-obk · 2020-08-08T23:29:26Z

clippy's stderr files need updating again, you may need to rebase first

…d not suggest anything

Ryan1729 · 2020-08-09T06:51:26Z

After several small changes, this is passing again, locally.

Two changes are most notable:
The fixed code wasn't compiling because I was getting a warning about use std::mem::transmute being unused after the fixes removed all calls to it. I noticed then that a situation where no suggestion should happen was not part of the tests. So I added a new test that containing a transmute call that should not and does not get removed by the fixer.

The other notable change was that I changed the use of -1isize in the new tests to usize::MAX since otherwise the fixed version resulted in -1 as *const i32 which does not compile since the 1 ia inferred to be a usize. I couldn't find an easy way to change the suggestion to include the type suffix, so since actually wanting to use a unary minus to write a number to be cast as a pointer seems rather niche, (I only used -1 here so if something went wrong it would be obvious), I just changed the type to be a usize and kept the same value.

oli-obk · 2020-08-10T08:09:08Z

@bors r+

bors · 2020-08-10T08:09:09Z

📌 Commit d2e7293 has been approved by oli-obk

@ghost

Rollup of 10 pull requests Successful merges: - rust-lang#75098 (Clippy pointer cast lint experiment) - rust-lang#75249 (Only add a border for the rust logo) - rust-lang#75315 (Avoid deleting temporary files on error) - rust-lang#75316 (Don't try to use wasm intrinsics on vectors) - rust-lang#75337 (instance: only polymorphize upvar substs) - rust-lang#75339 (evaluate required_consts when pushing stack frame in Miri engine) - rust-lang#75363 (Use existing `infcx` when emitting trait impl diagnostic) - rust-lang#75366 (Add help button) - rust-lang#75369 (Move to intra-doc links in /library/core/src/borrow.rs) - rust-lang#75379 (Use intra-doc links in /library/core/src/cmp.rs) Failed merges: r? @ghost

@ghost

Rustup r? @ghost Sync back rust-lang/rust#75098 changelog: none

@oli-obk

…xperiment, r=oli-obk Clippy pointer cast lint experiment This PR is an experiment about exposing more parts of `rustc_typeck` for use in `clippy`. In particular, the code that checks where a cast is valid or not was exposed, which necessitated exposing [`FnCtxt`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_typeck/check/struct.FnCtxt.html), and figuring out how to create an instance of that type inside `clippy`. This was prompted by [this clippy issue](rust-lang/rust-clippy#2064). r? @oli-obk

@Manishearth

Update Clippy Biweekly Clippy update (2 days late, since I wanted to wait for rust-lang#75098) r? @Manishearth

@Manishearth

Update Clippy Biweekly Clippy update (2 days late, since I wanted to wait for rust-lang/rust#75098) r? @Manishearth

rust-highfive assigned oli-obk Aug 3, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2020

Ryan1729 mentioned this pull request Aug 3, 2020

catch transmuting references which could be pointer casts rust-lang/rust-clippy#2064

Closed

oli-obk reviewed Aug 3, 2020

View reviewed changes

src/tools/clippy/clippy_lints/src/transmute.rs Outdated Show resolved Hide resolved

oli-obk reviewed Aug 3, 2020

View reviewed changes

src/tools/clippy/clippy_lints/src/transmute.rs Outdated Show resolved Hide resolved

oli-obk reviewed Aug 3, 2020

View reviewed changes

src/tools/clippy/clippy_lints/src/transmute.rs Show resolved Hide resolved

oli-obk reviewed Aug 3, 2020

View reviewed changes

Ryan1729 added 13 commits August 6, 2020 04:24

make parts of rustc_typeck public

ef29161

run cargo dev new_lint then move transmutes_expressible_as_ptr_casts …

5565c1a

…into transmute module

make InheritedBuilder::enter public

0c37239

initial compiling version of TRANSMUTES_EXPRESSIBLE_AS_PTR_CASTS

b4e6e70

write currently failing test for transmutes_expressible_as_ptr_casts

7061b1c

There are 5 errors, when there should be 7.

accidentally cause an ICE by putting the TRANSMUTES_EXPRESSIBLE_AS_PT…

b4ecee9

…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

try putting the can_be_expressed_as_pointer_cast at the top and find …

1e5c14d

…that we still get an ICE

get the expected number of errors by acknowledging that other lints a…

7a818c0

…re covering the same ground

address some review comments

ab93133

add description to assert

6fdd1db

add documentation to functions that call do_check and add a test ag…

8ba4101

…ainst lint ordering changing

add extra error message to the expected stderr for transmutes_express…

afd4909

…ible_as_ptr_casts test

change filter to assert, and update comments

5447247

Ryan1729 force-pushed the clippy-pointer-cast-lint-experiment branch from 421434a to 5447247 Compare August 6, 2020 10:24

oli-obk reviewed Aug 6, 2020

View reviewed changes

add newline to transmutes_expressible_as_ptr_casts.rs

e3170bb

Ryan1729 added 3 commits August 6, 2020 06:08

run ./x.py fmt

4027f15

run clippy_dev update_lints

007dc94

run clippy_dev fmt

32691da

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

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 8, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 8, 2020

Ryan1729 added 4 commits August 8, 2020 21:03

update stderr for transmutes_expressible_as_ptr_casts

b02bf05

add a test example of where transmutes_expressible_as_ptr_casts shoul…

0fbf450

…d not suggest anything

fix unary minus on usize and unused variable errors in .fixed file

58b8b11

add allow unused_unsafe and allow dead_code

d2e7293

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 10, 2020

Dylan-DPC-zz mentioned this pull request Aug 10, 2020

Rollup of 10 pull requests #75383

Merged

bors merged commit 63d1e37 into rust-lang:master Aug 11, 2020

flip1995 mentioned this pull request Aug 11, 2020

Rustup rust-lang/rust-clippy#5891

Merged

bors added a commit to rust-lang/rust-clippy that referenced this pull request Aug 11, 2020

Auto merge of #5891 - flip1995:rustup, r=flip1995

09bd400

Rustup r? @ghost Sync back rust-lang/rust#75098 changelog: none

flip1995 mentioned this pull request Aug 11, 2020

Update Clippy #75405

Merged

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 12, 2020

Auto merge of rust-lang#75405 - flip1995:clippyup, r=Manishearth

8da42dd

Update Clippy Biweekly Clippy update (2 days late, since I wanted to wait for rust-lang#75098) r? @Manishearth

flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Aug 12, 2020

Auto merge of #75405 - flip1995:clippyup, r=Manishearth

fdc2f87

Update Clippy Biweekly Clippy update (2 days late, since I wanted to wait for rust-lang/rust#75098) r? @Manishearth

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clippy pointer cast lint experiment #75098

Clippy pointer cast lint experiment #75098

Ryan1729 commented Aug 3, 2020 •

edited

Loading

rust-highfive commented Aug 3, 2020

oli-obk Aug 3, 2020

Ryan1729 Aug 4, 2020

oli-obk Aug 4, 2020

Ryan1729 Aug 4, 2020

Ryan1729 Aug 5, 2020 •

edited

Loading

oli-obk Aug 5, 2020

Ryan1729 Aug 6, 2020 •

edited

Loading

oli-obk Aug 6, 2020

Ryan1729 Aug 6, 2020

oli-obk commented Aug 6, 2020

oli-obk Aug 6, 2020

oli-obk commented Aug 6, 2020

flip1995 commented Aug 6, 2020

Ryan1729 commented Aug 8, 2020

oli-obk commented Aug 8, 2020

bors commented Aug 8, 2020

bors commented Aug 8, 2020

oli-obk commented Aug 8, 2020

Ryan1729 commented Aug 9, 2020 •

edited

Loading

oli-obk commented Aug 10, 2020

bors commented Aug 10, 2020

Clippy pointer cast lint experiment #75098

Clippy pointer cast lint experiment #75098

Conversation

Ryan1729 commented Aug 3, 2020 • edited Loading

rust-highfive commented Aug 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ryan1729 Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ryan1729 Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Aug 6, 2020

Choose a reason for hiding this comment

oli-obk commented Aug 6, 2020

flip1995 commented Aug 6, 2020

Ryan1729 commented Aug 8, 2020

oli-obk commented Aug 8, 2020

bors commented Aug 8, 2020

bors commented Aug 8, 2020

oli-obk commented Aug 8, 2020

Ryan1729 commented Aug 9, 2020 • edited Loading

oli-obk commented Aug 10, 2020

bors commented Aug 10, 2020

Ryan1729 commented Aug 3, 2020 •

edited

Loading

Ryan1729 Aug 5, 2020 •

edited

Loading

Ryan1729 Aug 6, 2020 •

edited

Loading

Ryan1729 commented Aug 9, 2020 •

edited

Loading