Disable cast_lossless when casting to u128 from any (u)int type

[Jacherr]

Fixes #12492

Disables cast_lossless when casting to u128 from any int or uint type. The lint states that when casting to any int type, there can potentially be lossy behaviour if the source type ever exceeds the size of the destination type in the future, which is impossible with a destination of u128.

It's possible this is a bit of a niche edge case which is better addressed by just disabling the lint in code, but I personally couldn't think of any good reason to still lint in this specific case - maybe except if the source is a bool, for readability reasons :).

changelog: FP: cast_lossless: disable lint when casting to u128 from any (u)int type

[rustbot]

r? @blyxyas

rustbot has assigned @blyxyas.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Alexendoo]

I think the lint still applies here, the message could be updated but replacing a lossless cast with from is still a good suggestion

The type of the RHS could be changed in the future for example to a lossy cast

[blyxyas]

Meow meow meow =^w^=

[blyxyas]

Casting from a u128 to a i128 could be dangerous. u128 is the only data type that we can be sure that they won't cause an error.

Suggested change

	&& (matches!(cast_to.kind(), ty::Int(IntTy::I128)) || matches!(cast_to.kind(), ty::Uint(UintTy::U128))))
	&& matches!(cast_to.kind(), ty::Uint(UintTy::U128))

[blyxyas]

Sorry for the delay, I actually viewed this change the day you committed it but forgot to approve it 😅

LGTM, thanks! ❤️ Could you squash the commits into one?

[blyxyas]

@bors r+

[bors]

📌 Commit 477108d has been approved by blyxyas

It is now in the queue for this repository.

[bors]

⌛ Testing commit 477108d with merge 34766a6...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 34766a6 to master...

[TomFryersMidsummer]

I'm not sure this should have been turned off. To expand on what @Alexendoo wrote:

This lint guards against two possible changes which would make the cast lossless.

1. Changing the source type to a larger one.
2. Changing the destination type to a smaller one.

While u128 is indeed immune to the first problem, it's (particularly) susceptible to the second.

For instance, take this code.

type Id = u128;

fn from_database_id(x: u64) -> Id {
    x as Id
}

If Id were to be made a u32, from_database_id would no longer be injective, which could cause a bug. Using Id::from would prevent this.

[smoelius]

If Id were to be made a u32, from_database_id would no longer be injective, which could cause a bug.

Your concern is valid, but I think cast_possible_truncation would fire in that case (playground).

(I should have mentioned this in #12492.)

[Alexendoo]

If anything that triggers cast_lossless becomes lossy it would trigger one of the other cast_ lints (unless we're missing a case)

[smoelius]

If anything that triggers cast_lossless becomes lossy it would trigger one of the other cast_ lints (unless we're missing a case)

I am willing to believe that, but is that an argument for keeping things the way they were? Could you explain?

[Alexendoo]

It was to say that it's consistent with the other cast_lossless cases and isn't a reason to remove as u128

My view of it is the lint suggests replacing n as T with T::from(n) where possible to prevent future changes to n or T from causing unexpected behaviour. It also benefits the reader as they no longer have to consider if there is truncation/wrapping/etc happening

There are lints to catch these potentially unexpected casts, but they're all in pedantic and often #[allow]d anyway because e.g. T::try_from(n).unwrap() is a bit ugly

For the suggestion of replacing n as u128 with u128::from(n) specifically it's true that no change to n can cause truncation to occur but that's not the only thing it protects against. As mentioned a change to the destination type (potentially through an alias) may cause issues

But also changes to the type of n can cause issues, if it becomes a signed integer type the from version will become a compile error and have you consider how you want to handle negatives. The as cast could be the answer to that, but you don't always want sign extension or the 2's complement form

[smoelius]

Thank you very much for explaining. I think this is the main point where we disagree:

As mentioned a change to the destination type (potentially through an alias) may cause issues

I agree this could happen, but it feels remote. To me, a change the type of the expression seems far more likely, and flagging such possibilities is where the lint really provides value.

Having said that, this is not a hill I'd want to die on. So thank you again for taking the time to write #12496 (comment).

[Alexendoo]

Thanks, there's a while until the next meeting so let's just go with a poll since it's split:

https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Should.20.60as.20u128.60.20trigger.20cast_lossless

[smoelius]

@Alexendoo Thank you for opening the poll.

@blyxyas Thank you for reviewing this PR.

@Jacherr Thank you jumping on my issue and resolving it. I'm sorry that your contribution will likely be reverted.