[VarCouldBeVal] fix overrides false positives

[meriouma]

I'm not sure this is correct as this seems a simple check, but this fixes #4661 and does not break other tests.

[marschwar]

Thank you for submitting the PR.

While I agree, that a lateinit var can never really become a val, I think the underlying issue here is the magic done by the framework. To detekt the variable never changes its value and therefor it could be val. Therefor changing your config to

  VarCouldBeVal:
    active: true
    ignoreAnnotated: ['Inject']

would in my opinion also be a valid fix.

Long story short - I think your approach here is valid. The issue that a private lateinit var is never assigned a value should be handled by a different rule.

[codecov]

Codecov Report

Merging #4664 (b65dbd5) into main (0be7775) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #4664   +/-   ##
=========================================
  Coverage     84.58%   84.58%           
  Complexity     3434     3434           
=========================================
  Files           492      492           
  Lines         11329    11329           
  Branches       2058     2058           
=========================================
  Hits           9583     9583           
  Misses          701      701           
  Partials       1045     1045           

Impacted Files	Coverage Δ
...lab/arturbosch/detekt/rules/style/VarCouldBeVal.kt	83.50% <100.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 0be7775...b65dbd5. Read the comment docs.

[meriouma]

Thank you for submitting the PR.

While I agree, that a lateinit var can never really become a val, I think the underlying issue here is the magic done by the framework. To detekt the variable never changes its value and therefor it could be val. Therefor changing your config to

  VarCouldBeVal:
    active: true
    ignoreAnnotated: ['Inject']

would in my opinion also be a valid fix.

Long story short - I think your approach here is valid. The issue that a private lateinit var is never assigned a value should be handled by a different rule.

I ended up doing this workaround for the Inject annotation as this was the main issue in one of our project. But there are many frameworks that rely on an annotation and a lateinit var field, like Mockito. So instead of ignoring all the annotations, I thought lateinit could be ignored by the rule. I believe there's already the rule LateinitUsage to check for lateinit anyway?

[BraisGabin]

I don't think that we should ignore lateinit var by default. They are var so if they aren't assigned they should be flagged. We have methods to disable properties with annotations. At most I would say to add a configuration, but i would like to see a user case where it is needed. Dagger or mockito doesn't need this to work.

[marschwar]

@meriouma Could you please change this PR to address the issue with override only. I believe that part is not controversial.

I can see the arguments on either side. Since there is an easy workaround for your specific problem I would also not change it at the moment. Maybe this becomes a pattern and we could change it in the future.

[meriouma]

@marschwar Done!

[marschwar]

@meriouma Thanks again for picking this up. I would like us to merge #4575 first to avoid a moving target there and would ask your to adopt your test case to junit if that is ok. I am sorry for the inconvenience.

[meriouma]

@marschwar Sure, no problem. I can rebase and rewrite the test case once #4575 is merged.

[schalkms]

PR #4575 has been merged.

[meriouma]

I've rebased and updated the test.

[BraisGabin]

Thanks!