feat(kotlin): Adding detekt static code analysis for Kotlin modules #450

robzienert · 2019-12-18T22:07:57Z

Playing around with the idea of adding SCA to Kotlin, Java.

I tried to get errorprone/nullaway to work for the Java source sets, but it was encountering fatal errors across a ton of the modules, so I gave up. Kotlin already has a ton of rules in IntelliJ, but not everyone uses IntelliJ (allegedly).

Nothing terribly interesting was detected for Kotlin, but there were a ton of valid things from errorprone where it actually worked.

Thoughts?

robfletcher

I'm open to trying it. I'm not fond of SCA generally, but if it's not requiring me to suppress too many valid exceptions it might provide some value.

robzienert · 2019-12-18T22:13:52Z

Agree - I think the only way for SCA to be successful is that we tune the warnings/errors to what we want as a community. For example, I disabled the performance->SpreadOperator warning, because it's awfully convenient, even if it comes at a perf disadvantage. If we had the detekt.yml config in spinnaker-gradle-project, we could keep everything synchronized so there's nothing unexpected project-to-project.

robfletcher · 2019-12-18T22:14:53Z

I think the biggest issue I tend to have is with arbitrarily bounded checks (e.g. this method is too long).

plumpy · 2019-12-18T22:22:29Z

Yeah, +1000 to error-prone if we can get it working. We have it running everywhere internally (tuned with the errors we care about, as you suggest, and with a ton of custom rules as well) and it's immensely helpful. I think it won't work most places in Spinnaker since we have to use groovy to compile, but I might be wrong.

robzienert · 2019-12-18T22:30:26Z

@plumpy I think you're correct on the Groovy thing, but it looks like the errorprone gradle plugin can be configured to use the JDK9 compiler for Java source sets (its the workaround for supporting JDK8, don't see why this wouldn't also apply for the Groovy compiler?)

There were a lot of exceptions that fail the build. It doesn't instill a lot of confidence - if you could venture a guess, would you say this may be rules that Google doesn't actively use?

plumpy · 2019-12-18T22:39:01Z

Maybe? What sorts of things? Certainly we don't use @Nullable checking by default, for example.

(But have you seen our codebase? It's full of things that probably should have failed the build 😁)

Google has tons of tools and processes for doing large scale changes across entire monorepo, so you can clean up an entire class of errors before enabling the check. Could you just set everything to disabled and just turn on a handful of warnings (not errors) to start? Might not be worth the effort if that's all we're getting out of it, though...

plumpy · 2019-12-18T22:45:56Z

i.e. for the check about how you shouldn't ever use methods that don't specify a charset (like String#getBytes()) we'd have our tool migrate every caller of that method before enabling that check. Presumably we'd have to do the same thing in Spinnaker, but by hand.

robzienert · 2019-12-18T22:51:49Z

Rerunning from the sca-java branch on kork:

/Users/rzienert/netflix/spinnaker/workspace/kork/kork-exceptions/src/main/java/com/netflix/spinnaker/kork/exceptions/ExceptionSummary.java:117: error: An unhandled exception was thrown by the Error Prone static analysis plugin.
    private String message;
                   ^
     Please report this at https://github.com/google/error-prone/issues/new and include the following:
  
     error-prone version: 2.3.4
     BugPattern: ReferenceEquality
     Stack Trace:
     java.lang.IllegalArgumentException: invalid replacement: [3963, -1) (Objects.equals(null, null))
        at com.google.common.base.Preconditions.checkArgument(Preconditions.java:459)
...

As you can see from the code there's nothing particularly interesting going on there.

Perhaps disabling everything and building the rule sets up by hand would be the only way forward. Certainly, nullaway is probably not something we want to enable from the start because, omg, I died inside watching the flurry of warnings go by. It looks like it'd be handy to have, though.

plumpy · 2019-12-18T22:54:21Z

Oh, I see what you mean. Not exceptions indicating a error-prone check failed, but actually a bug in error-prone. Yeah, that's not great 😒

robzienert · 2019-12-18T23:29:20Z

Here's the Java PR: #451

robzienert · 2019-12-19T06:09:53Z

Will merge with more approvals. Anyone against?

ezimanyi · 2019-12-19T15:26:11Z

Looks like some of the crashes we're running into are the same ones noted in google/error-prone#1195, though that PR has been open for a year.

ezimanyi

Definitely agree with adding static code analysis, and detekt seems reasonable.

Only minor comment is the number of things in the .detekt.yml file...are there defaults we could fall back on and only specify the differences so the config file isn't quite so long?

plumpy · 2019-12-19T16:28:29Z

...kotlin/com/netflix/spinnaker/kork/plugins/config/SpringEnvironmentExtensionConfigResolver.kt

@@ -91,7 +91,7 @@ class SpringEnvironmentExtensionConfigResolver(
      throw IntegrationException("Failed reading extension config: Input appears invalid", pe)
    } catch (me: JsonMappingException) {
      throw IntegrationException("Failed reading extension config: Could not map provided config to expected shape", me)
-    } catch (e: Exception) {
+    } catch (@Suppress("TooGenericExceptionCaught") e: Exception) {


FYI our general advice internally is to catch RuntimeException instead of Exception, which means if someone (ObjectMapper#readValue()) changes an API and adds a checked exception that you might actually care about, you'll get a compile time error and be forced to deal with it. Which might mean just adding it as NewCheckedException|RuntimeException to your catch block, but still means it won't go silently unconsidered...

That's not always what you want, but in this case it feels like you might actually want to do that?

(Looks like RuntimeException still triggers TooGenericExceptionCaught, though, so you'd still have to suppress, so 🤷‍♂️)

I'll address this regardless.

dreynaud · 2019-12-19T18:22:08Z

.detekt.yml

+    active: false
+  ThrowsCount:
+    active: true
+    max: 2


is this a check we care about? Looks like no if we're going to manually suppress it in a bunch of places

Probably not, we can disable 👍

robzienert · 2019-12-20T02:20:28Z

Only minor comment is the number of things in the .detekt.yml file...are there defaults we could fall back on and only specify the differences so the config file isn't quite so long?

I copied the default and changed things. There is a config for inheriting defaults so things could be much slimmer. My thought was it'd be harder to know if something slipped in as a new rule... but perhaps auto-inheriting new rules isn't such a bad thing?

ezimanyi · 2019-12-20T15:58:56Z

Only minor comment is the number of things in the .detekt.yml file...are there defaults we could fall back on and only specify the differences so the config file isn't quite so long?

I copied the default and changed things. There is a config for inheriting defaults so things could be much slimmer. My thought was it'd be harder to know if something slipped in as a new rule... but perhaps auto-inheriting new rules isn't such a bad thing?

That's fair...my main concern was just that it's easier to read the config and understand where we deviated from defaults if we're not explicitly setting everything. But maybe I'm placing too much trust in defaults if I was only planning to read where we've deviated from them 🤷‍♂️ . Either way is fine with me, super excited to have code analysis! 👍

robzienert · 2020-01-15T19:14:23Z

Merging. I've altered the config such that it automatically inherits from detekt's defaults and the config we store in the repo is now just customizations. Much bester.

…pinnaker#450) * feat(kotlin): Adding detekt static code analysis * chore(kotlin): Apply detekt fixes

robzienert requested review from robfletcher, cfieber, plumpy, ajordens, ezimanyi and ethanfrogers December 18, 2019 22:07

robfletcher approved these changes Dec 18, 2019

View reviewed changes

robzienert requested review from marchello2000, dreynaud and jonsie December 18, 2019 22:31

ezimanyi approved these changes Dec 19, 2019

View reviewed changes

plumpy approved these changes Dec 19, 2019

View reviewed changes

dreynaud reviewed Dec 19, 2019

View reviewed changes

dreynaud approved these changes Dec 19, 2019

View reviewed changes

robzienert force-pushed the sca-kt branch from 7b4098b to 89f7f74 Compare January 13, 2020 21:24

robzienert added the ready to merge Approved and ready for merge label Jan 13, 2020

feat(kotlin): Adding detekt static code analysis

4c63f32

chore(kotlin): Apply detekt fixes

68e0ab2

robzienert force-pushed the sca-kt branch from 89f7f74 to 68e0ab2 Compare January 15, 2020 19:12

mergify bot merged commit 89ef5be into spinnaker:master Jan 15, 2020

mergify bot added the auto merged label Jan 15, 2020

jonsie pushed a commit to jonsie/kork that referenced this pull request Jan 16, 2020

feat(kotlin): Adding detekt static code analysis for Kotlin modules (s…

4252e34

…pinnaker#450) * feat(kotlin): Adding detekt static code analysis * chore(kotlin): Apply detekt fixes

claymccoy pushed a commit to claymccoy/kork that referenced this pull request Mar 3, 2020

chore(dependencies): Autobump spinnaker-dependencies (spinnaker#450)

110354b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(kotlin): Adding detekt static code analysis for Kotlin modules #450

feat(kotlin): Adding detekt static code analysis for Kotlin modules #450

robzienert commented Dec 18, 2019 •

edited

Loading

robfletcher left a comment

robzienert commented Dec 18, 2019

robfletcher commented Dec 18, 2019

plumpy commented Dec 18, 2019

robzienert commented Dec 18, 2019 •

edited

Loading

plumpy commented Dec 18, 2019 •

edited

Loading

plumpy commented Dec 18, 2019

robzienert commented Dec 18, 2019

plumpy commented Dec 18, 2019

robzienert commented Dec 18, 2019

robzienert commented Dec 19, 2019 •

edited

Loading

ezimanyi commented Dec 19, 2019

ezimanyi left a comment

plumpy Dec 19, 2019

robzienert Dec 20, 2019

dreynaud Dec 19, 2019

robzienert Dec 20, 2019

robzienert commented Dec 20, 2019

ezimanyi commented Dec 20, 2019

robzienert commented Jan 15, 2020

feat(kotlin): Adding detekt static code analysis for Kotlin modules #450

feat(kotlin): Adding detekt static code analysis for Kotlin modules #450

Conversation

robzienert commented Dec 18, 2019 • edited Loading

robfletcher left a comment

Choose a reason for hiding this comment

robzienert commented Dec 18, 2019

robfletcher commented Dec 18, 2019

plumpy commented Dec 18, 2019

robzienert commented Dec 18, 2019 • edited Loading

plumpy commented Dec 18, 2019 • edited Loading

plumpy commented Dec 18, 2019

robzienert commented Dec 18, 2019

plumpy commented Dec 18, 2019

robzienert commented Dec 18, 2019

robzienert commented Dec 19, 2019 • edited Loading

ezimanyi commented Dec 19, 2019

ezimanyi left a comment

Choose a reason for hiding this comment

plumpy Dec 19, 2019

Choose a reason for hiding this comment

robzienert Dec 20, 2019

Choose a reason for hiding this comment

dreynaud Dec 19, 2019

Choose a reason for hiding this comment

robzienert Dec 20, 2019

Choose a reason for hiding this comment

robzienert commented Dec 20, 2019

ezimanyi commented Dec 20, 2019

robzienert commented Jan 15, 2020

robzienert commented Dec 18, 2019 •

edited

Loading

robzienert commented Dec 18, 2019 •

edited

Loading

plumpy commented Dec 18, 2019 •

edited

Loading

robzienert commented Dec 19, 2019 •

edited

Loading