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

Rule 6.1.7 implicit backing property(#443) #465

Merged
merged 20 commits into from
Nov 23, 2020

Conversation

aktsay6
Copy link
Collaborator

@aktsay6 aktsay6 commented Oct 30, 2020

Which rule and warnings did you add?

In case of using "implicit backing property" scheme, the name of real and back property should be the same

This pull request closes #443

Actions checklist

  • Implemented Rule, added Warnings
  • Added tests on checks
  • Added tests on fixers
  • Updated diktat-analysis.yml
  • Updated available-rules.md

### What's done:
  * First logic
  * Added tests
### What's done:
  * Logic made
  * Added warn tests
@aktsay6 aktsay6 added the enhancement New feature or request label Oct 30, 2020
@aktsay6 aktsay6 requested review from petertrr and kentr0w October 30, 2020 12:20
#443)

# Conflicts:
#	diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt
#	info/available-rules.md
@codecov
Copy link

codecov bot commented Oct 30, 2020

Codecov Report

Merging #465 (4f0debd) into master (052bd9d) will increase coverage by 0.17%.
The diff coverage is 92.42%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #465      +/-   ##
============================================
+ Coverage     81.68%   81.86%   +0.17%     
- Complexity     1641     1696      +55     
============================================
  Files            79       85       +6     
  Lines          4139     4257     +118     
  Branches       1307     1325      +18     
============================================
+ Hits           3381     3485     +104     
- Misses          229      237       +8     
- Partials        529      535       +6     
Flag Coverage Δ Complexity Δ
unittests 81.86% <92.42%> (+0.17%) 0.00 <38.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...at-rules/src/main/kotlin/generated/WarningNames.kt 0.00% <ø> (ø) 0.00 <0.00> (ø)
...lin/org/cqfn/diktat/ruleset/rules/PackageNaming.kt 93.75% <33.33%> (-1.19%) 32.00 <0.00> (ø)
...ktat/ruleset/rules/AvoidEmptyPrimaryConstructor.kt 90.00% <90.00%> (ø) 8.00 <8.00> (?)
...iktat/ruleset/rules/ImplicitBackingPropertyRule.kt 93.75% <93.75%> (ø) 23.00 <23.00> (?)
...tlin/org/cqfn/diktat/ruleset/constants/Warnings.kt 98.34% <100.00%> (+0.02%) 10.00 <0.00> (ø)
...cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt 97.33% <100.00%> (+0.07%) 8.00 <0.00> (ø)
...qfn/diktat/ruleset/rules/classes/SingleInitRule.kt 78.46% <100.00%> (+0.68%) 16.00 <0.00> (+6.00)
.../cqfn/diktat/ruleset/rules/files/BlankLinesRule.kt 100.00% <100.00%> (ø) 13.00 <7.00> (+1.00)
...rg/cqfn/diktat/ruleset/rules/files/NewlinesRule.kt 83.95% <100.00%> (+0.17%) 110.00 <0.00> (+6.00)
...g/cqfn/diktat/ruleset/rules/kdoc/KdocFormatting.kt 77.12% <100.00%> (-0.15%) 73.00 <0.00> (ø)
... and 8 more

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 cd83754...4f0debd. Read the comment docs.

Copy link
Member

@petertrr petertrr left a comment

Choose a reason for hiding this comment

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

See comments above

@aktsay6 aktsay6 requested a review from petertrr November 2, 2020 09:04
Copy link
Member

@petertrr petertrr left a comment

Choose a reason for hiding this comment

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

Please take a look at snapshot check. Your rule triggers quite a lot in diktat-test-framework with false-positives. Please add similar examples to tests, one idea how to fix algorithm is in comment below.

@aktsay6 aktsay6 requested a review from petertrr November 3, 2020 11:39
Copy link
Member

@petertrr petertrr left a comment

Choose a reason for hiding this comment

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

False-positives are still there, please add more tests (like TestProcessingFactory.allTestsFromResources in diktat-test-framework). We need to come up with better algorithm for identifying such backing properties. Maybe, check that a certain class property is used both in an assignment in setter and in the return statement in getter?

#443)

# Conflicts:
#	diktat-analysis.yml
#	diktat-rules/src/main/kotlin/generated/WarningNames.kt
#	diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt
#	diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt
#	diktat-rules/src/main/resources/diktat-analysis-huawei.yml
#	diktat-rules/src/main/resources/diktat-analysis.yml
#	info/available-rules.md
#443)

# Conflicts:
#	diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt
Copy link
Member

@petertrr petertrr left a comment

Choose a reason for hiding this comment

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

https://github.com/cqfn/diKTat/pull/465/checks?check_run_id=1440909442
Warning is raised on declaredOptions in TestArgumentsReader, where local variable is returned from getter. We should trigger this warning only for class properties.

@aktsay6 aktsay6 requested a review from petertrr November 23, 2020 10:52
.findAllNodesWithSpecificType(PROPERTY)
.map { (it.psi as KtProperty).name!! }

if (refExprs.isNotEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that any reference to non-local property will cause warning? What if some other class property is read for some reason? I think, similar to getters, we should check only left-hand side references in assignments. For example, this code:

val foo
    set(value) {
        if (isDebugMode) log.debug(value)
        field = value
    }

shouldn't raise a warning beca use there is no backing property.

Copy link
Member

@petertrr petertrr left a comment

Choose a reason for hiding this comment

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

LGTM

aktsay6 and others added 4 commits November 23, 2020 19:05
#443)

# Conflicts:
#	diktat-analysis.yml
#	diktat-rules/src/main/kotlin/generated/WarningNames.kt
#	diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt
#	diktat-rules/src/main/resources/diktat-analysis-huawei.yml
#	diktat-rules/src/main/resources/diktat-analysis.yml
…ing-property(#443)' into feature/rule-6.1.7-implicit-backing-property(#443)
@aktsay6 aktsay6 merged commit 4ab49e1 into master Nov 23, 2020
@aktsay6 aktsay6 deleted the feature/rule-6.1.7-implicit-backing-property(#443) branch November 23, 2020 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants