Can Subclass checking is incorrect when inner class constructs instance of outer, immutable class

[Grundlefleck]

The following class is considered mutable:

public class OnlyPrivateConstructors {
    private final String field;

    private OnlyPrivateConstructors(String field) {
        this.field = field;
    }

    public String getField() {
        return field;
    }

    public static class Builder {
        public OnlyPrivateConstructors build() {
            return new OnlyPrivateConstructors("hi");
        }
    }
}

With the message:

Expected: org.mutabilitydetector.issues.OnlyPrivateConstructors to be IMMUTABLE
     but: org.mutabilitydetector.issues.OnlyPrivateConstructors is actually NOT_IMMUTABLE
    Reasons:
        Can be subclassed, therefore parameters declared to be this type could be mutable subclasses at runtime. [Class: org.mutabilitydetector.issues.OnlyPrivateConstructors]
    Allowed reasons:
        None.

If the call to new OnlyPrivateConstructors() is commented out, the analysis correctly calls this class immutable. This pattern is a common way of implementing the Builder pattern (as described in Effective Java, iirc). It should not render the outer class as immutable.

[Grundlefleck]

Hey @neomatrix369 I've assigned this to you as you were asking if there was anything specific you can do. This one will be in the bytecode analysis part of the code. Let me know if you feel up for it.

Couple of pointers:

• the code to change will likely be in a class called CanSubclassChecker
• start by reproducing the bug with a an example like the one above
• there's a couple of flags which are used to tell if the class is marked final, or if it's constructors are all private, there is likely to be a flaw in the logic in the private method part, debugging will help :)

[neomatrix369]

Thanks @Grundlefleck, your assignment is pretty timely as I'm about to create an enhancement in my project as well to apply the Mutability Detector to project (see enhancement issue: neomatrix369/OpenJDKProductivityTool#20).

I gladly take on this issue and I'll keep you posted of my findings.

Btw. just finished reading Effective Java earlier this week!

[neomatrix369]

I commented the call to new OnlyPrivateConstructors("hi"); and got the below, I guess we cannot escape the below case with many of the classes we write:

org.mutabilitydetector.unittesting.MutabilityAssertionError: 
Expected: OnlyPrivateConstructors to be IMMUTABLE
     but: OnlyPrivateConstructors is actually NOT_IMMUTABLE
    Reasons:
        Can be subclassed, therefore parameters declared to be this type could be mutable subclasses at runtime. [Class: OnlyPrivateConstructors]
        Field can have a mutable type (java.lang.String) assigned to it. [Field: field, Class: OnlyPrivateConstructors]
    Allowed reasons:
        None.
    at org.mutabilitydetector.unittesting.internal.AssertionReporter.assertThat(AssertionReporter.java:41)
    at org.mutabilitydetector.unittesting.MutabilityAssert.assertImmutable(MutabilityAssert.java:276)
        ...

What do we do about this, another logic bug?

[Grundlefleck]

That case should be fixed in HEAD, as java.lang.String is now hardcoded to be immutable.

Interesting, could you post the complete code, including imports?

[neomatrix369]

Here's the project, I have uploaded into a new repo:
https://github.com/neomatrix369/Mutability-detector-examples/tree/master/ImmutableSubClassIssue

Should we maintain a repo with examples, which we can also use as a communication repo when resolving or debugging known issues - I;ll change ownership to us if you agree!

[Grundlefleck]

I was meaning just to paste into these comments or create a Gist. If we want to share examples in a project then the existing ClientOfMutabilityDetector can be that project.

I think I know what's going on. Since you have a separate project, and have declared the dependency as 0.8, you're getting an out of date version of Mutability Detector. If you do a 'mvn install' inside MutabilityDetector dir, then bump the version to 0.9-SNAPSHOT in your project, you should see the String warning disappear.

I was assuming if you were going to be working on the code itself, you would work on the latest code, from the head of the master branch in your fork. In that case, you can add example classes to 'src/test/benchmarks'. Then you can write unit tests for the checker in question in /src/test/java/org/mutabilitydetector/benchmarks/CanSubclassCheckerTest.java

Going afk for a while, let me know how you get on :)

[neomatrix369]

Agreed, I should have done that. So that's how you can use ClientOfMutabilityDetector - I'll take a look at to understand it better.

I couldn't get the project to build earlier due to issues (it does now after disabling tests and installing the jar). But its sorted now, my project is using 0.9-SNAPSHOT and does not give that error any more so we are in business.

Although commenting it does give compilation errors.

I'll take on board the example and test suggestions you have made.

[neomatrix369]

Haven't looked at ClientOfMutabilityDetector yet but we might have a solution to our current issue. Here's some gists I have created that give an idea of what I have done to resolve the issue (or atleast I think I did).

AccessModifierQuery.java - https://gist.github.com/4388325
CanSubclassChecker.java - https://gist.github.com/4388322
CanSubclassCheckerTest.java - https://gist.github.com/4388331
ImmutableByHavingOnlyAPrivateConstructorUsingTheBuilderPattern.java - https://gist.github.com/4388418 (new class)

I know you would have seen these when I pushed the results but I thought I rather give you an idea of what I have done before I pushed the changes.

Let me know if I have made changes I shouldn't have like in CanSubclassCheckerTest.java when I added the new class to the AnalysisResultTheory checks or the long name to the class or the way I have changed the logic in the CanSubclassChecker.java class.

Also don't we need to also check for the immutability of the inner class - with our example, if we did that, then the whole class is immutable?

Any thoughts on the above?

[Grundlefleck]

I think that changeset looks very good, very close to what I would have put myself, I think. The super long name is good, it's not like it's being used in real code, and in tests I find it better to be on the verbose side.

I may alter some formatting or make small changes, but that looks good to be pushed. Please close the issue once you get a chance to push, good job Mani :)

[neomatrix369]

As you can see its checked in, its cool that github linked my commit(s) to the issue. I like verbose names, that way I dont have to remember why I named them so - seems like we have a good habit in common!

I do have questions though?
Don;t we need to check for mutability of the inner class itself? Also whats this AnalysisResultTheory doing? Are we certain there's not edge-case that could break through our fix (test case)?
Do you have a list of all the behaviours or flow that the detector has to go through? Would be handy to have - maybe something I can draw when I understand it more!

[Grundlefleck]

In terms of inner classes, static inner classes are no issue at all. They do not have a reference to the enclosing class so state changed does not affect the outer class. Non-static inner classes may be an issue, and worth investigating more, but I don't think it should hold up this issue, as I suspect the builder pattern would be more common for immutable classes. Worth a look though, if you're interested.

The AnalysisResultTheory is a way of using the "Theory" feature in JUnit. It's a way of supplying lots of different test data in a single test case.

In terms of breaking other edge cases, it could happen, but there's a decent amount of coverage in the test cases we have. I'm confident on this one.

What do you mean in terms of "behaviours or flow that the detector has to go through". Do you mean as in data-flow analysis? There is only very limited forms of flow analysis, and is definitely not provably correct. Or do you mean just in terms of the executing of mutability detector in general, and the control and data flow that occurs during an analysis? If that's the case, let me know, I can expand on the "architecture" (if you can call it that).

You mentioned elsewhere, perhaps in an email, about trying to find a better way of helping devs get started without running into the issue you saw. I agree, could you raise an issue and propose a fix? You'll have better perspective as you were coming fresh-faced to the project. I'm thinking of something like having a GETTING_STARTED.md in the root directory of the git checkout, but open to more novel suggestions :)

[neomatrix369]

I have open issues as you can see above, considering the previous comment.

What do you mean in terms of "behaviours or flow that the detector has to go through". Do you mean as in data-flow analysis? There is only very limited forms of flow analysis, and is definitely not provably correct. Or do you mean just in terms of the executing of mutability detector in general, and the control and data flow that occurs during an analysis? If that's the case, let me know, I can expand on the "architecture" (if you can call it that).

With regards to the above comment I meant a flow diagram showing from the conceptual level what are the different cases and how the flow of logic works from start to finish. In a way we can also see the architecture of the system.

I was intending to use a tool like Graphviz (do you have any suggestions wrt it?), as they produce some useful diagrams (see http://www.graphviz.org/Gallery.php and http://www.flickr.com/photos/kentbye/sets/72157601523153827/). Its purely for my knowledge sake, but would also help other developers and users of @MutabilityDetector !

[Grundlefleck]

w.r.t. a diagram, feel free to try out Graphviz, if the output is good it would be well suited to put on the wiki page for developers. Failing that I think I could put something together in e.g. Inkscape that demonstrates how the pieces of mutability detector fit together.

Will add that to the description of issue #36