[refurb] Implement `reimplemented-starmap` rule (`FURB140`) by SavchenkoValeriy · Pull Request #7253 · astral-sh/ruff

SavchenkoValeriy · 2023-09-09T14:45:27Z

Summary

This PR is part of a bigger effort of re-implementing refurb rules #1348. It adds support for FURB140

Test Plan

I included a new test + checked that all other tests pass.

github-actions · 2023-09-09T15:52:50Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

crates/ruff/src/rules/refurb/rules/reimplemented_starmap.rs

MichaReiser · 2023-09-11T07:08:11Z

crates/ruff/src/rules/refurb/rules/reimplemented_starmap.rs

+}
+
+// FURB140
+pub(crate) fn reimplemented_starmap<T: StarmapEquivalent + Ranged>(


The use of a generic here means that Rust generates a dedicated implementation for each T instantiation.

I think it may be better to define a AnyComprehension enum (or similar) with a variant for each node that you want to support instead of using a trait. See

ruff/crates/ruff_python_formatter/src/expression/expr_yield.rs

Lines 10 to 35 in c05e462

pub(super) enum AnyExpressionYield<'a> {

Yield(&'a ExprYield),

YieldFrom(&'a ExprYieldFrom),

}

impl<'a> AnyExpressionYield<'a> {

const fn is_yield_from(&self) -> bool {

matches!(self, AnyExpressionYield::YieldFrom(_))

}

fn value(&self) -> Option<&Expr> {

match self {

AnyExpressionYield::Yield(yld) => yld.value.as_deref(),

AnyExpressionYield::YieldFrom(yld) => Some(&yld.value),

}

}

}

impl Ranged for AnyExpressionYield<'_> {

fn range(&self) -> TextRange {

match self {

AnyExpressionYield::Yield(yld) => yld.range(),

AnyExpressionYield::YieldFrom(yld) => yld.range(),

}

}

}

I'm not a huge fan of such a solution. With the trait, we don't need to perform any runtime checks for the given type. With the enum, we do them 4 times: element, generators, range, and try_fix. These seem not critical in any way, but one can argue that bloating from instantiating the same (rather small) generic 3 times is kind of similar.

I guess I can see how an enum solution is better for decoupling, if I remove try_fix from that interface altogether. Then it can be used in other places that share comprehensions. On the other hand, this interface can't include DictComprehension without making element optional and adding more runtime checks.

Sorry, for the rant. If you insist, I can change it, but I hope you see my point here 😄

crates/ruff/src/rules/refurb/rules/reimplemented_starmap.rs

MichaReiser · 2023-09-11T07:12:49Z

I have a few coding nits. Leaving it to @charliermarsh to review the rule logic

codspeed-hq · 2023-09-16T18:27:15Z

CodSpeed Performance Report

Merging #7253 will degrade performances by 2.38%

_{Comparing SavchenkoValeriy:refurb/furb140 (ea69b1c) with main (c6ba7df)}

Summary

❌ 1 regressions
✅ 24 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`main`	`SavchenkoValeriy:refurb/furb140`	Change
❌	`linter/all-rules[numpy/ctypeslib.py]`	34.2 ms	35.1 ms	-2.38%

SavchenkoValeriy force-pushed the refurb/furb140 branch 3 times, most recently from 1ab6401 to 0f72b35 Compare September 9, 2023 15:36

MichaReiser reviewed Sep 11, 2023

View reviewed changes

charliermarsh added the rule Implementing or modifying a lint rule label Sep 11, 2023

[refurb] Implement reimplemented-starmap rule (FURB140)

4ec3f0b

SavchenkoValeriy force-pushed the refurb/furb140 branch from 0f72b35 to 4ec3f0b Compare September 16, 2023 18:18

charliermarsh added 3 commits September 18, 2023 21:17

Merge branch 'main' into refurb/furb140

f4aa4d1

Use an enum

303a797

Move to preview

5d2be91

charliermarsh enabled auto-merge (squash) September 19, 2023 02:02

charliermarsh force-pushed the refurb/furb140 branch from d014c7c to ea69b1c Compare September 19, 2023 02:08

Fix test

ea69b1c

charliermarsh merged commit 4123d07 into astral-sh:main Sep 19, 2023

chenrui333 mentioned this pull request Sep 22, 2023

ruff 0.0.291 Homebrew/homebrew-core#144913

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refurb] Implement `reimplemented-starmap` rule (`FURB140`)#7253

[refurb] Implement `reimplemented-starmap` rule (`FURB140`)#7253
charliermarsh merged 5 commits intoastral-sh:mainfrom
SavchenkoValeriy:refurb/furb140

SavchenkoValeriy commented Sep 9, 2023

Uh oh!

github-actions bot commented Sep 9, 2023 •

edited

Loading

Uh oh!

Uh oh!

MichaReiser Sep 11, 2023

Uh oh!

SavchenkoValeriy Sep 11, 2023

Uh oh!

Uh oh!

Uh oh!

MichaReiser commented Sep 11, 2023

Uh oh!

codspeed-hq bot commented Sep 16, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	pub(super) enum AnyExpressionYield<'a> {
	Yield(&'a ExprYield),
	YieldFrom(&'a ExprYieldFrom),
	}

	impl<'a> AnyExpressionYield<'a> {
	const fn is_yield_from(&self) -> bool {
	matches!(self, AnyExpressionYield::YieldFrom(_))
	}

	fn value(&self) -> Option<&Expr> {
	match self {
	AnyExpressionYield::Yield(yld) => yld.value.as_deref(),
	AnyExpressionYield::YieldFrom(yld) => Some(&yld.value),
	}
	}
	}

	impl Ranged for AnyExpressionYield<'_> {
	fn range(&self) -> TextRange {
	match self {
	AnyExpressionYield::Yield(yld) => yld.range(),
	AnyExpressionYield::YieldFrom(yld) => yld.range(),
	}
	}
	}

Conversation

SavchenkoValeriy commented Sep 9, 2023

Summary

Test Plan

Uh oh!

github-actions bot commented Sep 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Ecosystem

Uh oh!

Uh oh!

MichaReiser Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

SavchenkoValeriy Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MichaReiser commented Sep 11, 2023

Uh oh!

codspeed-hq bot commented Sep 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #7253 will degrade performances by 2.38%

Summary

Benchmarks breakdown

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Sep 9, 2023 •

edited

Loading

codspeed-hq bot commented Sep 16, 2023 •

edited

Loading