Skip to content

Construct signatures based on named declaration instead of just the identifier#2785

Merged
arturbosch merged 1 commit intomasterfrom
fix-signatures
Jun 10, 2020
Merged

Construct signatures based on named declaration instead of just the identifier#2785
arturbosch merged 1 commit intomasterfrom
fix-signatures

Conversation

@arturbosch
Copy link
Member

@arturbosch arturbosch commented Jun 9, 2020

This fixes a regression introduced in #2702 for baseline files.
See the updated baseline file for an example.

Further it corrects a regression we introduced in PRs like #2061 where we changed the code to report at the identifier:

Entity.from(function.nameIdentifier!!)

in some complexity rules.

A signature should always include the whole function or class header to be unambiguous.

The signature algorithm is in fact wrong as it does not consider the whole signature of the parents of the reported elements.

See example from the testcase:

Test.kt\$C\$private fun memberFun(): Int

This signature includes the whole function header but not the package or extended types of C.
The right signature would be:

test.Test.kt\$C : Any\$private fun memberFun(): Int

In 99.5% of all cases this will be no problem for our users but we should consider fixing this in a major release.


Conclusions:

@arturbosch arturbosch added migration Marker to add a migration step in the changelog regression labels Jun 9, 2020
@arturbosch arturbosch added this to the 1.10.0 milestone Jun 9, 2020
@codecov
Copy link

codecov bot commented Jun 9, 2020

Codecov Report

Merging #2785 into master will not change coverage.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2785   +/-   ##
=========================================
  Coverage     80.53%   80.53%           
  Complexity     2321     2321           
=========================================
  Files           386      386           
  Lines          6952     6952           
  Branches       1260     1260           
=========================================
  Hits           5599     5599           
  Misses          725      725           
  Partials        628      628           
Impacted Files Coverage Δ Complexity Δ
...n/kotlin/io/gitlab/arturbosch/detekt/api/Entity.kt 84.21% <87.50%> (ø) 4.00 <0.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 f4054cb...dc0b761. Read the comment docs.

@BraisGabin
Copy link
Member

In 99.5% of all cases this will be no problem for our users but we should consider fixing this in a major release.

Should we add this to #2680?

@arturbosch
Copy link
Member Author

In 99.5% of all cases this will be no problem for our users but we should consider fixing this in a major release.

Should we add this to #2680?

Thanks for the reminder! I wrote the long text in this PR to also copy half of it to that ticket :).

@arturbosch arturbosch merged commit ac4d439 into master Jun 10, 2020
@arturbosch arturbosch deleted the fix-signatures branch June 10, 2020 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

migration Marker to add a migration step in the changelog regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants