New Java Check: Mutable enum

[Michel Pawlak]

Hi again,

Here follows another Java check proposition. Please tell me if you would be interested of such a contribution (again the rule is already written and my company can contribute it.)

Description:

@Rule(

    key = "",

    name = "Mutable Enum",

    description = "Rule that checks that Enums are immutable by enforcing fields to be final.",

    priority = Priority.CRITICAL,

    tags = {

        "security",

        "mutability"

    })

@SqaleMetadata(

    characteristic = RulesDefinition.SubCharacteristics.SECURITY_COMPLIANCE,

    remediationCostOffset = "2min", // please don't tell me someone is doing lazy enum configuration... 2 min is enough to add final modifiers, count 2 hours if a developer does not understand why having mutable enum is a really bad practice

    remediationFunctionType = DefaultDebtRemediationFunction.Type.CONSTANT_ISSUE)

Test oracle file :

public enum MyEnum {

    A,

    B,

    C;

    public String mutableField; // Noncompliant {{Make "mutableField" field final.}}

    

    final public String immutableField = ""; // Compliant should not raise an issue

}

[Michał Kordas]

Hi Michel,

A bit of feedback from my side:

• Checking just final is not enough, as you may have final List field and still be able to very easy change state of the enum.
  
• Your check prohibits "lazy enums" when the fields are initialized only when needed. This is common pattern and prohibiting it would be controversial decision.

Thanks,

Michal Kordas

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/368da131-bde8-46da-86d6-93af2ff7e1cf%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Michel Pawlak]

Hi Michal,

Indeed as is the rule does not handle collections / maps, nor any other type of container. To be really immutable all fields need themselve to be immutable otherwise you'll always have a leak somewhere. The real question is "is it worth the effort to do such recursive check", "when should you stop digging", and "is it the aim of the rule" ? In our opinion (the rule was discussed by 5 people and we decided to follow this approach as the one having the best ROI) it is not worth the effort, and the false negatives are acceptable as we have another rule that forbids classes to return arrays/collections/ maps... you have to make a copy first. 

But I agree that there is an opportunity for improvement (a costly one.)

Concerning lazy enums, as I wrote in my previous message :

// please don't tell me someone is doing lazy enum configuration... 2 min is enough to add final modifiers, count 2 hours if a developer does not understand why having mutable enum is a really bad practice

Kind regards ;-)

Michel

[Michał Kordas]

Hi Michel,

Indeed as is the rule does not handle collections / maps, nor any other type of container.  "when should you stop digging", and "is it the aim of the rule"

I thought that SonarQube Java has already some mechanism to detect when collection is for sure mutable. But if not, why not just name this rule Non-final Field in Enum? 

Concerning lazy enums, as I wrote in my previous message 

Ok, but it's just statement. Can you explain in the rule why lazy enums are bad? What are the risks?

Thanks,

Michal 

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/dd753230-b26e-437b-af29-8f8ae52b0205%40googlegroups.com.

[Michel Pawlak]

Hi Michal

Indeed as is the rule does not handle collections / maps, nor any other type of container.  "when should you stop digging", and "is it the aim of the rule"

I thought that SonarQube Java has already some mechanism to detect when collection is for sure mutable. But if not, why not just name this rule Non-final Field in Enum? 

If there is such a mechanism it could be used for sure. Concerning the rule naming, well the goal of having final fields is to have (as close as possible to) immutable enums.

 

Concerning lazy enums, as I wrote in my previous message 

Ok, but it's just statement. Can you explain in the rule why lazy enums are bad? What are the risks?

I admit that I wasn't clear enough and that what I wrote may be misleading. It's more the mutability than the laziness that is a bad practice. Let me quote Oracle's tutorial on Enum Types :

An enum type is a special data type that enables for a variable to be a set of predefined constants.

And here is a definition of a constant : 

In computer programming, a constant is an identifier with an associated value which cannot be altered by the program during normal execution – the value is constant

Well a constant should not have its value be modified. In other words the value should be immutable.

In order to be able to have lazy value definition, you need to have non final fields or final fields of a mutable type. If you define the value of the constant in a lazy way then prevent any further modification, ok. But is it then really a constant ? What about distributed programming where you may end with enums having different values on both ends (is it really what is expected from a constant ?) What about values changing when you restart your application ? It may be what you want to achieve, but is it what a programmer will expect from an enum ? 

It is true that you can have corner cases where you may need / want to have Enums with changing values, but it is not the nominal case. I would not recommend this approach for understandability and reliability reasons.

What is your opinion ?

Regards,

Michel

[arjen.v...@gmail.com]

Hi,

Wouldn't this rule conflict with projects that use an enum to implement a Singleton.

Using single instance enums as singleton is a pattern that is advised by 'Effective Java' as a good way to implement a singleton.

Now, a singleton that is purely immutable would rather be named a constant I suppose, so most projects will likely use that singleton for some mutable object.

Regards,

Arjen

[Michel Pawlak]

Hi Arjen,

Effective Java was written in 2008 i.e. 7 year ago and 3 years before java 1.7 was out. At that times using enums was the only safe way to go in order to implement the Singleton pattern in Java due to a bug related to double check locking (I can't remember which one) in VMs. Since then the bug was fixed (you can use DCL + volatile field if you really want a Singleton), now the singleton pattern is often considered to be an anti-pattern (mostly for testing and concurrency reasons) and DI should be preferred to Singleton usage .

Well, all in all, why using a programming construct to do something it was not meant for in the beginning ? Abraham Maslow said in 1966 "I suppose it is tempting, if the only tool you have is a hammer, to treat everything as if it were a nail".

Enums were meant to provide "smart" strongly typed constants you can use as switch cases and that can have a behavior, let's use them for this purpose. And back to the book you're referring to, IMHO, we should remember that yesterday's best practices can become today's anti-patterns faster than we think.

Kind regards,

Michel

[arjen.v...@gmail.com]

Hi,

True, the book is somewhat dated, on the other hand that does not mean all of its advise is inaccurate.

The book is still one of the most popular books on Java, and by lots of people seen as an ultimate guideline to writing java code.

So yes I understand that:

- Singletons have become somewhat controversial

- There are other ways to implement them (there already were at the time of the books release)

- Best practices are time-bound

But on the other hand does that mean that using an enum to implement a singleton is a bad practice.

There are situations for which a singleton might be a good solution and on those projects an enum is in my opinion an acceptable way to implement it. 

Not every project uses DI, think about plain JavaSE applications or embedded software (raspberry pi, etc). 

And without DI, this is still the best and easiest way to implement them. Maybe even the only way to provide compile-time guarantees ?

It is (in my opinion) somewhat controversial to state that enums should be used only to contain immutable data if that excludes the singleton or lazy loading approach. 

I'm not saying your rule is bad, or should not be used. Not at all.  My intention was to point out that, besides lazy initialized enums, there is another common mutable enum (that you might not have thought about).

Regards,

Arjen

[Nicolas Peru]

Hi to all, 

Sorry I am coming late to the party but I have also some doubts on this rule : Enum was conceived to limit the number of instances of an object, but we can totally imagine cases where you want to store some states of those objects and so they can totally be mutable. 

Let's imagine a simple example where you have a temperature sensor on each of the planet of the solar system (Nasa mission in 2134, probably tricky to install on Jupiter). You could design this by having an enum that encodes the planet and the fact that the number of instance of a planet does not change that much over the course of time (yeah, I know, Pluto... ) and on this enum having a field temperature where the sensors will update the field when the temperature change (so a mutable field).

I don't see flaw on this kind of design and so this rule would raise false positives.

Moreover as precised by Michal, final field is only an approximation of immutability, so I don't think that even with not activated by default this rule will make sense to be included in Sonar Java quality profile. 

I let you keep it as a custom rule Michel, thanks for the submission !

Cheers.  

Nicolas PERU | SonarSource

Senior Developer
http://sonarsource.com

https://twitter.com/benzonico

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/e1394e0f-26af-42d4-88a0-a0605d67cfba%40googlegroups.com.