CC: Tweaked

CC: Tweaked

42M Downloads

Running static analysis from Gradle

SquidDev opened this issue ยท 3 comments

commented

As #235 has shown, there's definitely some still some code-style issues within CC. While we do have IntelliJ formatting/inspection configs in the root directory, this doesn't work very well for people using other IDEs (and is somewhat harder to do as part of the build process).

It would be pretty nice if we could setup decent configurations for CheckStyle and possibly SpotBugs, so we can run them from Gradle. I know the latter can be rather aggressive, so we may want to start with the settings turned down quite low and slowly ramp them up.

I'd also quite like to get some static analysis running on the Lua side, even if just to catch undeclared variables. I've been building some tooling as part of #133 which should help with this, but it may be worth trying to get LuaCheck running in the meantime.

commented

One other thing possibly worth doing is using CheckStyle's ImportControl to enforce some separation between CC's various components:

  • dan200.computercraft.api shouldn't depend on any other CC package.
  • dan200.computercraft.core shouldn't use Minecraft classes (aside from Terminal.java, due to serialisation).
  • Possibly also block some external libraries from being referenced at all (scala.*, com.sun.*, etc...).

It may be worth seeing if we want to split up dan200.computercraft.util into "core" and "non-core" packages. Or alternatively move some of the Minecraft-specific things into shared. For instance, TickScheduler should probably be moved to make it clear that it's part of the core infrastructure, rather than just a helper class.

commented

So I added CheckStyle and the license plugin last week, and they caught a couple of issues (not actual bugs, but definite problems).

I've had a bit of a fiddle with SpotBugs, and found a couple of possible bugs in our code - I need to play around a little more, fix those and hopefully enable it by default.

A couple of other things it may be worth trying instead:

  • Error Prone. Like SpotBugs, probably more aggressive than I'd like, but still...
  • Checker Framework. This is something I've wanted to do for a while. Some things I'd like to see if we can enforce with this (or with custom checkers):
    • Enforce purity on writeDescription methods (see #245).
    • Some sort of tainting for server-thread only methods - can we ensure that peripheral methods are thread safe (likewise, client/server methods). This feels like a modification of the "Gui Effect checker".
commented

I'd love to do more on this, but I think right now the cost/benefit of doing so isn't worthwhile.