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

Add fn_to_numeric_cast_usize lint #5914

Closed
wants to merge 2 commits into from

Conversation

Coder-256
Copy link

@Coder-256 Coder-256 commented Aug 17, 2020

changelog: Add fn_to_numeric_cast_usize lint

Fixes #5906.

In order to implement this, I had to make the design choice that casts from a function to usize should first go through a function pointer for clarity. For example:

fn fun() -> usize { 1 }
let a: usize = fun as fn() -> usize as usize;

Casts from a function pointer to a usize are still OK. Also, if you simply disable this lint, the old behavior should apply (except for error/help messages, as described below).

I also changed the error/help messages for "function as numeric" in order to more clearly explain that the function is being cast as a pointer, and suggest calling it then casting it, rather than casting a pointer to it as a usize (if I write foo as i32, I probably mean foo() as i32, not foo as usize).

Furthermore, I had to special-case the transmutes_expressible_as_ptr_casts lint in order to account for this change (recommending casting rather than calling due to the nature of transmute), and I am not 100% sure that I did it correctly. I based it on this method. This part might also need more tests.

Finally, this is my first contribution to Clippy, so please let me know if I missed anything.


EDIT: I've thought about this some more, and I think it would actually make more sense to remove the fn_to_numeric_cast_with_truncation lint entirely. I don't think the question of whether the cast would truncate is really relevant to this kind of lint; any cast to something other than usize is equally questionable IMO. Again, I think it's far more likely that the correct solution is to call the function and cast the result, rather than what the old help suggests: casting to a usize instead of whatever numeric were trying to cast to. I don't think I have ever needed to use a function pointer except for calling a C function.

Taking that into consideration, another solution could look something like:

Original Lint Help
func as i8 fn_to_numeric_cast func() as i8, [func as fn() -> () as usize]
func as i128 fn_to_numeric_cast func() as i128, [func as fn() -> () as usize]
func as usize fn_to_numeric_cast_usize func() as usize, func as fn() -> () as usize
fn_ptr as i8 fn_to_numeric_cast fn_ptr() as i8, [fn_ptr as usize]
fn_ptr as i128 fn_to_numeric_cast fn_ptr() as i128, [fn_ptr as usize]

The bracketed help messages are messages that I think would do more harm than good by being overly confusing (especially to new users) but should still be considered.

Casts that are allowed:

  • fn_ptr as usize
  • fn_ptr as usize as i8
  • fn_ptr as usize as i128
  • func as fn() -> ()
  • func as fn() -> () as usize
  • func as fn() -> () as usize as i8
  • func as fn() -> () as usize as i128

I think I prefer this design better. You need to explicitly convert from a function to a function pointer, from a function pointer to a usize, and from a usize to another numeric. It's more verbose, but I think this way it is very hard to do the wrong thing by mistake. Plus, this way, the longer the cast, the less likely it is that you are doing the right thing! I'm not quite sure who makes these decisions though.

@rust-highfive
Copy link
Collaborator

rust-highfive commented Aug 17, 2020

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (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 17, 2020
@Coder-256
Copy link
Author

Coder-256 commented Aug 17, 2020

For the function item errors, I was also going to add something like "help: did you mean to cast to a function pointer?" but I'm not sure I know how to do that correctly. Should I just replace span_lint_and_sugg with span_lint_and_then, then cal diag.span_suggestion and diag.help?

@Coder-256
Copy link
Author

Coder-256 commented Aug 18, 2020

Hmm, I had only guessed the 32-bit test output, and I just realized that I think I guessed wrong, but apparently CI doesn't test it(??), and the test failure is actually spurious(???).

Also, I implemented the alternate model in my fn-cast-usize-alternate branch, and I think the error messages are even better than before.

Copy link
Member

@flip1995 flip1995 left a comment

to remove the fn_to_numeric_cast_with_truncation lint entirely

I disagree. I'm not sure why fn_to_numeric_cast_with_truncation and fn_to_numeric_cast are separate lints though.

Taking that into consideration, another solution could look something like:

The thing is, that Clippy suggestions should always keep the semantics of the original program, except we can be absolutely sure, that the written code is utterly wrong. We can't assume this in this case. What we can do is to suggest both, but the semantic preserving suggestion should always come first.

Should I just replace span_lint_and_sugg with span_lint_and_then, then cal diag.span_suggestion and diag.help?

Yes this is how that is done.

Hmm, I had only guessed the 32-bit test output, and I just realized that I think I guessed wrong, but apparently CI doesn't test it(??), and the test failure is actually spurious(???).

It will be tested by @bors before the PR is merged. We have an i686 (?) runner.

@@ -630,6 +630,25 @@ impl<'tcx> LateLintPass<'tcx> for Transmute {
);
}
},
(ty::FnDef(..), ty::Int(_) | ty::Uint(_)) => span_lint_and_then(
Copy link
Member

@flip1995 flip1995 Aug 18, 2020

Choose a reason for hiding this comment

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

This is unrelated?

I wouldn't do this, since the extra cast to fn() -> T is code bloat. If people prefer these explicit casts, they should enable the new lint (which should be pedantic).

/// let b: usize = fun2();
/// ```
pub FN_TO_NUMERIC_CAST_USIZE,
style,
Copy link
Member

@flip1995 flip1995 Aug 18, 2020

Choose a reason for hiding this comment

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

This should be pedantic or even restriction IMO, since this cast is usually fine.


let to_nbits = int_ty_to_nbits(cast_to, cx.tcx);

if is_def {
Copy link
Member

@flip1995 flip1995 Aug 18, 2020

Choose a reason for hiding this comment

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

This repeats too much code.

match cast_from.kind {
    ty::FnDef(..) if cast_to.kind == ty::Uint(UintTy::Usize) => { /* lint FN_TO_NUMERIC_CAST_USIZE */ },
    ty::FnDef(..) | ty::FnPtr(_) => { /* keep old code */ },
    .. => {},
}

// Casting to fn pointer is OK and should not warn
let _ = foo as fn() -> String;
Copy link
Member

@flip1995 flip1995 Aug 18, 2020

Choose a reason for hiding this comment

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

As an additional test:

    // Casting to fn pointer and then to a `usize` is OK and should not warn
    let _ = foo as fn() -> String as usize;

&format!("casting *pointer* to function `{}` as `{}`", from_snippet, cast_to),
"try",
format!("{}() as {}", from_snippet, cast_to),
Copy link
Member

@flip1995 flip1995 Aug 18, 2020

Choose a reason for hiding this comment

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

I don't think we should assume here, that a cast of the return value was meant. Also I think the casting function pointer ... message was clear enough that it talks about a function pointer. Also emphasizing with * or something else isn't used in error messages.

What we can do is add two suggestions, one that suggests to cast the pointer to usize and one to call the function and cast the resulting value to usize, iff it is a FnDef(..)

@flip1995
Copy link
Member

flip1995 commented Aug 18, 2020

r? @flip1995

@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Aug 18, 2020
@Coder-256
Copy link
Author

Coder-256 commented Aug 22, 2020

This is waiting on discussion in #5921.

@flip1995 flip1995 added S-needs-discussion Status: Needs further discussion before merging or work can be started and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Aug 24, 2020
@bors
Copy link
Contributor

bors commented Sep 4, 2020

The latest upstream changes (presumably #6007) made this pull request unmergeable. Please resolve the merge conflicts.

@kraktus
Copy link
Contributor

kraktus commented Sep 24, 2022

I think this can be closed as implemented by #7705

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check casting function item to usize
6 participants