Skip to content
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

Add support for immutable collections of Google Guava #39

Closed
heri333 opened this issue Apr 24, 2013 · 12 comments

Comments

@heri333
Copy link

commented Apr 24, 2013

Google Guava provides several immutable utility classes, for example ImmutableList.

If there is an assignment to a final field in a constructor like

this.foo = ImmutableList.of();

it is reported as

org.mutabilitydetector.unittesting.MutabilityAssertionError: 
  Expected: ... to be IMMUTABLE
       but: ... is actually NOT_IMMUTABLE
      Reasons:
          Field can have an abstract type (com.google.common.collect.ImmutableList) assigned to it. [Field: foo, Class: ...]
      Allowed reasons:
          None.
@Grundlefleck

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2013

Hi, this is a quick ack to let you know I've seen the issue and will respond shortly. Definitely something we can do here.

@Grundlefleck

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2013

Hello,

There's two options I can think of for what to do here:

  1. configure analysis to hardcode various types from Guava as immutable.
  2. change Mutability Detector code to treat assignments of Guava types much like assignments of Collections.unmodifiableXXX

Benefits of 1. is that you don't need to wait on a code change and a new release from us, you can configure that yourself. That's described here. The downside is that it won't detect when you have, for example, an ImmutableList, which would be considered mutable. Any use of ImmutableList would be considered immutable. The benefits and drawbacks of 2. are the inverse: you have to wait for a release, but it can be smarter about the element type of the collection.

I intend to work on 2, but I recommend you do 1 in the meantime.

Let me know your thoughts.

@heri333

This comment has been minimized.

Copy link
Author

commented Apr 25, 2013

Thanks for your fast response! I actually missed the existing sentence in the start page pointing to the JavaDoc.

Option 1. is fine for me, but I think it would be valuable to provide the smarter and default support for Guava mentioned in 2., since it is a widespread library with dedicated classes to handle immutability.

@Grundlefleck

This comment has been minimized.

Copy link
Contributor

commented May 18, 2013

Been doing more thinking about this, and I think there should be a general idea of immutable container types within Mutability Detector. The support for the JDK's Collections.unmodifiableXXX should be able to refer to a hardcoded list of container types, supplied in the configuration, while also raising warnings when the elements contained are mutable.

@Grundlefleck

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2013

I have decided to attempt the more general solution to this problem. That is, making it possible to configure Mutability Detector to recognise any arbitrary immutable collections.

@sbilinski

This comment has been minimized.

Copy link

commented Apr 16, 2015

Any news regarding this issue? I could really use Guava collection support, but I'm not sure if I should start implementing it on my own.

@Grundlefleck

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2015

Hi @sbilinski, I've been keeping this issue around as something a new contributor could work on, but since there's interest in it, I'll bump the priority.

Are you more interested in contributing code to the project or in having it Just Work? I hope that doesn't sound "snippy", it's a genuine question, if you have a particular interest in contributing it, I'm more than happy to help towards that, otherwise, I'll pick it up myself.

Thanks!

@Grundlefleck

This comment has been minimized.

Copy link
Contributor

commented May 4, 2015

Just released 0.9.3 with support for immutable collection types of Guava.

@Stephan202

This comment has been minimized.

Copy link

commented May 15, 2015

@Grundlefleck, this is pretty cool. However, for some reason it seems that ImmutableSortedMap is still deemed mutable. The first two tests in the following class pass, but the third fails:

public final class ImmutabilityTest {
    // Passes
    @Test
    public void testWithImmutableMapField() {
        final class ImmutableMapWrapper {
            private final ImmutableMap<String, String> map = ImmutableMap.of();
        }

        MutabilityAssert.assertImmutable(ImmutableMapWrapper.class);
    }

    // Passes
    @Test
    public void testWithImmutableSortedMultisetField() {
        final class ImmutableSortedMultisetWrapper {
            private final ImmutableSortedMultiset<String> map = ImmutableSortedMultiset.of();
        }

        MutabilityAssert.assertImmutable(ImmutableSortedMultisetWrapper.class);
    }

    // Fails
    @Test
    public void testWithImmutableSortedMapField() {
        final class ImmutableSortedMapWrapper {
            private final ImmutableSortedMap<String, String> map = ImmutableSortedMap.of();
        }

        MutabilityAssert.assertImmutable(ImmutableSortedMapWrapper.class);
    }
}

The reported error is:

org.mutabilitydetector.unittesting.MutabilityAssertionError: 
    Expected: com.example.ImmutabilityTest$1ImmutableSortedMapWrapper to be IMMUTABLE
         but: com.example.ImmutabilityTest$1ImmutableSortedMapWrapper is actually NOT_IMMUTABLE
        Reasons:
            Field can have an abstract type (org.mutabilitydetector.internal.com.google.common.collect.ImmutableSortedMap) assigned to it. [Field: map, Class: com.example.ImmutabilityTest$1ImmutableSortedMapWrapper]
        Allowed reasons:
            None.

Interestingly, org.mutabilitydetector.config.GuavaConfiguration does explicitly list ImmutableSortedMap (just like the other two).

NB: If you prefer I can file a separate issue for this.

@Grundlefleck

This comment has been minimized.

Copy link
Contributor

commented May 17, 2015

Hi @Stephan202,

This is a problem with your imports -- but it's introduced because of how I package the Jar, so apologies :)

The imported ImmutableSortedMap comes from the repackaged version of Guava that Mutability Detector uses. The clue is in this message:
Field can have an abstract type (org.mutabilitydetector.internal.com.google.common.collect.ImmutableSortedMap) assigned to it.

Note the org.mutabilitydetector.internal prefix on the package. If you change the imports to point directly to com.google.common.collect.ImmutableSortedMap it should be fine.

I'm assuming that the reason the repackaged class gets picked up is because of the automatic import feature of whichever IDE you're using. I believe most IDEs allow this feature to be configured, and I have observed the convention of excluding an import matching *.internal.*, which is why I picked that package name.

Lemme know if that's not the issue.

@Stephan202

This comment has been minimized.

Copy link

commented May 17, 2015

Hi @Grundlefleck, very good catch. I should have seen that... :/ Fixing the imports makes the test pass.

The reason I created the test cases in the first, place, however, is that I did have an issue with ImmutableSortedMap in code that was written before Mutability Detector was even on the classpath. (I.e., no incorrect import statements.) This is a an example that's closer to the real code (note the nesting):

// Fails
@Test
public void testWithImmutableSortedMapField2() {
    final class ImmutableSortedMapWrapper {
        private final ImmutableSortedMap<String, ImmutableSortedMultiset<String>> map = ImmutableSortedMap.of();
    }

    MutabilityAssert.assertImmutable(ImmutableSortedMapWrapper.class);
}

This test fails with the following error (observe how the error message makes it appear that ImmutableSortedMap takes three type parameters):

org.mutabilitydetector.unittesting.MutabilityAssertionError: 
    Expected: com.example.ImmutabilityTest$2ImmutableSortedMapWrapper to be IMMUTABLE
         but: com.example.ImmutabilityTest$2ImmutableSortedMapWrapper is actually NOT_IMMUTABLE
        Reasons:
            Field can have collection with mutable element type (com.google.common.collect.ImmutableSortedMap<java.lang.String, com.google.common.collect.ImmutableSortedMultiset, java.lang.String>) assigned to it. [Field: map, Class: com.example.ImmutabilityTest$2ImmutableSortedMapWrapper]
        Allowed reasons:
            None.

Thanks for having a look :)

@Grundlefleck

This comment has been minimized.

Copy link
Contributor

commented May 17, 2015

Ah yes, nested generics is a known problem. I'll open another issue for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.