Skip to content
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

[WIP] Add lint for int to ptr cast #43339

Closed
wants to merge 3 commits into from

Conversation

mattico
Copy link
Contributor

@mattico mattico commented Jul 19, 2017

Fixes #42915

First commit adds the lint, the second commit fixes the lint warnings in the compiler on Windows. More sure to follow for other platforms.

[WIP] because we should wait for the decision about #43291 to determine what message we use. Specifically, if it's decided that those strange casts can't/won't be fixed, the message should warn that these casts don't sign-extend as expected unless you go through {u,i}size.

Edit: Also we should consider the interaction this lint can/should have with the portability lint RFC (unimplemented right now), and if it requires an RFC of its own like rust-lang/rfcs#1782. I hadn't considered that adding a lint would require an RFC.

If #43291 does get fixed, we may want this lint to only apply to {i,u}8 as those cases are much more likely to be bugs than false positives compared to the other integer types.

I should also probably add a NOTE: try {expr} as usize as *const

@rust-highfive
Copy link
Collaborator

r? @arielb1

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

fn check_expr(&mut self, ctx: &LateContext, expr: &hir::Expr) {
if let hir::ExprCast(ref source, _) = expr.node {
match ctx.tables.cast_kinds.get(&source.id) {
Some(&CastKind::AddrPtrCast) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if let Some(&CastKind::AddrPtrCast) = ctx.tables.cast_kinds.get(&source.id) {
?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

certainly

@aidanhs
Copy link
Member

aidanhs commented Jul 20, 2017

Thanks for the PR @mattico! We'll check in now and again to make sure @arielb1 or another reviewer gets to this soon.

@aidanhs aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2017
@carols10cents
Copy link
Member

friendly ping @arielb1!

@arielb1
Copy link
Contributor

arielb1 commented Jul 24, 2017

This LGTM. Let's wait for the lang team meeting to see what we want for the message to be.

ctx.span_lint(
INT_TO_PTR_CAST,
expr.span,
"casting from a different-size integer to a pointer");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message is a placeholder, but I think "casting from a potentially non-pointer-sized integer to a pointer" would be better English.

@ollie27
Copy link
Member

ollie27 commented Jul 24, 2017

While you're at it, how about linting the other direction as well (rust-lang/rfcs#1782)?

@mattico
Copy link
Contributor Author

mattico commented Jul 24, 2017

@ollie27 I hadn't seen that RFC before, thanks for the link.

It seems like these lints could interact with the portability lint (rust-lang/rfcs#1868) to have a different warning when the integer size matches the pointer. e.g:

#[cfg(target_pointer_width = "32")]
fn bar(foo: *const u8) {
    let x = foo as u32; // No warning
}

fn baz(foo: *const u8) {
    let x = foo as u32; // warning: pointer cast is not portable ...
}

This is becoming more complicated than I expected...

@retep998
Copy link
Member

I'd personally rather have a more broad lint now that doesn't take into account cfg portability stuff, and later it can be amended to take that stuff into account.

@alexcrichton alexcrichton added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2017
@alexcrichton
Copy link
Member

According to #43339 (comment) I've tagged as T-lang and waiting on team

@mattico
Copy link
Contributor Author

mattico commented Jul 28, 2017

Note to self: rebase on #43522

@mattico
Copy link
Contributor Author

mattico commented Aug 3, 2017

Lang team decided (#43291 (comment)) to lint first, maybe fix later. Will update after #43522.

@shepmaster shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Aug 4, 2017
@aidanhs aidanhs added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 10, 2017
@RalfJung
Copy link
Member

This looks like it's doing something very similar to #43641? (as noted in #43641 (review))

@retep998
Copy link
Member

#43641 is specifically for casts from signed integer types smaller than isize to raw pointers, which is a future compatibility hazard if we ever change that cast to perform sign extension instead of zero extension. This PR meanwhile is for a more general lint on all casts from any integer types other than isize/usize to raw pointers.

@aidanhs
Copy link
Member

aidanhs commented Aug 16, 2017

#43522 has landed, looks like this didn't need a rebase. Just for clarity, @mattico can you explain where this PR is up to/what it's waiting on?

@mattico
Copy link
Contributor Author

mattico commented Aug 17, 2017

There's some overlap between this and #43641. I think I'm just going to let that PR take over. If we change our minds about what cases we want to cover, we can edit that lint later on.

@mattico mattico closed this Aug 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add lint for u8 as *mut cast