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

Range::step_by(0) #95

Closed
llogiq opened this issue Jun 12, 2015 · 11 comments
Closed

Range::step_by(0) #95

llogiq opened this issue Jun 12, 2015 · 11 comments
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST

Comments

@llogiq
Copy link
Contributor

llogiq commented Jun 12, 2015

This currently doesn't even panic, but happily loops forever. Well, unless it is clipped with .take(_) or some such.

@Manishearth Manishearth added good-first-issue These issues are a good way to get started with Clippy T-middle Type: Probably requires verifiying types labels Aug 11, 2015
@Manishearth
Copy link
Member

We should probably have a single file which handles all lints which ban a type, function, or method

@Manishearth Manishearth added the A-lint Area: New lints label Aug 11, 2015
@llogiq
Copy link
Contributor Author

llogiq commented Aug 11, 2015

Yeah, this should probably be in the same LintPass as #96.

@Manishearth
Copy link
Member

Actually, no, #96 is a pure AST match, whereas all of these (Option::unwrap, Range::step_by(0)) are middle-based checks that need to use expr_ty and match_def_path

@llogiq
Copy link
Contributor Author

llogiq commented Aug 11, 2015

Most ranges will be defined in the same line of code as their .step_by(..), so even if we don't use middle and just match either ExprMethodCall{step_by, [ExprRange{Lit(from), Lit(to)}, ref step]} or just ExprRange{..} without step_by (simplified here), we can probably catch a good percentage of occurrences.

We would need const folding for the check anyway (because the type won't tell us the range boundaries), so expr_ty would give us no benefit whatsoever.

@Manishearth
Copy link
Member

No, I meant that all of these "ban a method" etc use expr_ty and match_def_path in a uniform way, which we can do all at once in a single place. (That's how I'm proposing this be organized, at least, feel free to propose something else 😄 )

@llogiq
Copy link
Contributor Author

llogiq commented Aug 11, 2015

I agree wholeheartedly with your organization, but I think you've misclassified this issue. As I said, this should be a pure AST match + const folding.

@Manishearth
Copy link
Member

Oh, you intend for this lint to only work on range literals?

Works for me!

@Manishearth Manishearth added T-AST Type: Requires working with the AST and removed T-middle Type: Probably requires verifiying types labels Aug 11, 2015
@Manishearth
Copy link
Member

Note: Using T-AST for lints which need const folding but are otherwise pure AST

@llogiq
Copy link
Contributor Author

llogiq commented Aug 11, 2015

Full ack. Once we have the const folding separated out into a utils method, there's no challenge in using it anyway 😄

@nweston
Copy link
Contributor

nweston commented Aug 15, 2015

I'm going to take a stab at this today.

My plan is to warn on any use of step_by(0). Even if there's a subsequent call to .take(_) or similar that prevents an infinite loop, std::iter::repeat is preferable. So I can't think of any case where step_by(0) is a good idea.

Are there any examples of how to do constant folding?

@Manishearth
Copy link
Member

Ignore the constant folding for a bit, that will be available once @llogiq's stuff merges. Match on literal zeroes and leave a FIXME comment about constant folding

nweston added a commit to nweston/rust-clippy that referenced this issue Aug 15, 2015
nweston added a commit to nweston/rust-clippy that referenced this issue Aug 15, 2015
@llogiq llogiq closed this as completed in 23a38c4 Aug 16, 2015
llogiq added a commit that referenced this issue Aug 16, 2015
New lint: Range::step_by(0) (fixes #95)
toidiu pushed a commit to toidiu/rust-clippy that referenced this issue Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST
Projects
None yet
Development

No branches or pull requests

3 participants