Add LanguageVersionSettings and DataFlowValueFactory to BaseRule #2929
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2929 +/- ##
============================================
+ Coverage 80.23% 80.26% +0.03%
- Complexity 2449 2455 +6
============================================
Files 421 422 +1
Lines 7407 7414 +7
Branches 1354 1356 +2
============================================
+ Hits 5943 5951 +8
+ Misses 764 760 -4
- Partials 700 703 +3
Continue to review full report at Codecov.
|
| @@ -23,9 +27,16 @@ abstract class BaseRule( | |||
| * receive the correct compile classpath for the code being analyzed otherwise the default value | |||
| * BindingContext.EMPTY will be used and it will not be possible for detekt to resolve types or symbols. | |||
There was a problem hiding this comment.
Maybe the documentation of the functions should be updated to comply with the new parameters and implementation.
There was a problem hiding this comment.
lets better hide the information for now :)
| root: KtFile, | ||
| bindingContext: BindingContext = BindingContext.EMPTY, | ||
| languageVersionSettings: LanguageVersionSettings? = null, | ||
| dataFlowValueFactory: DataFlowValueFactory? = null |
There was a problem hiding this comment.
Maybe we should group the 3 objects concerning type and symbol solving into a new object. As a result the parameter list would be smaller.
There was a problem hiding this comment.
Lets go with an object for the new two and leave the BindingContext extra so we do not need to update all rules?
There was a problem hiding this comment.
Introduced CompilerResources class.
|
@t-kameyama thanks for the contribution and showing us a new way of writing more complex rules with smart casting. I'm afraid to tell you that I have to decline this change. I really do not want to make our current rule api and design more complex. It is not the direction we want to go with the Rule and BaseRule classes. There are already things like the context, excludes and aliases which we regret to have added to it. In #2680 we discuss further breaking api changes and list design improvements we want to make for 2.0. |
|
I don’t fully agree with declining this change. |
|
Or we can just allow extending the current rule api following its "spirit" and once we have the new api we can port it. |
|
Hm, convinced. Edit: thanks for the discussion! Yep, exploring the compiler's possibilities is definately the way to go 👍 |
#2903 has tests that requires the detection of smart casting. |
t-kameyama
force-pushed
the
detect_smart_cast_types
branch
2 times, most recently
from
84f8a17
to
f07877d
Compare
t-kameyama
force-pushed
the
detect_smart_cast_types
branch
from
f07877d
to
ec39fca
Compare
| val findingsPerFile: FindingsResult = | ||
| if (settings.spec.executionSpec.parallelAnalysis) { | ||
| runAsync(ktFiles, bindingContext) | ||
| runAsync(ktFiles, bindingContext, compilerResources) |
There was a problem hiding this comment.
Should bindingContext also be moved to compilerResources? @arturbosch
There was a problem hiding this comment.
Ah, yes on the core side we definitely should do this. On the rule side I would like them to be separated as we rey heavily on the BindingContext.
Lets do this in another PR.
|
I've restarted the CI and will merge it then. |
We need to detect smart cast types in #2903.
In order to detect the smart cast types,
LanguageVersionSettingsandDataFlowValueFactoryare required as follows:https://github.com/JetBrains/kotlin/blob/040204e36e8d55395cdbbda2c8145dbb68bb0f00/idea/src/org/jetbrains/kotlin/idea/codeInsight/KotlinExpressionTypeProvider.kt#L116-L119
So we should add them to
BaseRulelikeBindingContext.