views:

2490

answers:

11

In my code, I am creating a collection of objects which will be accessed by various threads in a fashion that is only safe if the objects are immutable. When an attempt is made to insert a new object into my collection, I want to test to see if it is immutable (if not, I'll throw an exception).

One thing I can do is to check a few well-known immutable types:

private static final Set<Class> knownImmutables = new HashSet<Class>(Arrays.asList(
        String.class, Byte.class, Short.class, Integer.class, Long.class,
        Float.class, Double.class, Boolean.class, BigInteger.class, BigDecimal.class
));

...

public static boolean isImmutable(Object o) {
    return knownImmutables.contains(o.getClass());
}

This actually gets me 90% of the way, but sometimes my users will want to create simple immutable types of their own:

public class ImmutableRectangle {
    private final int width;
    private final int height;
    public ImmutableRectangle(int width, int height) {
        this.width = width;
        this.height = height;
    }
    public int getWidth() { return width; }
    public int getHeight() { return height; }
}

Is there some way (perhaps using reflection) that I could reliably detect whether a class is immutable? False positives (thinking it's immutable when it isn't) are not acceptable but false negatives (thinking it's mutable when it isn't) are.

Edited to add: Thanks for the insightful and helpful answers. As some of the answers pointed out, I neglected to define my security objectives. The threat here is clueless developers -- this is a piece of framework code that will be used by large numbers of people who know next-to-nothing about threading and won't be reading the documentation. I do NOT need to defend against malicious developers -- anyone clever enough to mutate a String or perform other shenanigans will also be smart enough to know it's not safe in this case. Static analysis of the codebase IS an option, so long as it is automated, but code reviews cannot be counted on because there is no guarantee every review will have threading-savvy reviewers.

+5  A: 

Basically no.

You could build a giant white-list of accepted classes but I think the less crazy way would be to just write in the documentation for the collection that everything that goes is this collection must be immutable.

Edit: Other people have suggested having an immutable annotation. This is fine, but you need the documentation as well. Otherwise people will just think "if I put this annotation on my class I can store it in the collection" and will just chuck it on anything, immutable and mutable classes alike. In fact, I would be wary of having an immutable annotation just in case people think that annotation makes their class immutable.

SCdF
Also, in the JCIP book there is discussion about effectively immutable classes like java.util.Date, which in practice are used as immutable even if they aren't
ddimitrov
+5  A: 

At my company we've defined an Attribute called @Immutable. If you choose to attach that to a class, it means you promise you're immutable.

It works for documentation, and in your case it would work as a filter.

Of course you're still depending on the author keeping his word about being immutable, but since the author explicitly added the annotation it's a reasonable assumption.

Jason Cohen
If you're depending on the author keeping his word that it's immutable what's the point? It's not adding anything, and it could potentially cause harm if people think the annotation *makes* their class immutable..
SCdF
The idea is hopefully in the near future you'll have some static code analysis - maybe integrated into your IDE - to go through your code and check that your @Immutable annotated classes *are* actually immutable.
Cem Catikkas
At Jason's company (which I also work at), we also code review every line of code that's written. So, if you put the @Immutable attribute on a class that's not immutable, you're gonna get called on it. Ask Jason, I've done it several times.
Brandon DuRette
By the way, the idea for the @Immutable attribute is not original, it comes from Java Concurrency In Practice by Brian Goetz.
Brandon DuRette
Also, @SCdF, you're always relying on the author to if a class is immutable or not in Java, and even then there are ways of changing any member variables. The Annotation is probably the best of what YOU might call the worst.
PintSizedCat
The annotation is a great idea. I'm not sure it meets all my needs, because of the danger people would apply it indiscriminately.
mcherm
I'm not saying the annotation is wrong, I'm saying that it's like security by obscurity-- it gives a false sense of security. In this case a false comfort that your code is immutable.
SCdF
Also, all you've done is made sure that the original author (assuming he typed the @immutable himself) has *possibly* thought about what that means. You have no such guarantee on people who extend or alter the class in a year and never even *notice* the @immutable at the top.
SCdF
@SCdF: Yes, but welcome to software development. It's true of *everything* that someone might come along later and abuse it. They might not read the JavaDoc. They might not grok the spirit of the methods. They might not understand how the class is *now* used, as opposed to the design.
Jason Cohen
+1  A: 

You Can Ask your clients to add metadata (annotations) and check them at runtime with reflection, like this:

Metadata:

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.CLASS)
public @interface Immutable{ }

Client Code:

@Immutable
public class ImmutableRectangle {
    private final int width;
    private final int height;
    public ImmutableRectangle(int width, int height) {
        this.width = width;
        this.height = height;
    }
    public int getWidth() { return width; }
    public int getHeight() { return height; }
}

Then by using reflection on the class, check if it has the annotation (I would paste the code but its boilerplate and can be found easily online)

Pablo Fernandez
How would you stop people from just putting that annotation on non-immutable classes?
SCdF
You don't stop them. If you can't follow instructions how to use something, you shouldn't try to program it.
Simon Lehmann
@Simon, OK, so then what's the point of the annotation at all? Can't you just assume that they are smart enough to read the documentation about the collection and thus know that it's only for immutable things?
SCdF
@SCdF, I tried to give a cost-effective approach, maybe he can make it work. Saying "NO, YOU CANT" is the same as not saying anything at all (or worst)
Pablo Fernandez
Now that I have thought about it a bit more, I have to partially agree, that having an annotation might lead to confusion and misunderstandings.
Simon Lehmann
I fear that some of our developers would incompletely understand the concept and misuse the annotation. Otherwise, it would be an excellent solution. Combined with validation it might work perfectly.
mcherm
+10  A: 

There is no reliable way to detect if a class is immutable. This is because there are so many ways a property of a class might be altered and you can't detect all of them via reflection.

The only way to get close to this is:

  • Only allow final properties of types that are immutable (primitive types and classes you know are immutable),
  • Require the class to be final itself
  • Require that they inherit from a base class you provide (which is guaranteed to be immutable)

Then you can check with the following code if the object you have is immutable:

static boolean isImmutable(Object obj) {
 Class<?> objClass = obj.getClass();

 // Class of the object must be a direct child class of the required class
 Class<?> superClass = objClass.getSuperclass();
 if (!Immutable.class.equals(superClass)) {
  return false;
 }

 // Class must be final
 if (!Modifier.isFinal(objClass.getModifiers())) {
  return false;
 }

 // Check all fields defined in the class for type and if they are final
 Field[] objFields = objClass.getDeclaredFields();
 for (int i = 0; i < objFields.length; i++) {
  if (!Modifier.isFinal(objFields[i].getModifiers())
    || !isValidFieldType(objFields[i].getType())) {
   return false;
  }
 }

 // Lets hope we didn't forget something
 return true;
}

static boolean isValidFieldType(Class<?> type) {
 // Check for all allowed property types...
 return type.isPrimitive() || String.class.equals(type);
}

Update: As suggested in the comments, it could be extended to recurse on the superclass instead of checking for a certain class. It was also suggested to recursively use isImmutable in the isValidFieldType Method. This could probably work and I have also done some testing. But this is not trivial. You can't just check all field types with a call to isImmutable, because String already fails this test (its field hash is not final!). Also you are easily running into endless recursions, causing StackOverflowErrors ;) Other problems might be caused by generics, where you also have to check their types for immutablity.

I think with some work, these potential problems might be solved somehow. But then, you have to ask yourself first if it really is worth it (also performance wise).

Simon Lehmann
Maybe "isValidFieldType" could do a recursion to isImmutable. :-)
marcospereira
With marcospereira's modification, this could be quite useful! We could also recurse on the superclass (allowing Object as a special case) which would eliminate the need to inherit from a common ancestor. This is good!
mcherm
These are good suggestions. I tried to avoid recursing on the superclasses and members for the sake of simplicity and performance (the recursion could get quite extensive). But I'll add these suggestions to the anweser.
Simon Lehmann
If you're checking the class of a specific instance, there is IMHO no need for it to be final - if another instance comes by that is a subclass, it will be checked anyways.
Martin Probst
"There is no reliable way to detect if a class is immutable." - this is not the case. Perhaps you mean using just reflection? But bytecode analysis using a library such as BCEL or ASM would allow discovering if a class is mutable. In fact, I've just done 95% of that as part of a university assignment.
Grundlefleck
@Grundlefleck: Could you explain how to do this using bytecode analysis? Can this be done easily during runtime? I find this really interesting and it would probably help others who want to do this in the future.
Simon Lehmann
@Simon, there are several checks which could ensure a class is immutable - if the fields are immutable and and can't be reassigned, etc. The tool I created checked these things with ASM. It was static, but I don't know of any reason why it wouldn't work at run time. If you're interested in hearing more, you can contact me at grundlefleck at gmail dot com. The tool is basic, written in two weeks, but if you're interested, I can see if I can get it thrown up on googlecode or something.
Grundlefleck
@Simon: threw something together - http://code.google.com/p/mutability-detector/
Grundlefleck
+2  A: 

This could be another hint:

If the class has no setters then it cannot be mutated, granted the parameters it was created with are either "primitive" types or not mutable themselves.

Also no methods could be overriden, all fields are final and private,

I'll try to code something tomorrow for you, but Simon's code using reflection looks pretty good.

In the mean time try to grab a copy of the "Effective Java" book by Josh Block , it has an Item related to this topic. While is does not for sure say how to detect an inmmutable class, it shows how to create a good one.

The item is called: "Favor immutability"

link: http://java.sun.com/docs/books/effective/

OscarRyz
+1  A: 

Like the other answerers already said, IMHO there is no reliable way to find out if an object is really immutable.

I would just introduce an interface "Immutable" to check against when appending. This works as a hint that only immutable objects should be inserted for whatever reason you're doing it.

interface Immutable {}

class MyImmutable implements Immutable{...}

public void add(Object o) {
  if (!(o instanceof Immutable) && !checkIsImmutableBasePrimitive(o))
    throw new IllegalArgumentException("o is not immutable!");
  ...
}
dhiller
+1  A: 

In my code, I am creating a collection of objects which will be accessed by various threads in a fashion that is only safe if the objects are immutable.

Not a direct answer to your question, but keep in mind that objects that are immutable are not automatically guaranteed to be thread safe (sadly). Code needs to be side-effect free to be thread safe, and that's quite a bit more difficult.

Suppose you have this class:

class Foo {
  final String x;
  final Integer y;
  ...

  public bar() {
    Singleton.getInstance().foolAround();
  }
}

Then the foolAround() method might include some non-thread safe operations, which will blow up your app. And it's not possible to test for this using reflection, as the actual reference can only be found in the method body, not in the fields or exposed interface.

Other than that, the others are correct: you can scan for all declared fields of the class, check if every one of them is final and also an immutable class, and you're done. I don't think methods being final is a requirement.

Also, be careful about recursively checking dependent fields for immutability, you might end up with circles:

class A {
  final B b; // might be immutable...
}

class B {
  final A a; // same so here.
}

Classes A and B are perfectly immutable (and possibly even usable through some reflection hacks), but naive recursive code will go into an endless loop checking A, then B, then A again, onwards to B, ...

You can fix that with a 'seen' map that disallows cycles, or with some really clever code that decides classes are immutable if all their dependees are immutable only depending on themselves, but that's going to be really complicated...

Martin Probst
These are both really good warnings, neither of which I had really considered. I doubt that I need to defend against the recursively contained immutable object (simplest case would be an immutable object containing itself in a field!), but the other is a bit of a danger.
mcherm
+5  A: 

Use the Immutable annotation from Java Concurrency in Practice. The tool FindBugs can then help in detecting classes which are mutable but shouldn't be.

Bno
How should he apply FindBugs while running his code? From what I understand he wants to check at runtime if a mutable object is added?
dhiller
He can use Findbugs at build time to reduce the number of missanotated classes and use the annotation at runtime.
ddimitrov
I hadn't realized that Findbugs did this analysis -- that's great! I took a look at the docs and it seems that it simply verifies that all fields of an annotated class are final (but does not verify that they all contain immutable objects).
mcherm
+1  A: 

why do all the recommendations require the class to be final? if you are using reflection to check the class of each object, and you can determine programmatically that that class is immutable (immutable, final fields), then you don't need to require that the class itself is final.

james
Besides, having the *class* be final has *absolutely* no bearing on its immutability. It just means it can't be extended.
bendin
A: 

Check out joe-e, a implementation of capabilities for java.

A: 

Something which works for a high percentage of builtin classes is test for instanceof Comparable. For the classes which are not immutable like Date, they are often treated as immutable in most cases.

Peter Lawrey