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

Linter for .as(()) => .void #181

Merged
merged 1 commit into from
Dec 18, 2024
Merged

Linter for .as(()) => .void #181

merged 1 commit into from
Dec 18, 2024

Conversation

TonioGela
Copy link
Member

This is absolutely my first scalafix rule contribution, so please review it with extra care 😇

I think that this rule might have a non zero false-positive ratio, as (I think) it will match ALL the method/functions called as(()), not just the one available in cats functor, but I don't know if this is acceptable.

@DavidGregory084 I'm requesting you as a reviewer as I saw that you've implemented a good number of rules in this repo.

@TonioGela TonioGela requested a review from DavidGregory084 July 5, 2024 06:26
@armanbilge
Copy link
Member

armanbilge commented Jul 15, 2024

Thanks for this PR!

We also have this related rule.

class As extends SemanticRule("TypelevelAs") {

(I think) it will match ALL the method/functions called as(()), not just the one available in cats functor, but I don't know if this is acceptable.

I think you are right, but that is also how the existing rules are implemented. I think that this choice of implementation makes it possible to run the rule directly on (uncompiled) sources (but I'm hardly an expert). An implementation that checks the FQN is possible but would need a Semantic DB generated first.

However that seems inconsistent with the fact that these are SemanticRules (needs Semantic DB) vs SyntacticRules. I wonder if this was an oversight.

Another thing worth considering is if instead of simply linting, whether the rule should actually directly apply the rewrite. Of course this would only make sense if it is a true SemanticRule.

@TonioGela
Copy link
Member Author

We also have this related rule.

Aren't you linking precisely the rule I opened a PR against? :D

I think you are right, but that is also how the existing rules are implemented. I think that this choice of implementation makes it possible to run the rule directly on (uncompiled) sources

That's precisely the thing, I read the whole documentation and I can confirm this

An implementation that checks the FQN is possible but would need a Semantic DB generated first.

Also true, the advertised thing is that SemanticRules are (ofc) way slower to check than SyntacticRules

Another thing worth considering is if instead of simply linting, whether the rule should actually directly apply the rewrite. Of course this would only make sense if it is a true SemanticRule.

Given that this rule users are used to have a slower (as it's semantic) rule, it might be the case to squat the further information we have and make it rewrite the relevant pieces of code.

There's a thing to consider anyway (something that I've experienced the hard way). as is strict, and there's at least one case where .map(_ => f) behaves differently from as(f): mutability. For this very reason I think I'll try to make this rule emit a code rewriting patch just in the .map(_ => ()) and .as(()) cases. WDYT?

@armanbilge armanbilge closed this Dec 18, 2024
@armanbilge armanbilge reopened this Dec 18, 2024
@armanbilge armanbilge merged commit 6b86461 into main Dec 18, 2024
16 checks passed
@armanbilge armanbilge deleted the as-unit-to-void branch December 18, 2024 19:02
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.

2 participants