WIP: Add lint on cast Fn to all numerical except usize.

[VKlayd]

This is should fix #2588. But I'm novice here and not sure is it OK.

First of all - should I add some new lint type or can use UNNECESSARY_CAST for example? Is this lint message correct?

Also I have been trying to add more specific message for casting Enum constructor to int but I can't find a way to check is 'cast_from' function returns Enum variant. May be it's unnecessary?

Thanks for review.

[flip1995]

Thanks for the contribution!
The doc of the lint UNNECESSARY_CAST says that it "Checks for casts to the same type.". This doesn't really apply to the fn -> numeric cast. I would suggest you make a new lint (something like FN_TO_NUMERIC_CAST maybe? Feel free to choose a better name!). I think this lint should be in the correctness lint group.

Is this lint message correct?

Since the better cast would be to usize you could modify the message a bit and add a help message. You can use the span_help_and_lint function for that:
For the lint message you can use format! to emit something like "casting a Fn to {TYPE} may truncate the function address value." and the help message could be "consider casting to `usize` instead."

Bonus points if you manage to use the span_lint_and_sugg function where the sugg would be the correct code to replace the cast. But this is not necessary.

Also I have been trying to add more specific message for casting Enum constructor to int but I can't find a way to check is 'cast_from' function returns Enum variant.

Imo this is unnecessary.

[VKlayd]

Thank you for review.

So, I have changed lint type to unique one, fix message and add suggestion.
Maybe

casting a fn(usize) -> Foo {Foo::A} to i32 may truncate...

is better way than

casting a Fn to i32 may truncate...

Real type is more explicit. Or in such place it's excessive?

[flip1995]

Maybe
casting a fn(usize) -> Foo {Foo::A} to i32 may truncate...

good catch, that would indeed be better. You can set the function type and the cast type in ticks: `{}`, which makes the error message a little bit nicer imo.

[flip1995]

This looks good! I think for the suggestion message "if you need the address of the function, consider:" would be enough, since the usize hint is already in the suggestion.

[flip1995]

What it does: "Checks for casts of a function pointer to a numeric type except `usize`."
Why is this bad? Casting a function pointer to something other than `usize` could truncate the address value.

This would be a little more precise, what this lint does.

[flip1995]

Oh and could you add another test for a function which is not called x, just to make sure, that the suggestion is right?

[VKlayd]

I've done it.

But I'm confused a bit. Why format of help strongly depends on number of words in 'help' message?
i.e

   |
16 |     let z = bar as u32;
   |             ^^^^^^^^^^
   |
   = note: #[deny(fn_to_numeric_cast)] on by default
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.205/index.html#fn_to_numeric_cast
help: if you need the address of the function, consider :
   |
16 |     let z = bar as usize;
   |             ^^^^^^^^^^^^

   |
16 |     let z = bar as u32;
   |             ^^^^^^^^^^ help: if you need the address of the function, consider:: `bar as usize`
   |

it differs only by one whitespace before ':' and dramatically change whole view.

I think that first one is better because with such big 'help' message second one looks not so good. Is there a way to set format explicitly? Or which way is better here?

[flip1995]

such big 'help' message second one looks not so good.

That is exactly why it depends on the number of signs/words in the help message.

Is there a way to set format explicitly?

Not really. What you could do is change the help message once more: just "consider". Without a :, this will be added by the short inline help message automatically.

Sorry for the back and forth with the help/lint messages.

[VKlayd]

No problem! This help-message auto-formatting logic looks a bit weird at first time. Hope now it's OK.

[flip1995]

LGTM. Thanks for all the work!

@oli-obk

[oli-obk]

Just a test-nit, lgtm otherwise

[oli-obk]

Can you also add a check for Foo::A as i32 directly without an intermediate variable?

[VKlayd]

Hmm. I remembered about 128 bit types.

error: casting a `fn(i32) -> Foo {Foo::A}` to `u128` may truncate the function address value.

it looks not correct enough. Should I fix error message or add constraints for this lint (allow cast to 'usize' or integer type with bigger size)?

[oli-obk]

Hmm. I personally want all casts to go through a usize cast, so maybe just improve the error message for u64 and u128?

[VKlayd]

I have thought about this. I think that conversion without truncation and with one is not the same, so I decide to divide this lint to two ones. One of them is about 'correctness' and other is about 'style'.

What do you think on it?

[VKlayd]

Excuse me, @oli-obk, please give me feedback.

[oli-obk]

Sorry this got lost in my emails. Yes I agree with your conclusion to make two lints out of it and with the categories you placed it into.