Search code examples
javasonarlint

SonarLint rule Java:S3252 ("static" base class members should not be accessed via derived types)


I've installed SonarLint for STS a few days ago and I would like to ask a question about the rule Java:S3252 ("static" base class members should not be accessed via derived types).

In my code, for some reason (I'm not the company's architect), there are interfaces containing i18n keys used across many programs. One of those interfaces is called RegieI18n and extends CommonI18n. When I need a key from CommonI18n, I use RegieI18n if the class concerns "Regie", for example RegieI18n.KEY_ACTION_EXECUTE, instead of CommonI18n.KEY_ACTION_EXECUTE. The rule S3252 is not respected in that case.

In my mind, using RegieI18n (the child) is useful to prevent developers to re-write some code when, one day, someone decides to override KEY_ACTION_EXECUTE with another value for RegieI18n. The rule can also be applied to methods (in fact, all accessible members), please don't focus on the fact that we use i18n keys for the example.

What could be the problem if I use RegieI18n.KEY_ACTION_EXECUTE instead of CommonI18n.KEY_ACTION_EXECUTE?


Solution

  • This is steering dangerously close to 'opinion' which is beyond the scope of SO. Hence, not much opinion in this answer - just the objective basis on which S3252 is based.

    Java is generally called 'Object Oriented' and as a consequence, most of its concepts are explained in terms of OO - and presumably your average java coder's instincts lie in the OO direction.

    That's a problem in this case: static stuff does not "do" OO. AT ALL. - for example, dynamic dispatch (where if you call someAnimalRef.makeNoise() will actually call the Dog class's makeNoise() method if the ref turns out to be referencing an instance of Dog, even though the variable someAnimalRef is typed Animal) - does not apply to static at all.

    In other words, given that this is static stuff, javac decides which constant to use (and for an encore, if said constant is Compile Time Constant - i.e. a string or number literal or could have been derived using only baked-in math (no libraries) that involves only literals – then any refs to this variable are straight up replaced with the constant directly).

    This could be confusing. The way you wrote it, you're still getting 'inheritance' of sorts - but it's compile time inheritance. Specifically, if you have (store these 3 in separate files):

    interface CommonI18n {
      String KEY_ACTION_EXECUTE = "Execute";
    }
    
    interface RegieI18N extends  CommonI18n {
    }
    
    class Test {
      public static void main(String[] args) {
        System.out.println(RegieI18N.KEY_ACTION_EXECUTE);
      }
    }
    

    and run:

    javac *.java; java Test
    

    It prints Execute, obviously. If you then edit RegieI18N.java to this and recompile just it:

    interface RegieI18N extends CommonI18n {
      String KEY_ACTION_EXECUTE = "Uitvoeren";
    }
    
    // and then on the command line:
    
    javac RegieI18N.java
    java Test
    

    It still prints Execute, even though your intent was obviously that it should be printing Uitvoeren now. There are 2 separate reasons for this (and either one is sufficient): [A] Static stuff is resolved 100% by javac, so unless you're recompiling Test.java, it'll never work, and [B] constants like this are inlined.

    That's what the underlying idea behind the warning: That this is confusing, and even if not, that this is hampering your future flexibility. You cannot introduce/change any of the i18n stuff unless you recompile the entire application. Given that some 'incremental compilation' tools will not recompile Test if Test.class is newer than Test.java, that's quite a big deal.

    So to avoid this?

    Simple - don't use constants. Don't use static if you want the ability to 'override in a subtype'. Java has i18n stuff baked in, where you put the data in properties files which you can then ship together with your class files (inside jars, if you want). This interface based strategy seems to have essentially no upside (I guess technically its more performant, but I doubt you could ever measure it, and given that i18n kinda implies 'strings that are meant for human eyeballs', and 'meant for human eyeballs' kinda implies: The CPU is so not going to be the bottleneck, you can completely ignore it), the performance upside seems like a farcically crazy misunderstanding of perspective here. Worrying about performance for this is completely wrong. Most likely - there are exceptions to every rule, of course.

    If somehow you don't want to use java's stuff, a good first start is to invoke a method (static if you really need it to be) to get these strings; you don't want to write RegieI18N.KEY_ACTION_EXECUTE, you want to write something like RegieI18N.getKeyActionExecute(), or even RegieI18n.KEY_ACTION_EXECUTE() (note the parentheses at the end there). You can use code-gen to create these methods if you must. You now have the flexibility to write whatever you feel like in these, and the JVM will never inline or otherwise just eliminate such a call. If later on you decide you want that method to return something else, and you want to deploy it by recompiling just RegieI18n, then now you can.

    You're just reinventing wheels badly of course, but that gets rid of the primary concern that S3252 is trying to address.