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.

Sign up for GitHub

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

Jump to bottom

Don't lint assertions_on_constants on any const assertions #12840

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Don't lint assertions_on_constants on any const assertions #12840

wants to merge 7 commits into from
+105 −22

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented May 24, 2024 •
edited

changelog: Don't lint assertions_on_constants on any const assertions
changelog: fix false-positive of unnecessary_operation lint in const contexts.

close #12816
close #12847
cc #12817

@rustbot
Copy link
Collaborator

rustbot commented May 24, 2024

r? @dswij

rustbot has assigned @dswij.
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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 24, 2024
dswij
dswij reviewed May 24, 2024
View reviewed changes
clippy_lints/src/default_numeric_fallback.rs Outdated Show resolved Hide resolved
tests/ui/assertions_on_constants.stderr Outdated
error: `assert!(true)` will be optimized out by the compiler
--> tests/ui/assertions_on_constants.rs:49:19
|
LL | const _: () = assert!(true);
Copy link
Member

Choose a reason for hiding this comment

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

Is this intended to affect those that are not explicitly a block? i.e. inside {}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is intended. The lint now doesn't fire on any const-assertions.

Copy link
Contributor Author

@tesuji tesuji May 27, 2024 •
edited

Choose a reason for hiding this comment

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

We could catch the true literal value. As it sems like a programming typo?
Happy to hear your thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

assert!(true) isn't much use, and is linted by assertion_on_const for that reason.

Looking at #12816, why do we want to skip linting assert!(8 == (7+1));? Is there a scenario where checking 8 == 7+1 makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

8 == 7+1 is just an illustration for simple cases. It could be a more complex expression, like checking layout equality of different types. But I don't think checking if expressions is simple or complex is worth the complexity.
Also with the case of 8 == 7+1 get a typo, we have a compile-time panicking. So asserting 8 == 7+1 in const context is kinda worth it.

So maybe I could add a comment about this in test file?

Copy link
Member

Choose a reason for hiding this comment

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

I think the layout case is handled by #11966. It just doesn't cover correctly when it's in a block. Can we build on that and just add the block case to the check?

#11966 is the one that added this test case.

Copy link
Contributor Author

@tesuji tesuji Jun 8, 2024 •
edited

Choose a reason for hiding this comment

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

It's done by a0234b4

@tesuji tesuji changed the title Don't lint assertions-on-constants when it is const assertions Don't lint assertions-on-constants on any const assertions May 24, 2024
@tesuji tesuji mentioned this pull request May 26, 2024
Closed
@tesuji tesuji mentioned this pull request May 26, 2024
Closed
@tesuji
Copy link
Contributor Author

tesuji commented Jun 5, 2024

It's been 2 weeks without new review
Reassigning : r? y21

@rustbot rustbot assigned y21 and unassigned dswij Jun 5, 2024
@tesuji tesuji force-pushed the const-asserts branch 2 times, most recently from e6cb5a2 to 7b06d72 Compare June 8, 2024 09:39
@tesuji
Copy link
Contributor Author

tesuji commented Jun 8, 2024 •
edited

Build fail as expected. In summary: body_const_context doesn't recognize const blocks.
See rust-lang/rust#125918 for more details

So waiting for next rustup...
@rustbot block

Edit:
Nevermind, just bless the test

@tesuji
Ignore non ExprKind::{Path,Lit) inside const context
a0234b4 
- remove now dead code in ASSERTIONS_ON_CONSTANTS
  cc rust-lang#11966
- Partially revert "ignore `assertions-on-constants` in const contexts"
  This reverts commit c7074de.
@tesuji
Copy link
Contributor Author

tesuji commented Jun 19, 2024

rerolling r? @llogiq

@rustbot rustbot assigned llogiq and unassigned y21 Jun 19, 2024
@llogiq
Copy link
Contributor

llogiq commented Jun 19, 2024

Looks ok to me. Thank you for staying the course.

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 19, 2024

📌 Commit 0d188f0 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jun 19, 2024

⌛ Testing commit 0d188f0 with merge 7de2cd7...

bors added a commit that referenced this pull request Jun 19, 2024
@bors
Auto merge of #12840 - tesuji:const-asserts, r=llogiq
7de2cd7 
Don't lint `assertions-on-constants` on any const assertions

changelog: Don't lint `assertions-on-constants` on any const assertions
changelog: fix false-positive of `unnecessary-operation` lint in const contexts.

close #12816
close #12847
cc #12817
@bors
Copy link
Collaborator

bors commented Jun 19, 2024

💔 Test failed - checks-action_test

@tesuji
Copy link
Contributor Author

tesuji commented Jun 20, 2024 •
edited

Blessed the test after rustup !

@tesuji tesuji changed the title Don't lint assertions-on-constants on any const assertions Don't lint assertions_on_constants on any const assertions Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dswij dswij dswij left review comments

Assignees

@llogiq llogiq

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
6 participants
@tesuji @rustbot @llogiq @bors @dswij @y21