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

ci: fail linting on truncated and wrapping casts #5233

Closed

Conversation

AaronFeickert
Copy link
Collaborator

@AaronFeickert AaronFeickert commented Mar 8, 2023

Description

Fails linting on possible truncated or wrapping casts. Various updates to address failures.

Closes #5478.

Motivation and Context

It was observed in another PR that truncated casts are not picked up by the linter. The clippy::cast_possible_truncation lint will catch this, but was disabled. Some of these casts may be risky, so it seems prudent to fail on them unless specifically allowed and documented.

This PR enables the lint as a deny, as well as a related lint for wrapping. For each lint failure, it either uses an allow (if the truncation is impossible or safe) or makes an update to fix it.

Note that after PR 5237 is merged, it can be assumed that the target usize is not 32 bits.

How Has This Been Tested?

It hasn't. This behavior occurs in multiple places (around a couple dozen) within the codebase; in each case, it needs to be decided if the cast is safe (in which case the lint can be selectively disabled) or risky (in which case it should be replaced). This should happen before the PR is merged.

@AaronFeickert AaronFeickert marked this pull request as draft March 8, 2023 17:59
@AaronFeickert
Copy link
Collaborator Author

This PR is a draft until each instance of the lint behavior is addressed in the codebase. Seems reasonable to include them all here.

@AaronFeickert AaronFeickert force-pushed the cast-off branch 3 times, most recently from f9f8d1c to 88c7165 Compare March 14, 2023 21:40
@ghpbot-tari-project ghpbot-tari-project added P-conflicts Process - The PR has merge conflicts that need to be resolved P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Jun 20, 2023
@AaronFeickert AaronFeickert changed the title ci: fail linting on truncated casts ci: fail linting on truncated and wrapping casts Jun 20, 2023
@ghpbot-tari-project ghpbot-tari-project removed the P-conflicts Process - The PR has merge conflicts that need to be resolved label Jun 20, 2023
@github-actions
Copy link

Test Results (Integration tests)

0 tests  ±0   0 ✔️ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ±0 

Results for commit 278bc4e. ± Comparison against base commit ef98482.

@github-actions
Copy link

Test Results (CI)

0 tests   - 1 147   0 ✔️  - 1 147   0s ⏱️ - 11m 29s
0 suites  -      37   0 💤 ±       0 
0 files    -        1   0 ±       0 

Results for commit 278bc4e. ± Comparison against base commit ef98482.

@AaronFeickert
Copy link
Collaborator Author

This has become wildly out of date, and rebasing seems like an exercise in futility. Closing so that a newer, shinier, better PR can take its place.

@AaronFeickert AaronFeickert deleted the cast-off branch June 27, 2023 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linting for cast truncation and wrapping is disabled
2 participants