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

Disable arithmetic lints in constant items #3331

Merged
merged 1 commit into from
Oct 25, 2018

Conversation

pengowen123
Copy link
Contributor

Currently this will not catch cases in associated constants. I'm not sure whether checking spans is the best way to solve this issue, but I don't think it will cause any problems.
Fixes #1858

@oli-obk
Copy link
Contributor

oli-obk commented Oct 18, 2018

This won't catch array lengths. You can get them the easiest by having an impl of check_anon_const (I hope that exists, just guessing here). This would also mean you won't need the enum arm for the item check anymore

I think you should name the item_span as const_span and add a doc comment explaining why we do this.

@pengowen123 pengowen123 force-pushed the fix_integer_arithmetic branch from bc845c2 to 7dea4dc Compare October 18, 2018 15:22
@pengowen123
Copy link
Contributor Author

I changed the name and added a doc comment. There doesn't seem to be any similar method to check_anon_const in LateLintPass. The only method I found that catches constants is check_body, but it doesn't seem possible to use that here without access to the HIR map. Perhaps check_anon_const should be added to rustc?

@pengowen123 pengowen123 force-pushed the fix_integer_arithmetic branch from 7dea4dc to 25d63c8 Compare October 24, 2018 04:46
@pengowen123
Copy link
Contributor Author

@oli-obk I've rewritten the checks to use check_body. This seems to catch everything.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

r=me with nit fixed

fn check_body(&mut self, cx: &LateContext<'_, '_>, body: &hir::Body) {
let body_owner = cx.tcx.hir.body_owner(body.id());

if let hir::BodyOwnerKind::Fn = cx.tcx.hir.body_owner_kind(body_owner) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you change this to != instead of having an empty arm on if let

Copy link
Contributor

Choose a reason for hiding this comment

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

or do an exhaustive match on all BodyOwnerKind variants so future changes don't trip us up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@pengowen123 pengowen123 force-pushed the fix_integer_arithmetic branch from 25d63c8 to 0b9e9c9 Compare October 25, 2018 03:27
@flip1995
Copy link
Member

bors r=oli-obk

@bors
Copy link
Contributor

bors bot commented Oct 25, 2018

👎 Rejected by code reviews

@oli-obk
Copy link
Contributor

oli-obk commented Oct 25, 2018

bors r+

weird... feel free to dismiss my reviews in such situations

bors bot added a commit that referenced this pull request Oct 25, 2018
3331: Disable arithmetic lints in constant items r=oli-obk a=pengowen123

Currently this will not catch cases in associated constants. I'm not sure whether checking spans is the best way to solve this issue, but I don't think it will cause any problems.
Fixes #1858

Co-authored-by: Owen Sanchez <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 25, 2018

@bors bors bot merged commit 0b9e9c9 into rust-lang:master Oct 25, 2018
@pengowen123 pengowen123 deleted the fix_integer_arithmetic branch October 25, 2018 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants