New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No obvious way to use Collections.umodifiable* #26

Closed
Grundlefleck opened this Issue Dec 5, 2012 · 5 comments

Comments

Projects
None yet
1 participant
@Grundlefleck
Contributor

Grundlefleck commented Dec 5, 2012

Original author: mahow...@gmail.com (March 07, 2012 15:32:55)

What steps will reproduce the problem?

I have a class that has just one field, and that field can't be changed, and it's assigned in the constructor to be an immutable colllection. I don't have an obvious way though to prove that with this. Obviously, static analysis of a class may be difficult/impossible without the actual instance being created, but I didn't see how to test against that. I also don't see a way from Collections that I can even get hold of the unmodifable classes (private for the unmodifable map) so that I could write a custom matcher. Thoughts?

With a class that has:

private final Map<String,Object> propMap;
public ImmutableConfiguration(AbstractConfiguration origConfiguration) {

    LOG.debug(&quot;&gt;&gt;&gt; ImmutableConfiguration&quot;);

    Map store = new HashMap();
    //we need the variables interpolated, which doesn't come at the getProperty level
    Configuration interpolated = origConfiguration.interpolatedConfiguration();
    final Iterator keyIter = interpolated.getKeys();
    while (keyIter.hasNext()) {
        String key = (String) keyIter.next();
        store.put(key,interpolated.getProperty(key));
    }
    this.propMap = Collections.unmodifiableMap(store);
    LOG.debug(&quot;&lt;&lt;&lt; ImmutableConfiguration&quot;);
}

}

I'll get
Field can have an abstract type (java.util.Map) assigned to it. [Field: propMap, Class: com.angel.config.ImmutableConfiguration]

Original issue: http://code.google.com/p/mutability-detector/issues/detail?id=25

@ghost ghost assigned Grundlefleck Dec 5, 2012

@Grundlefleck

This comment has been minimized.

Contributor

Grundlefleck commented Dec 5, 2012

From Grundlefleck@gmail.com on March 07, 2012 17:57:23
Hi,

This is just a quick ACK to let you know I've seen this issue and will get back to you once I've had a think about it :)

Thanks!
Graham

@Grundlefleck

This comment has been minimized.

Contributor

Grundlefleck commented Dec 5, 2012

From Grundlefleck@gmail.com on March 07, 2012 22:31:53
Hi,

tl;dr: pinch the allowed reason implementation (class AssumeCopiedIntoUnmodifiable) from here: bit.ly/AftG2r

The unmodifiableXXX methods are a bit tricky at the moment. It would require a significantly more sophisticated analysis to correctly discover this is safe. Some thoughts:

  • analysis could potentially dive into the methods within Collections, see that unmodifiableMap() returns a certain concrete type 'private static class UnmodifiableMap', and analyse that concrete type for immutability, letting that information flow back to the original analysis. But beyond this special case of a one line method, the data and control flow analysis needed to prove what exactly will be returned from any given method is, frankly (and sadly), a bit too difficult for me to achieve at the moment.
  • analysis would need to make sure that no references to the wrapped collection escapes. Consider in your example, if you passed the 'store' map to another method, that method could change the contents, and since the unmodifiable version is just a view, it could be seen to change. Some analysis currently exists to check that the 'this' reference doesn't escape, but it's not sound (i.e. provably correct) and I'm not hugely confident that analysis could also apply with local variables quite so well.
  • it could be possible to check for this very specific case. Meaning, the analysis could 'bless' Collections.unmodifiableXXX as being safe, and not raise an error when the result is assigned to a field. It would have to be careful, about lines of code like this: "myField = someCondition ? unmodifiableMap(store) : someOtherTotallyMutableMap;". Hardcoding it is doable, I think, but the issue here is that as things get more complicated, it's way harder to get it right. And if there are any similar constructs in your own code, e.g. "unmodifiableMyDomainThingy(MyDomainThingy d);", Mutability Detector will be completely oblivious to them.

With that in mind, I think the best approach for now is to leave the core analysis much as it is, and try to match the scenario in the test. It could be possible, for example, to store analysis information which states something along the lines of "field assigned directly from parameter x", "field assigned directly from method call to java.util.Collections.unmodifiableMap", "field assigned from constructor invocation", etc. That's not something that's available now, I'm afraid, but it's a possibility if it becomes more effective to match situations accurately in tests vs. improving the core analysis.

For your specific problem, I have written a custom allowed reason, which is as accurate as I can currently make it. There is an allowed reason already available "AllowedReason.provided(Map.class).isAlsoImmutable()" which will allow the test to pass. It's a bit weak, in that someone could come along and add another Map field, without wrapping it with unmodifiableMap, and the test would still pass. It also doesn't really represent the unmodifiableXXX scenario, as you can declare you assume any type as immutable.

The code for the custom allowed reason is viewable here: https://mutability-detector.googlecode.com/svn/trunk/ClientOfMutabilityDetector/trunk/ClientOfMutabilityDetector/src/test/java/org/mutabilitydetector/issues/ImmutableConfiguration.java

If this seems suitable for you, and I don't find any huge problems with it, it should be available out-of-the-box with release 0.9.

The allowed reason will check that the type that's being assigned is one of the types available in an unmodifiable version from java.util.Collections.unmodifiableXXX. It will fail if the call is replaced, for example, by "new HashMap(store)". It will still pass if however, you extract "new HashMap(store)" into a private method declared like this:

private java.util.Map myFakeUnmodifiableMap(HashMap store) {
return store;
}

... which obviously is a false negative. This is also a bit lame in that it's not refactor friendly, since the field name is matched as a String repeated in the test. There's also the whole can of worms that the elements of propMap might also be mutable, so as far as static analysis can tell, you might have a method like this:

public Object getSomeProperty(String key) {
    return propMap.get(key);
}

Where a caller can do "SomeMutableThing s = (SomeMutableThing) immutableConfiguration.get("theKey"); s.horribleMutateyThings();", obviously wrecking any guarantees of immutability. Note as well, that generics does not help to solve this, since with erasure, they types end up declared as java.lang.Object in bytecode anyway. But that's probably an issue for another day! :)

Hopefully that implementation of an AllowedReason allows you to still have confidence in the test. Please let me know if you have any thoughts or questions.

Kind regards,
Graham

@Grundlefleck

This comment has been minimized.

Contributor

Grundlefleck commented Dec 5, 2012

From Grundlefleck@gmail.com on March 07, 2012 22:42:58
Sorry, the allowed reason in that code is unlikely to work without modification, because it's using an internal, repackaged type from Mutability Detector. Hopefully the functionality is clear enough that it's just minor tweaking of imports to get it working.

~ Graham

@Grundlefleck

This comment has been minimized.

@Grundlefleck

This comment has been minimized.

Contributor

Grundlefleck commented Dec 16, 2012

Hi,

I'm going to close this issue, as the upcoming 0.9 release will have support for Collections.unmodifiableXXX methods, that will be usable without any extra configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment