[refurb] Implement unnecessary-enumerate (FURB148)

[tjkuson]

Summary

Implement no-ignored-enumerate-items as unnecessary-enumerate (FURB148).

The auto-fix considers if a start argument is passed to the enumerate() function. If only the index is used, then the suggested fix is to pass the start value to the range() function. So,

for i, _ in enumerate(xs, 1):
    ...

becomes

for i in range(1, len(xs)):
    ...

If the index is ignored and only the value is ignored, and if a start value greater than zero is passed to enumerate(), the rule doesn't produce a suggestion. I couldn't find a unanimously accepted best way to iterate over a collection whilst skipping the first n elements. The rule still triggers, however.

Related to #1348.

Test Plan

cargo test

[github-actions]

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

[Skylion007]

I couldn't find a unanimously accepted best way to iterate over a collection whilst skipping the first n elements.

Isn't this explicitly what itertools.islice is designed for? Specifically, itertools.islice(start_index, None).

[tjkuson]

Isn't this explicitly what itertools.islice is designed for? Specifically, itertools.islice(start_index, None).

Yes, and it's what I would personally do. But, looking online, there are alternative methods without a consensus over which is best. My concern is that a non-trivial number of people would consider an itertools.isclice auto-fix (or alternative, for that matter) unexpected behaviour.

Very happy to adapt the merge request to add an islice auto-fix if the maintainers support it, however!

EDIT: The islice solution has the advantage of working on all iterables, whereas solutions like iterating over a [n:] sliced sequence won't work on all iterables. Could be a reason to settle on this fix over others?

[Skylion007]

The first upvoted answer wouldn't work on generators. The 2nd upvoted answer is verbose and just suggests calling next on the iterator, leaving islice as the most upvoted answer in that StackOverflow post.

…

On Sun, Sep 17, 2023 at 3:57 PM Tom Kuson ***@***.***> wrote: Isn't this explicitly what itertools.islice <https://docs.python.org/3/library/itertools.html#itertools.islice> is designed for? Specifically, itertools.islice(start_index, None). Yes, and it's what I would personally do. But, looking online <https://stackoverflow.com/questions/10079216/skip-first-entry-in-for-loop-in-python>, there are alternative methods without a consensus over which is best. My concern is that a non-trivial number of people would consider an itertools.isclice auto-fix (or alternative, for that matter) unexpected behaviour. Very happy to adapt the merge request to add an islice auto-fix if the maintainers support it, however! — Reply to this email directly, view it on GitHub <#7454 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPVMX6MLD56J5SXFBJCRMLX25I35ANCNFSM6AAAAAA43RTTEQ> . You are receiving this because you commented.Message ID: ***@***.***>

[dhruvmanila]

This is looking good, thanks for doing this!

I think the implementation is similar to https://docs.astral.sh/ruff/rules/incorrect-dict-iterator/, maybe in the future we could combine and parameterize the implementation.

[dhruvmanila]

I wonder if this could be made a method on the SemanticModel:

impl SemanticModel {
	// ...

	pub fn is_expr_unused(&self, expr: &Expr) -> bool {
		// ...
	}	
}

checker.semantic().is_expr_unused(expr)

But then it would mean that we would need to visit all relevant expressions in this context which I think is out of scope for this PR.

[dhruvmanila]

You could probably just use map instead:

Suggested change

	if let Some(suggestion) = iterable_suggestion
	.as_ref()
	.and_then(SourceCodeSnippet::full_display)
	if let Some(suggestion) = iterable_suggestion.map(SourceCodeSnippet::full_display)

[tjkuson]

The suggestion causes a type mismatch, is there a better way?

[dhruvmanila]

Same suggestion as above.

[charliermarsh]

Thanks! I made some modifications. I think the start argument to enumerate just changes the value of the index, it doesn't skip the first start elements. Notice how in this example from the docs, they iterate over the same arguments, but the index is different. So I changed the fix in "unused index" case to just ignore start altogether, and I changed the unused value case to avoid fixing if start was set to something other than the default.

[tjkuson]

Oops, that was silly of me. Thanks!

[Skylion007]

FYI @tjkuson , the fix generates invalid code when the arg is a generator: #7656