Skip to content

Add enum_intrinsics_non_enums lint #83908

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Oct 11, 2021
Merged

Add enum_intrinsics_non_enums lint #83908

merged 2 commits into from
Oct 11, 2021

Conversation

Flying-Toast
Copy link
Contributor

@Flying-Toast Flying-Toast commented Apr 5, 2021

There is a clippy lint to prevent calling mem::discriminant with a non-enum type. I think the lint is worthy of being included in rustc, given that discriminant::<T>() where T is a non-enum has an unspecified return value, and there are no valid use cases where you'd actually want this.

I've also made the lint check variant_count (#73662).

closes #83899

@rust-highfive
Copy link
Contributor

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 5, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Flying-Toast
Copy link
Contributor Author

While I'm at it, I made the lint also check variant_count

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Flying-Toast Flying-Toast changed the title Add mem_discriminant_non_enum lint (from clippy) Add enum_intrinsics_non_enums lint Apr 6, 2021
@rust-log-analyzer

This comment has been minimized.

@flip1995
Copy link
Member

flip1995 commented Apr 6, 2021

@Flying-Toast When this lint is uplifted, the Clippy lint should be deprecated properly. Since this is a lengthy procedure and explaining it takes longer than doing it myself, please ping me once the rustc changes are ready and I can add the patch to deprecate the Clippy lint.

@Flying-Toast
Copy link
Contributor Author

@Flying-Toast When this lint is uplifted, the Clippy lint should be deprecated properly. Since this is a lengthy procedure and explaining it takes longer than doing it myself, please ping me once the rustc changes are ready and I can add the patch to deprecate the Clippy lint.

@flip1995 Thanks. This PR is ready now

@flip1995
Copy link
Member

flip1995 commented Apr 13, 2021

Thanks! I'm waiting for a review of the rustc part and confirmation that this lint should be uplifted. After that, I'll provide the necessary Clippy changes.

@crlf0710 crlf0710 added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 15, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 31, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2021
@Dylan-DPC-zz
Copy link

@estebank any update?

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2021
@bors
Copy link
Collaborator

bors commented Jul 18, 2021

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

@Flying-Toast
Copy link
Contributor Author

@estebank when do you think you can review this?

@camelid
Copy link
Member

camelid commented Aug 15, 2021

r? @davidtwco

@rust-highfive rust-highfive assigned davidtwco and unassigned estebank Aug 15, 2021
@bors
Copy link
Collaborator

bors commented Aug 16, 2021

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

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

LGTM, r=me after rebase, apologies this took so long to be reviewed.

@flip1995
Copy link
Member

@Flying-Toast please apply the patch in this gist for the PR to pass the Clippy test suite.

@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
@davidtwco davidtwco 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@davidtwco
Copy link
Member

@Flying-Toast GitHub is still reporting your branch as having conflicts, have you been able to rebase?

@JohnCSimon
Copy link
Member

Ping again from triage:
@Flying-Toast GitHub is still reporting your branch as having conflicts, have you been able to rebase?

@Flying-Toast
Copy link
Contributor Author

@JohnCSimon @davidtwco I've rebased the PR, sorry for the delay - I've been busy with school. @flip1995: The patch doesn't apply very well anymore by now - could you make the patch again?

@camelid
Copy link
Member

camelid commented Oct 11, 2021

@Flying-Toast rust-lang/rust follows a "No-Merge Policy". Could you please rebase over upstream changes instead of merging? Thanks!

Flying-Toast and others added 2 commits October 11, 2021 09:46
This lint has been uplifted and is now included in
enum_intrinsics_non_enums.
@flip1995
Copy link
Member

flip1995 commented Oct 11, 2021

I updated the gist. This is based on a rebased version of this branch ontop of the current master branch.

I also pushed a already rebased version with this patch applied here: https://github.com/flip1995/rust/tree/enum_intrinsics_non_enums You can just force push this to your branch:

git remote add flip1995 https://github.com/flip1995/rust --fetch
# The next command assumes, that `origin` is the remote of your fork.
git push origin enum_intrinsics_non_enums:master --force-with-lease

@Flying-Toast
Copy link
Contributor Author

@flip1995 thanks :) I've pushed from your branch.

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

LGTM!

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 11, 2021

📌 Commit f483676 has been approved by davidtwco

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 11, 2021
@bors
Copy link
Collaborator

bors commented Oct 11, 2021

⌛ Testing commit f483676 with merge 5b21064...

@bors
Copy link
Collaborator

bors commented Oct 11, 2021

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 5b21064 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 11, 2021
@bors bors merged commit 5b21064 into rust-lang:master Oct 11, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 11, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5b21064): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uplift clippy::mem_discriminant_non_enum into rustc