Resource IntelliJ Inspections for Spigot/Bukkit

Discussion in 'Spigot Plugin Development' started by Stef, May 20, 2017.

  1. For the past three weeks or so, I've been working on a small side project. Instead of making something directly for Minecraft I decided to make something for the development of Minecraft plugins instead. So this is the result.

    This IntelliJ (no Eclipse, Netbeans etc.) plugin adds more inspections to the tons of already built-in inspections IntelliJ already has, specifically for plugins made with Spigot/Bukkit. Some of them highlight mistakes made if you're new to developing plugins, and should therefore hopefully lower the barrier of getting into making plugins, and a few are just things you easily forget/do wrong.

    Skip this paragraph if you don't want to read about the development process
    I've looked at YouTube tutorials as well as the Spigot Plugin Development section for mistakes which you can easily make. I also checked the Wiki for some stuff which you might forget/do wrong. This is my first IntelliJ plugin and it was pretty difficult. With a pretty short section on how to create inspections as documentation, most of this plugin was created by just looking at the source code from other plugins which I've never used. I wouldn't be surprised if the code isn't that great, but that's part of the learning process. I did put my code open for review (on a different site), but already six days have passed and nobody has replied yet, therefore I decided to leave it for now and continue what I already was doing the past weeks.

    At this point, the plugin has eighteen inspections. The inspections are purely for Java, so stuff like the plugin.yml aren't covered (just because I couldn't find a single thing about working with YAML files). If some of you have good suggestions for some inspections let me know.
    Here's a reference list of all inspections, with their default level type (warning/error) and their fixes (if any):
    This inspection reports magic values used (like ┬ža) instead of the ChatColor enum (ChatColor.GREEN) inside string literals. It's better to use the ChatColors as they don't require characters that can mess up depending on file encodings and will still work even if the magic values change.

    Default inspection level: Warning

    Quick fixes:
    This changes the magic values to the ChatColor variant and adds an import if needed. It also does the string concatenation if that's needed as well.

    This:
    Code (Text):
    String text = "┬žaWelcome to my server!";
    will become this:
    Code (Text):
    String text = ChatColor.GREEN + "Welcome to my server!";
    This inspection reports the creation of a class which extends JavaPlugin. Instantiating a class which extends JavaPlugin will cause an error as the class is already initialized by Bukkit/Spigot itself.

    Default inspection level: Error
    Reports the use of Logger.getLogger("Minecraft"). 'Stealing' Minecraft's logger is discouraged and using getLogger() is better.

    Default inspection level: Warning

    Quick fixes:
    This changes the Logger.getLogger("Minecraft") statement to getLogger() if you're inside a class extending JavaPlugin. If this isn't the case this fix won't be available.

    This:
    Code (Text):
    Logger logger = Logger.getLogger("Minecraft");
    will become this:
    Code (Text):
    Logger logger = getLogger();
    This inspection reports classes which extend some kind of event (e.g. PlayerEvent) and are missing a getHandlerList method. This method is mandatory and the event won't work if this isn't implemented.

    Default inspection level: Error
    This inspection reports methods which appear to be event methods (and are inside a class implementing Listener), but don't have the @EventHandler annotation. The EventHandler annotation is mandatory for such methods and forgetting it is likely a mistake.

    Default inspection level: Error

    Quick fixes:
    This fix adds the EventHandler annotation to the method.

    This:
    Code (Text):
    public void onBlockBreak(BlockBreakEvent event) {}
    will become this:
    Code (Text):
    @EventHandler
    public void onBlockBreak(BlockBreakEvent event) {}
    This inspection reports the use of EventPriority.MONITOR. This level should only be used for listening to the output for events and not for any real production code as it is too high.

    Default inspection level: Warning
    This inspection reports the use of unnecessary chatcolors at the start of a string literal inside a method.

    Default inspection level: Warning

    Quick fixes:
    Removes the unnecessary chat color.
    This:
    Code (Text):
    player.sendMessage(ChatColor.WHITE + "Hi there, player");
    will become this:
    Code (Text):
    player.sendMessage("Hi there, player");
    Reports methods which have an EventHandler annotation, but the class doesn't implement Listener, making them useless.

    Default inspection level: Warning

    Quick fixes:
    Makes the class implement the Listener interface.

    This:
    Code (Text):
    class MyClass {
      @EventHandler
      public void onBlockBreak(BlockBreakEvent event) {}
    }
    will become this:
    Code (Text):
    class MyClass implements Listener {
      @EventHandler
      public void onBlockBreak(BlockBreakEvent event) {}
    }
    Removes the unnecessary EventHandler annotation from the method.

    This:
    Code (Text):
    class MyClass {
      @EventHandler
      public void onBlockBreak(BlockBreakEvent event) {}
    }
    will become this:
    Code (Text):
    class MyClass {
      public void onBlockBreak(BlockBreakEvent event) {}
    }
    Reports calls like this getCommand("mycommand").setExecutor(this) inside a class extending JavaPlugin. Commands default to the JavaPlugin class and it isn't needed to assign them again.

    Default inspection level: Warning
    This inspection reports classes which implement Listener, but don't contain any method with an EventHandler annotation, which makes implementing Listener useless.

    Default inspection level: Warning

    Removes the import Listener statement.

    This:
    Code (Text):
    class MyClass implements Listener {}
    will become this:
    Code (Text):
    class MyClass {}
    This inspection reports classes which implement Cancellable, but don't extend any Event type, making the Cancellable useless.

    Default inspection level: Warning

    Quick fixes:
    Removes the unnecessaryimplements Cancellable.

    This:
    Code (Text):
    class MyClass implements Cancellable {}
    will become this:
    Code (Text):
    class MyClass {}
    Makes the class extend the base Event class.

    This:
    Code (Text):
    class MyClass implements Cancellable {}
    will become this:
    Code (Text):
    class MyClass extends Event implements Cancellable {}
    This inspection reports Note (the sounds) constructors with an octave of 2, but a note that differs from F#. The only note in the second octave is F#, so if it's different it won't work.

    Default inspection level: Error
    This inspection reports the use of Java 7 or lower. Minecraft uses Java 8 since 1.12, so there's no reason to stay behind.
    Only available in Analyze | Inspect Code

    Default inspection level: Warning
    This inspection reports checking the command executed by utilizing the commandLabel instead of Command#getName. Command#getName is reocmmended, so aliases work correctly as well.

    Default inspection level: Warning

    Quick fix:
    Changes commandLabel to command#getName

    This:
    Code (Text):
    @Override
    public boolean onCommand(CommandSender sender, Command cmd, String label, String args[]) {
      if (label.equalsIgnoreCase("myCommand")) {}
    }
    will become this:
    Code (Text):
    @Override
    public boolean onCommand(CommandSender sender, Command cmd, String label, String args[]) {
      if (cmd.getName().equalsIgnoreCase("myCommand")) {}
    }
    This inspection reports metod calls to setCustomName on a player object. This is useless as this method doesn't change anything for players.

    Default inspection level: Warning
    This inspection reports Location#add and Location#subtract calls with parameters which are negative numbers/zeroes. To increase readability invert this method and inverts all numbers.

    Default inspection level: Warning
    Quick fix:
    This inverts the method call and all the parameters.

    This:
    Code (Text):
    location.add(0, -1, 0);
    will become this:
    Code (Text):
    location.subtract(0, 1, 0);
    Reports methods with an EventHandler annotation which aren't event methods due to an incorrect amount of parameters or incorrect parameter types.

    Default inspection level: Warning

    Quick fix:
    Removes the EventHandler annotation.

    This:
    Code (Text):
    @EventHandler
    public void someMethod() {}
    will become this:
    Code (Text):
    public void someMethod() {}
    Reports World#setTicksPerAnimalSpawns argument being zero, to disable animal spawns you should use World#setSpanFlags instead.

    Default inspection level: Warning

    Quick fix:
    Changes the currently used method to the correct one and applies the correct arguments to it.

    This:
    Code (Text):
    world.setTicksPerAnimalSpawns(0)
    will become this:
    Code (Text):
    world.setSpawnFlags(world.getAllowMonsters(), false)
    You can enable/disable all these inspections (entirely, or in different scopes) and change the inspection levels if you want to. You can do so by going to File > Settings, and then Editor > Inspections and then selecting Bukkit/Spigot and opening the menu. It works the same as with other inspections.

    I haven't published the plugin on IntelliJ's built-in plugin stuff (in case it turns out bad), but you can install it manually. I've attached a file to this post where you can download the plugin. Then go to C:\Users\$user$\$ideafolder$\config\plugins and putting it there. Then close IntelliJ (if it was open) and start it.

    Well, that's about it, let me know what you think of it. Also, I probably will make this open source, but it's currently pretty late in the evening for me, so I'll probably do that tomorrow. GitHub link: https://github.com/stefvanschie/SpigotIntelliJPlugin

    Oh, on a last note: I tested this plugin on my own project (instead of my testing thing) for about a week and it seemed to work fine, but I can't 100% guarantee that it won't make errors with quick fixes, so please double-check the code it generated before actually using it.

    TL;DR: Even more inspections for IntelliJ (cause there aren't enough already...)
     

    Attached Files:

    #1 Stef, May 20, 2017
    Last edited: May 27, 2017
    • Like Like x 15
    • Winner Winner x 2
    • Creative Creative x 2
    • Useful Useful x 1
  2. Mas

    Mas

    Nice job!

    However, about using avoiding Logger.getLogger("Minecraft"), that's exactly what Bukkit.getLogger() returns anyway IIRC (It returns CraftServer's logger field which is set to Logger.getLogger("Minecraft") by default. You should use JavaPlugin#getLogger() instead).
     
    • Agree Agree x 1
  3. Thanks for pointing that out, fixed this. Uploaded the new jar to the main post.
     
    • Like Like x 1
  4. Static abusing? Might be tricky to detect.
     
    • Funny Funny x 1
  5. That's incredibly hard to detect and I feel like that's more Java-related and not necessarily something from Spigot itself. IntelliJ itself also has some inspections regarding the static keyword, which should be sufficient.
     
    • Agree Agree x 1
  6. Reaally cool!
    I suggest adding a check for the commands, the Command#getName and using an element of the args without checking the lenght.
     
    • Agree Agree x 1
  7. You should make a check for registering an event for a class that doesn't implement Listener and not registering the event when implementing Listener.
     
    • Funny Funny x 1
  8. IntelliJ automatically warns you for that.
     
    • Agree Agree x 1
  9. Thanks for the suggestions. There are a bit harder than I'm used to doing, but it'll be a fun challenge.

    For now, I added a new global inspection which reports the use of Java 7 or less (as Minecraft 1.12 will use Java 8, there is no reason to stay behind). Added the new file to the main post.
     
  10. Definitely knew this :cool:
     
  11. Good job creating these inspections, looks like a good way to catch some nasty errors/problems.

    There is one inspection I do not fully agree with, and that is the use of EventPriority.MONITOR. This priority is totally fine, as long as you do not use event.setCancelled(). For example for logging all chat messages to a file, the MONITOR priority is perfect, because the message cannot be cancelled anymore and we will never want to cancel the event ourselves. So you could change the inspection to only warn if you are also using event.setCancelled(), but I'm not sure how easy that is to detect.

    The 'white ChatColor at the start of a string'-inspection seems nice in theory, but there are some corner cases that you might have missed. For chat the default is indeed white, but for signs the default is black, so there it does make sense to use the white ChatColor at the start of a string. The code might also combine multiple strings to build a message, in which case the white ChatColor could also appear at the start of the string at first, but some other message might get placed before it.

    I would like to see this in the plugin repository of Intellij IDEA, because that would make installing easier. I think you can also easily update the plugin on there, so having some bugs in this version is not that big of a problem.

    If I have some time later I'll probably check your code, although I don't have experience writing inspections myself so I'm not sure if I can give much feedback about that.
     
    • Agree Agree x 2
  12. I agree with the EventPriority.MONITOR. According to the javadocs you shouldn't make any modifications to the event when using that priority (source) which also includes changing specific properties to the event itself (e.g. FoodLevelChangeEvent#setFoodLevel). I decided to just add it in and then add checks to see if you're modifying anything in the future. I'm not really certain how to check this (without checking the entire spigot methods for modifications), but I can make a start.

    Didn't really think about signs being black by default. I can add some checks to see in which method you use the string, although this takes a bit more effort. Could you provide an example for a string which is combined and needs ChatColor.WHITE / ChatColor.RESET to turn it to the default values?

    I might upload the plugin to IntelliJ's plugin repository when there are a bit more inspections.
     
  13. I changed the redundat chat color inspection, so now it detects in which method you're using the string and changes its behavior depending on this. So CommandSender#sendMessage will have ChatColor#WHITE and ChatColor#RESET marked as redundant, but Sign#setLine will have ChatColor#BLACK and ChatColor#RESET marked as redundant. This detection won't apply to normal strings anymore (e.g. not inside a method). Thanks for pointing this out @NLThijs48.
    Uploaded the new file to the main post.
     
  14. @Stef MONITOR priority indeed means you are not allowed to changed anything in an event anymore, but cancelling it is the most obvious one.

    For my own plugins I never use sendMessage() directly, I always use my messaging library which provides translations, so the messages themselves are in a YAML file, but the theoretical problem I described is fixed now, nice.

    I used the plugin on AreaShop, it actually choked on this file with an exception:
    Code (Text):
    In file: WorldEditHandler5.java
    java.lang.NullPointerException
        at com.gmail.stefvanschiedev.spigotintellijplugin.inspections.RedundantChatColorUsage.classImplements(RedundantChatColorUsage.java:155)
        at com.gmail.stefvanschiedev.spigotintellijplugin.inspections.RedundantChatColorUsage.matching(RedundantChatColorUsage.java:94)
        at com.gmail.stefvanschiedev.spigotintellijplugin.inspections.RedundantChatColorUsage.getAllProblemDescriptors(RedundantChatColorUsage.java:62)
        at com.gmail.stefvanschiedev.spigotintellijplugin.inspections.RedundantChatColorUsage.checkClass(RedundantChatColorUsage.java:55)
        at com.intellij.codeInspection.AbstractBaseJavaLocalInspectionTool$1.visitClass(AbstractBaseJavaLocalInspectionTool.java:78)
        at com.intellij.psi.impl.source.PsiClassImpl.accept(PsiClassImpl.java:469)
        at com.intellij.codeInspection.InspectionEngine.acceptElements(InspectionEngine.java:81)
        at com.intellij.codeInsight.daemon.impl.LocalInspectionsPass.a(LocalInspectionsPass.java:305)
        at com.intellij.concurrency.ApplierCompleter.c(ApplierCompleter.java:133)
        at com.intellij.openapi.application.impl.ApplicationImpl.tryRunReadAction(ApplicationImpl.java:1147)
        at com.intellij.concurrency.ApplierCompleter.b(ApplierCompleter.java:105)
        at com.intellij.openapi.progress.impl.CoreProgressManager.a(CoreProgressManager.java:556)
        at com.intellij.openapi.progress.impl.CoreProgressManager.executeProcessUnderProgress(CoreProgressManager.java:501)
        at com.intellij.openapi.progress.impl.ProgressManagerImpl.executeProcessUnderProgress(ProgressManagerImpl.java:66)
        at com.intellij.concurrency.ApplierCompleter.a(ApplierCompleter.java:116)
        at com.intellij.concurrency.ApplierCompleter.compute(ApplierCompleter.java:99)
        at java.util.concurrent.CountedCompleter.exec(CountedCompleter.java:731)
        at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
        at java.util.concurrent.ForkJoinPool$WorkQueue.pollAndExecCC(ForkJoinPool.java:1190)
        at java.util.concurrent.ForkJoinPool.helpComplete(ForkJoinPool.java:1879)
        at java.util.concurrent.ForkJoinPool.awaitJoin(ForkJoinPool.java:2045)
        at java.util.concurrent.ForkJoinTask.doJoin(ForkJoinTask.java:390)
        at java.util.concurrent.ForkJoinTask.join(ForkJoinTask.java:719)
        at java.util.concurrent.ForkJoinPool.invoke(ForkJoinPool.java:2616)
        at com.intellij.concurrency.JobLauncherImpl.invokeConcurrentlyUnderProgress(JobLauncherImpl.java:63)
        at com.intellij.concurrency.JobLauncher.invokeConcurrentlyUnderProgress(JobLauncher.java:57)
        at com.intellij.codeInsight.daemon.impl.LocalInspectionsPass.a(LocalInspectionsPass.java:316)
        at com.intellij.codeInsight.daemon.impl.LocalInspectionsPass.a(LocalInspectionsPass.java:226)
        at com.intellij.codeInsight.daemon.impl.LocalInspectionsPass.doInspectInBatch(LocalInspectionsPass.java:147)
        at com.intellij.codeInspection.ex.GlobalInspectionContextImpl.a(GlobalInspectionContextImpl.java:512)
        at com.intellij.codeInspection.ex.GlobalInspectionContextImpl.a(GlobalInspectionContextImpl.java:424)
        at com.intellij.openapi.application.impl.ApplicationImpl.tryRunReadAction(ApplicationImpl.java:1153)
        at com.intellij.codeInspection.ex.GlobalInspectionContextImpl.a(GlobalInspectionContextImpl.java:419)
        at com.intellij.concurrency.JobLauncherImpl$1MyTask.a(JobLauncherImpl.java:295)
        at com.intellij.openapi.progress.impl.CoreProgressManager.a(CoreProgressManager.java:556)
        at com.intellij.openapi.progress.impl.CoreProgressManager.executeProcessUnderProgress(CoreProgressManager.java:501)
        at com.intellij.openapi.progress.impl.ProgressManagerImpl.executeProcessUnderProgress(ProgressManagerImpl.java:66)
        at com.intellij.concurrency.JobLauncherImpl$1MyTask.call(JobLauncherImpl.java:282)
        at com.intellij.concurrency.JobLauncherImpl$1MyTask.call(JobLauncherImpl.java:272)
        at java.util.concurrent.ForkJoinTask$AdaptedCallable.exec(ForkJoinTask.java:1424)
        at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
        at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
        at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
        at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)
    In this file it reports "Listener class has no methods with @EventHandler", but it is an abstract class, so maybe don't run this inspection on abstract classes?

    In all classes in the events package (and its subpackages) it reports "No getHandlerList method is present", but the getHandlerList() method is actually implemented in a super class of these event classes, so the inspection also needs to scan the parent classes for the method.

    The inspection for "EventHandler annotation is unnecessary" has a similar problem, it for example reports that the the @EventHandler in this class is unnecessary, but the super class RegionFeature does implement Listener, so the warning is incorrect.

    I'll run the inspections on some more of my plugins later to see if I can find more, just to keep you busy :D

    Edit: by the way, you can simply go to File > Settings > Plugins, and click on 'Install plugin from disk...' to install the plugin, which is a bit easier than putting it in some Intellij folder.
     
  15. For the MONITOR I changed it to something like this:
    It now checks for a method with an EventHandler annotation with a priority set to MONITOR. Then it checks all method calls in the event method and sees if a method is called on the event parameter. If so it checks if the method name starts with "set". If so it will report it.
    So this:
    Code (Text):
    @EventHandler(priority = EventPriority.MONITOR)
    public void onBlockBreak(BloockBreakEvent event) {
      event.getPlayer(); //won't cause a warning to appear
      event.setExpToDrop(0); // causes a warning to appear
      event.setCancelled(true); // causes a warning to appear

      someVariable.setValue("some value"); //won't cause a warning to appear
    }
    is what it's doing now. There are still a bunch of methods that don't start with "set", which still alter the event, but this works for the majority of stuff.

    I've done some more work on the stuff you said, so it should hopefully function better with classes that implement and extend other classes. I'll probably give it a test tomorrow and see how it goes.
     
  16. Alright, made the changes I described in the above post and changed the inspections, so they should work better with extending and implementing classes. I also added two new inspections. The first one checks if you use commandLabel to check the command used instead of command#getName. The second one checks if you call setCustomName on a player object (that doesn't do anything: source). Added the new file to the main post.
     
    • Agree Agree x 1
  17. The entire project is in Kotlin and I don't know how Kotlin works, so making a PR isn't really that helpful. But if you want you can just take the source and change it to Kotlin yourself. The entire source can be found here.

    I added a new inspection which reports the use of Location#add or Location#subtract, but the parameters are all negative numbers/zeroes. Instead, use the inverse method (add -> subtract and subtract -> add) with positive numbers to increase readability. Added new file to main post.
     
  18. How about also checking if the associated method takes an event class as a parameter? If not the annotation is also useless.
     
    • Agree Agree x 1
  19. What about adding a way for Deprecated methods? Like if you use a deprecated method, it will give you a non-deprecated solution (i don't have ideas at moment)