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

Do not report vararg arguments which are just passed to a vararg parameter #3157

Merged
merged 4 commits into from
Oct 20, 2020

Conversation

arturbosch
Copy link
Member

Closes #3145

@arturbosch arturbosch added this to the 1.14.2 milestone Oct 19, 2020
@codecov
Copy link

codecov bot commented Oct 19, 2020

Codecov Report

Merging #3157 into master will decrease coverage by 0.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3157      +/-   ##
============================================
- Coverage     79.52%   79.50%   -0.02%     
- Complexity     2592     2596       +4     
============================================
  Files           437      437              
  Lines          7852     7861       +9     
  Branches       1495     1497       +2     
============================================
+ Hits           6244     6250       +6     
  Misses          817      817              
- Partials        791      794       +3     
Impacted Files Coverage Δ Complexity Δ
...urbosch/detekt/rules/performance/SpreadOperator.kt 81.25% <83.33%> (-5.71%) 12.00 <6.00> (+4.00) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3108bfc...a010d3c. Read the comment docs.

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

In general I approve this PR. However, I wanted to raise the question, whether we want to support this rule without having type resolution enabled?

@arturbosch
Copy link
Member Author

In general I approve this PR. However, I wanted to raise the question, whether we want to support this rule without having type resolution enabled?

As long as our compiler plugin is not 1.0 and the changes are easy, I would say yes :)

@arturbosch arturbosch merged commit d073452 into master Oct 20, 2020
@arturbosch arturbosch deleted the 3145-spread-operator-passthrough branch October 20, 2020 10:35
@chris-tipton
Copy link

It appears this exception to the SpreadOperator rule still requires type resolution. Is that accurate?

@cortinico
Copy link
Member

It appears this exception to the SpreadOperator rule still requires type resolution. Is that accurate?

Yes it's correct.
Moreover I'd like to point out the still requires you mentioned. In the future Detekt will evolve to have type resolution enabled by default. More and more rules will evolve to support type resolution (we will not remove it).

@chris-tipton
Copy link

It appears this exception to the SpreadOperator rule still requires type resolution. Is that accurate?

Yes it's correct.
Moreover I'd like to point out the still requires you mentioned. In the future Detekt will evolve to have type resolution enabled by default. More and more rules will evolve to support type resolution (we will not remove it).

Yeah, I understand that is part of the plan for a 2.0 release. I just wanted to make sure that I wasn't missing anything based on the above comments about supporting this rule without type resolution. Our project currently does not use type resolution, so for the time being I am manually suppressing the rule for cases like this.

Thanks for your response!

@cortinico
Copy link
Member

I just wanted to make sure that I wasn't missing anything based on the above comments about supporting this rule without type resolution

This is one of the rule with "mixed behavior" (see more on #2994). Our take is that we will keep on supporting them for the time being, but will slowly transition to offer more and more Type Resolution capabilities.

In other words: this rule could be converted to by a Type Resolution-only rule as part of our ongoing migration.

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.

False positive performance SpreadOperator in case of pass-through vararg
5 participants