Add GlobalCoroutineUsage rule + coroutines ruleset #2174

xouabita · 2019-12-12T10:07:11Z

Add GlobalScopeUsage rule which reports use of GlobalScope.
See Add rule [GlobalScopeUsage] to disallow usage of GlobalScope #2155 for more context
Add concurrency rule set
Add a way to pass optionalPaths in create environment
Add coroutines as a test dependency to have coroutine types in the test

codecov-io · 2019-12-12T11:03:38Z

Codecov Report

Merging #2174 into master will decrease coverage by 0.03%.
The diff coverage is 92.3%.

@@             Coverage Diff              @@
##             master    #2174      +/-   ##
============================================
- Coverage     81.41%   81.37%   -0.04%     
- Complexity     2064     2073       +9     
============================================
  Files           340      343       +3     
  Lines          5934     5981      +47     
  Branches       1077     1086       +9     
============================================
+ Hits           4831     4867      +36     
- Misses          526      532       +6     
- Partials        577      582       +5

Impacted Files	Coverage Δ	Complexity Δ
...bosch/detekt/rules/providers/CoroutinesProvider.kt	`100% <100%> (ø)`	`3 <3> (?)`
...ch/detekt/rules/coroutines/GlobalCoroutineUsage.kt	`88.88% <88.88%> (ø)`	`3 <3> (?)`
.../io/gitlab/arturbosch/detekt/generator/out/Yaml.kt	`58.62% <0%> (-6.77%)`	`0% <0%> (ø)`
...arturbosch/detekt/generator/collection/KDocTags.kt	`77.41% <0%> (-3.35%)`	`0% <0%> (ø)`
...ch/detekt/api/internal/ValidatableConfiguration.kt	`86.36% <0%> (-2.21%)`	`0% <0%> (ø)`
.../io/gitlab/arturbosch/detekt/cli/runners/Runner.kt	`86.95% <0%> (-1.94%)`	`10% <0%> (ø)`
...gitlab/arturbosch/detekt/generator/out/Markdown.kt	`20.68% <0%> (-0.74%)`	`4% <0%> (ø)`
...rturbosch/detekt/rules/empty/EmptyFunctionBlock.kt	`92.3% <0%> (ø)`	`12% <0%> (ø)`	⬇️
...bosch/detekt/generator/collection/Configuration.kt	`100% <0%> (ø)`	`5% <0%> (+1%)`	⬆️
...h/detekt/rules/naming/MemberNameEqualsClassName.kt	`88.46% <0%> (ø)`	`14% <0%> (ø)`	⬇️
... and 9 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 01b6fe1...970165d. Read the comment docs.

schalkms

Thanks for contributing this rule! I would make the rule more conservative as mentioned in my comment.

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/concurrency/GlobalScopeUsage.kt

detekt-cli/src/main/resources/default-detekt-config.yml

xouabita · 2019-12-16T15:52:20Z

I have changed the rule to only report GlobalScope.async and GlobalScope.launch. It also does not need type resolution anymore.
Let me know what do you think 🙂

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/GlobalScopeUsage.kt

arturbosch · 2019-12-28T21:11:35Z

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/providers/CoroutinesProvider.kt

+import io.gitlab.arturbosch.detekt.api.RuleSetProvider
+import io.gitlab.arturbosch.detekt.rules.coroutines.GlobalScopeUsage
+
+/** The coroutines rule set analyzes code for potential coroutines problems.


Please move the documentation start to the next line.

arturbosch · 2019-12-28T21:11:47Z

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/providers/CoroutinesProvider.kt

+
+/** The coroutines rule set analyzes code for potential coroutines problems.
+ *
+ * @active since v1.3.0


Please update this to 1.4.0

arturbosch · 2019-12-28T21:12:15Z

...t-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/GlobalScopeUsageSpec.kt

+                    bar(GlobalScope)
+                }
+            """
+            assertThat(subject.compileAndLint(code)).hasSize(0)


Please use isEmpty() instead of hasSize(0).

arturbosch

Looks good to go for 1.4.0. Thanks for the rule.
Please see my comments.

arturbosch · 2019-12-28T21:12:20Z

...t-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/GlobalScopeUsageSpec.kt

+                    bar(scope)
+                }
+            """
+            assertThat(subject.compileAndLint(code)).hasSize(0)


Please use isEmpty() instead of hasSize(0).

xouabita · 2020-01-02T11:19:25Z

detekt-gradle-plugin/build.gradle.kts

@@ -133,8 +133,8 @@ artifacts {
 }

 detekt {
+    buildUponDefaultConfig = true


This is needed because :detekt-gradle-plugin:detekt was failing.

* What went wrong: Execution failed for task ':detekt-gradle-plugin:detekt'. > Invalid detekt configuration file detected.

Property 'coroutines' is misspelled or does not exist. io.gitlab.arturbosch.detekt.cli.InvalidConfig: Run failed with 1 invalid config property.

Blocked by #2217

If this issue is resolved, this line should be removed.

Submitted #2227 before I saw this - I think @xouabita has the correct solution, but we can discuss further in my PR. I don't see any other way to fix this while also enforcing validation of the config file.

xouabita · 2020-01-07T17:38:44Z

I have rebased with #2227

schalkms · 2020-01-07T18:41:48Z

Cool thanks! I’m waiting for @3flex final review to merge this PR.

3flex

Just two comments, @schalkms let me know your thoughts, but I'm otherwise happy with this. Hopefully it will inspire some more rules focused on correct usage of coroutines!

Original rule was called GlobalScopeUsage but scope is now limited to just flagging use of async and launch on GlobalScope. Is there a better name for the rule? Maybe GlobalCoroutine since the intention is to flag new coroutines with global scope?
GlobalScope.broadcast and GlobalScope.produce also create new coroutines with global scope. I have no experience with these and the docs don't specifically discourage their use, any opinions on whether they should also be flagged? Easy to add later though but should do it now if anyone knows the answer.

arturbosch · 2020-01-08T06:55:19Z

How about GlobalCoroutineUsage then?
I guess we can limit this for async and launch for now and when users start use this rule they will report if these two fucntions are also discouraged to use :)

schalkms · 2020-01-08T16:32:20Z

GlobalCoroutineUsage

+1

I guess we can limit this for async and launch for now and when users start use this rule they will report if these two fucntions are also discouraged to use :)

Absolutely! This is a very good start! I’d really like to merge this. Extension can be made in the future if we see a need.

xouabita · 2020-01-09T10:20:54Z

will rename to GlobalCoroutineUsage then

xouabita marked this pull request as ready for review December 12, 2019 11:27

schalkms reviewed Dec 12, 2019

View reviewed changes

3flex reviewed Dec 13, 2019

View reviewed changes

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/concurrency/GlobalScopeUsage.kt Outdated Show resolved Hide resolved

detekt-cli/src/main/resources/default-detekt-config.yml Outdated Show resolved Hide resolved

xouabita changed the title ~~Add GlobalScopeUsage rule + concurrency ruleset~~ Add GlobalScopeUsage rule + coroutines ruleset Dec 16, 2019

xouabita requested review from 3flex and schalkms December 16, 2019 15:49

schalkms approved these changes Dec 16, 2019

View reviewed changes

thuytrinh reviewed Dec 17, 2019

View reviewed changes

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/GlobalScopeUsage.kt Outdated Show resolved Hide resolved

BoxResin approved these changes Dec 20, 2019

View reviewed changes

arturbosch reviewed Dec 28, 2019

View reviewed changes

arturbosch approved these changes Dec 28, 2019

View reviewed changes

arturbosch added this to the 1.4.0 milestone Dec 28, 2019

xouabita commented Jan 2, 2020

View reviewed changes

schalkms added blocked and removed blocked labels Jan 2, 2020

xouabita added 11 commits January 7, 2020 15:13

Add GlobalScopeUsage rule + concurrency ruleset

d515fb8

performance -> concurrency for ruleSetId in ConcurrencyProvider

9ee9124

fix wildcard imports

8ed457d

concurrency -> coroutines

1b376cd

remove concurrency documentation

d56f918

Move companion object at the bottom of the rule

ec4d7c9

Better doc

d81ac5b

generate documentation

388a77f

Only report GlobalScope.async and GlobalScope.launch

013803e

Remove coroutines dependency from gradle

e81eb12

fix RuleProviderTest

0c1d94e

xouabita added 3 commits January 7, 2020 15:13

Update compliant / non-compliant code

d38cafc

Correct coroutines provider comments

1f03d29

hasSize(0) -> isEmpty

5073bda

3flex reviewed Jan 8, 2020

View reviewed changes

GlobalScopeUsage -> GlobalCoroutineUsage

970165d

xouabita changed the title ~~Add GlobalScopeUsage rule + coroutines ruleset~~ Add GlobalCoroutineUsage rule + coroutines ruleset Jan 9, 2020

3flex approved these changes Jan 9, 2020

View reviewed changes

3flex merged commit 483131d into detekt:master Jan 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GlobalCoroutineUsage rule + coroutines ruleset #2174

Add GlobalCoroutineUsage rule + coroutines ruleset #2174

xouabita commented Dec 12, 2019 •

edited by 3flex

Loading

codecov-io commented Dec 12, 2019 •

edited

Loading

schalkms left a comment

xouabita commented Dec 16, 2019

arturbosch Dec 28, 2019

arturbosch Dec 28, 2019

arturbosch Dec 28, 2019

arturbosch left a comment

arturbosch Dec 28, 2019

xouabita Jan 2, 2020

schalkms Jan 2, 2020

schalkms Jan 2, 2020

3flex Jan 7, 2020

xouabita Jan 7, 2020

xouabita commented Jan 7, 2020

schalkms commented Jan 7, 2020 •

edited

Loading

3flex left a comment

arturbosch commented Jan 8, 2020

schalkms commented Jan 8, 2020

xouabita commented Jan 9, 2020

Add GlobalCoroutineUsage rule + coroutines ruleset #2174

Add GlobalCoroutineUsage rule + coroutines ruleset #2174

Conversation

xouabita commented Dec 12, 2019 • edited by 3flex Loading

codecov-io commented Dec 12, 2019 • edited Loading

Codecov Report

schalkms left a comment

Choose a reason for hiding this comment

xouabita commented Dec 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturbosch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xouabita commented Jan 7, 2020

schalkms commented Jan 7, 2020 • edited Loading

3flex left a comment

Choose a reason for hiding this comment

arturbosch commented Jan 8, 2020

schalkms commented Jan 8, 2020

xouabita commented Jan 9, 2020

xouabita commented Dec 12, 2019 •

edited by 3flex

Loading

codecov-io commented Dec 12, 2019 •

edited

Loading

schalkms commented Jan 7, 2020 •

edited

Loading