Mutability Detector says my class is immutable, but effectively it is not!
49 views
Skip to first unread message
Markus Karg
Jul 21, 2012, 8:46:30 AM7/21/12
to mutabilit...@googlegroups.com
I wonder why Mutability Detector says my class is immutable, since effectively it is not!
Here is the full story. I wrote a class that is "effectively" immutable (it has a getter that returns Collections.unmodifiableList(backingField), so it is impossible to modify the content). As the backingField itself is typed as List and implemented as LinkedList, I have to allow MUTABLE_TYPE_TO_FIELD and ABSTRACT_TYPE_TO_FIELD. Using this, Mutability Detector confirms that my class is effectively immutable. So far so good.
Now to check the unit test itself, I replaced "return Collections.unmodifiableList(backingField);" in the getter by simply "return backingField" which obviously makes my class pretty mutable, as anybody can modify the collection now.
But Mutability Detector still says, my class would be effectively immutable. It seems it neither actually tries to modify the collection, nor does it know anything about the JRE's unmodifiableList API or this simple way to reach effective immutability (described in Josh Bloch's Effective Java as "defensive copy").
I do not understand why Mutability Detector does not see the difference. In fact, this is very harmful, as Mutability Detector declares something to be "safe" which absolutely is not. What is my fault? How to fix it? What I like to reach is that I want the unit test to fail if sombody returns the backing field instead of a defensive copy or an immutable wrapper.
Graham Allan
Jul 21, 2012, 4:37:49 PM7/21/12
to mutabilit...@googlegroups.com
Hi Markus,
On 21 July 2012 13:46, Markus Karg <mar...@headcrashing.eu> wrote:
> [snip]
the analysis just panics when it sees a mutable field, because there's
a long list of potential problems when this happens. There is more
detail at a similar issue filed here:
http://code.google.com/p/mutability-detector/issues/detail?id=25
The upshot is that you are correct, Mutability Detector is oblivious
of Collections.unmodifiableXXX, but perhaps it's time to check for
that specific idiom. I reckon it would be very specific to this
particular idiom though, it wouldn't recognise a generic "wrap this x
in an unmodifiable x" pattern, and it wouldn't recognise defensive
copies in general. It would likely also only allow the defensive copy
on field assignment, not at the point of the getter method (this
starts to move into the trickier world of escape analysis, which is
non-trivial).
It's certainly not your fault, the warning that MutDet raises is a
fairly weak, generic "panic", and suppressing it is equally generic.
The only thing I can think of to make the tool happy is basically
duplicate the unmodifiable types, and declare the fields to be of that
type, but that's horrible. Instead I'll make this the top priority
issue for 0.9.
If MutDet only allowed copy and wrap at the point of field assignment
in the constructor, would that be enough?
Regards,
Graham
On 21 July 2012 13:46, Markus Karg <mar...@headcrashing.eu> wrote:
> [snip]
>
> I do not understand why Mutability Detector does not see the difference. In
> fact, this is very harmful, as Mutability Detector declares something to be
> "safe" which absolutely is not. What is my fault? How to fix it? What I like
> to reach is that I want the unit test to fail if sombody returns the backing
> field instead of a defensive copy or an immutable wrapper.
It's an unfortunate weakness in Mutability Detector. At the moment,
> I do not understand why Mutability Detector does not see the difference. In
> fact, this is very harmful, as Mutability Detector declares something to be
> "safe" which absolutely is not. What is my fault? How to fix it? What I like
> to reach is that I want the unit test to fail if sombody returns the backing
> field instead of a defensive copy or an immutable wrapper.
the analysis just panics when it sees a mutable field, because there's
a long list of potential problems when this happens. There is more
detail at a similar issue filed here:
http://code.google.com/p/mutability-detector/issues/detail?id=25
The upshot is that you are correct, Mutability Detector is oblivious
of Collections.unmodifiableXXX, but perhaps it's time to check for
that specific idiom. I reckon it would be very specific to this
particular idiom though, it wouldn't recognise a generic "wrap this x
in an unmodifiable x" pattern, and it wouldn't recognise defensive
copies in general. It would likely also only allow the defensive copy
on field assignment, not at the point of the getter method (this
starts to move into the trickier world of escape analysis, which is
non-trivial).
It's certainly not your fault, the warning that MutDet raises is a
fairly weak, generic "panic", and suppressing it is equally generic.
The only thing I can think of to make the tool happy is basically
duplicate the unmodifiable types, and declare the fields to be of that
type, but that's horrible. Instead I'll make this the top priority
issue for 0.9.
If MutDet only allowed copy and wrap at the point of field assignment
in the constructor, would that be enough?
Regards,
Graham
Markus Karg
Aug 4, 2012, 1:29:00 PM8/4/12
to mutabilit...@googlegroups.com
Graham,
sorry for the delay, but I was rather busy these days...
Please find my answer below the quoted text.
Thank you for making this a top priority. In fact, solving this issue will
safe me a lot of time as I currently in fact use a rather bad hack -- doing
a trial and error game of injection of dynamic mocks into the getter's
returned collection (using Powermock's Whitebox.newInstance hammer to get
"anything" mocked) and then checking for equality of the original collection
and another get()'s result... which indeed works, but is not as smart as
using a framework like yours. :-)
For my special case, unfortunately copy and wrap at field assignment will
not work, as the class is providing another defensive copy at time of get().
This is necessary because the set() is not the only one constructing the
internal collection: The class uses JAXB, and the JAXB provider will first
construct and empty instance (hence, internal collection field) and after
that using either set() OR directly modify the internal collection field.
Sad but true, as long as JAXB is part of the project, things are rather
complex.
Thanks!
Markus
sorry for the delay, but I was rather busy these days...
Please find my answer below the quoted text.
safe me a lot of time as I currently in fact use a rather bad hack -- doing
a trial and error game of injection of dynamic mocks into the getter's
returned collection (using Powermock's Whitebox.newInstance hammer to get
"anything" mocked) and then checking for equality of the original collection
and another get()'s result... which indeed works, but is not as smart as
using a framework like yours. :-)
For my special case, unfortunately copy and wrap at field assignment will
not work, as the class is providing another defensive copy at time of get().
This is necessary because the set() is not the only one constructing the
internal collection: The class uses JAXB, and the JAXB provider will first
construct and empty instance (hence, internal collection field) and after
that using either set() OR directly modify the internal collection field.
Sad but true, as long as JAXB is part of the project, things are rather
complex.
Thanks!
Markus
Graham Allan
Aug 8, 2012, 8:52:14 AM8/8/12
to mutabilit...@googlegroups.com
Hi Markus,
>
> For my special case, unfortunately copy and wrap at field assignment will
> not work, as the class is providing another defensive copy at time of get().
> This is necessary because the set() is not the only one constructing the
> internal collection: The class uses JAXB, and the JAXB provider will first
> construct and empty instance (hence, internal collection field) and after
> that using either set() OR directly modify the internal collection field.
> Sad but true, as long as JAXB is part of the project, things are rather
> complex.
>
> Thanks!
> Markus
>
This sounds to me kind of like the situation with Hibernate, where
trying to convince yourself that the class is immutable is a losing
battle. For effectively immutable, you basically have to guarantee
that no-one else in your entire program, except the JAXB magic, is
calling your setter method. For me, this seems out of scope for
Mutability Detector. If possible, could you please provide a couple of
code snippets that embody your classes and how they're used? I'd like
to have a greater understanding of what your code looks like, in case
I'm misunderstanding some nuance.
The confusion of what Effectively Immutable means to MutDec is perhaps
a flaw in how I've translated it from JCIP. I've basically taken it to
relate to safe publication, whereas JCIP does talk about your whole
program using a type as effectively immutable (even java.util.Date).
It could be an interesting avenue to go down to perform whole-program
analysis, checking that mutator methods are never called, but I don't
see it fitting into the "write a single test for this class" pattern.
I'm apprehensive about doing any analysis outside of ensuring
unmodifiable wrapping at field assignment. There would be a lot of
ways to trick the analysis, and false negatives, as you say, are very
dangerous.
Hopefully if you can provide some code examples, I can provide a more
definitive response :)
Thanks for the feedback!
~ Graham
>
> For my special case, unfortunately copy and wrap at field assignment will
> not work, as the class is providing another defensive copy at time of get().
> This is necessary because the set() is not the only one constructing the
> internal collection: The class uses JAXB, and the JAXB provider will first
> construct and empty instance (hence, internal collection field) and after
> that using either set() OR directly modify the internal collection field.
> Sad but true, as long as JAXB is part of the project, things are rather
> complex.
>
> Thanks!
> Markus
>
trying to convince yourself that the class is immutable is a losing
battle. For effectively immutable, you basically have to guarantee
that no-one else in your entire program, except the JAXB magic, is
calling your setter method. For me, this seems out of scope for
Mutability Detector. If possible, could you please provide a couple of
code snippets that embody your classes and how they're used? I'd like
to have a greater understanding of what your code looks like, in case
I'm misunderstanding some nuance.
The confusion of what Effectively Immutable means to MutDec is perhaps
a flaw in how I've translated it from JCIP. I've basically taken it to
relate to safe publication, whereas JCIP does talk about your whole
program using a type as effectively immutable (even java.util.Date).
It could be an interesting avenue to go down to perform whole-program
analysis, checking that mutator methods are never called, but I don't
see it fitting into the "write a single test for this class" pattern.
I'm apprehensive about doing any analysis outside of ensuring
unmodifiable wrapping at field assignment. There would be a lot of
ways to trick the analysis, and false negatives, as you say, are very
dangerous.
Hopefully if you can provide some code examples, I can provide a more
definitive response :)
Thanks for the feedback!
~ Graham
Markus Karg
Aug 15, 2012, 4:26:07 PM8/15/12
to mutabilit...@googlegroups.com
Graham,
I think you misunderstood a bit. There is no magic, and no setter. To better
explain the shortcoming of Mutability Detector, and the obvious solution,
please find the attached ZIP which contains a very simple maven based set
up. When reading the few source lines, you will understand the problem:
The code shows a most simple JAXB application. Class A references to
serveral instances of Class B. The reference (FIELD "bs") itself is not
mutable as it is final. There is no setter for bs. But there is a getter.
"bs" is initialized to LinkedList. Yes, LinkedList IS mutable so Mutability
Detector fails step1 of the test case, complaining about exactly that. But,
LikedList MUST be mutable, as JAXB injects the Class B instances into it.
But, EFFECTIVELY (i. e. for 99.9% of all Java programs, say, all those not
doing dirty "under-the-hood" tricks like ASM or Reflection), the LinkedList
IS IMMUTABLE as it is shielded by a defensive copy (best practice, see Josh
Bloch's "Effective Java"). In our particular scenario, the defensive copy
actually is even more effective, as it uses JRE's "unmodifiableList"
defensive wrapper builder method)! So, as we know that the LinkedList here
does no harm, we tell Mutability Detector in step2 to ignore this fact.
Step2 will succeed.
So far so good. But here come the bad news. Some silly programmer "by
incident" removes the defensive wrapper, so Class A directly returns the
LinkedList itself. NOW certainly Class A IS MUTABLE as the LinkedList is
mutable by any "normal" Java program. The horror is that Step2 still says:
"Sleep well, everything is fine.". Well, nothing is fine. This is a
false-positive!
See, this has nothing to do with strange or seldom use cases or "magic" in
any way. It has to do with the fact that checking for EFFECTIVE IMMUTABILITY
only looks at the FIELDS while it actually must look at the PROPERTIES. The
property "bs" IS EFFECTIVELY IMMUTABLE as long as it cannot be modified with
other means than ASM or Reflection. Mutability Detector's main shortcoming
is the assumption that mutability is always a FIELDS problem and could be
detected by static code analysis. But in contrast, EFFECTIVE IMMUTABILITY in
the real world of Java APIs like JAXB, JPA, and lots more, is understood as
IMMUTABLE PROPERTIES. But how to check this?
The solution is rather simple. I'm doing it like this (pseudo-code), and
Mutability Detector easily could do the same with a bit of Reflection:
* Create an instance of A (to be fully sure, this has to be done for EVERY
constructor).
* Use Reflection to inject a value into the Field A.bs.
* Call getter A.getBs().
* Try to modify the result of the getter by invoking the well-known
Collection API's modification methods (ignoring any Exceptions).
* Call getter A.getBs() again.
* Compare both collections.
* If both are equals() then the PROPERTY A.bs is EFFECTIVELY IMMUTABLE --
independent of the fact that a LinkedList was used! --> SUCCESS
* If both are NOT equals() then the PROPERTY A.bs IS MUTABLE -- independent
of the ignored reasons masking the FIELD! --> FAIL
Yes, this is complex, but working. :-)
Regards
Markus
> -----Original Message-----
> From: mutabilit...@googlegroups.com [mailto:mutability-
> dete...@googlegroups.com] On Behalf Of Graham Allan
> Sent: Mittwoch, 8. August 2012 14:52
> To: mutabilit...@googlegroups.com
> Subject: Re: Mutability Detector says my class is immutable, but
> effectively it is not!
>
I think you misunderstood a bit. There is no magic, and no setter. To better
explain the shortcoming of Mutability Detector, and the obvious solution,
please find the attached ZIP which contains a very simple maven based set
up. When reading the few source lines, you will understand the problem:
The code shows a most simple JAXB application. Class A references to
serveral instances of Class B. The reference (FIELD "bs") itself is not
mutable as it is final. There is no setter for bs. But there is a getter.
"bs" is initialized to LinkedList. Yes, LinkedList IS mutable so Mutability
Detector fails step1 of the test case, complaining about exactly that. But,
LikedList MUST be mutable, as JAXB injects the Class B instances into it.
But, EFFECTIVELY (i. e. for 99.9% of all Java programs, say, all those not
doing dirty "under-the-hood" tricks like ASM or Reflection), the LinkedList
IS IMMUTABLE as it is shielded by a defensive copy (best practice, see Josh
Bloch's "Effective Java"). In our particular scenario, the defensive copy
actually is even more effective, as it uses JRE's "unmodifiableList"
defensive wrapper builder method)! So, as we know that the LinkedList here
does no harm, we tell Mutability Detector in step2 to ignore this fact.
Step2 will succeed.
So far so good. But here come the bad news. Some silly programmer "by
incident" removes the defensive wrapper, so Class A directly returns the
LinkedList itself. NOW certainly Class A IS MUTABLE as the LinkedList is
mutable by any "normal" Java program. The horror is that Step2 still says:
"Sleep well, everything is fine.". Well, nothing is fine. This is a
false-positive!
See, this has nothing to do with strange or seldom use cases or "magic" in
any way. It has to do with the fact that checking for EFFECTIVE IMMUTABILITY
only looks at the FIELDS while it actually must look at the PROPERTIES. The
property "bs" IS EFFECTIVELY IMMUTABLE as long as it cannot be modified with
other means than ASM or Reflection. Mutability Detector's main shortcoming
is the assumption that mutability is always a FIELDS problem and could be
detected by static code analysis. But in contrast, EFFECTIVE IMMUTABILITY in
the real world of Java APIs like JAXB, JPA, and lots more, is understood as
IMMUTABLE PROPERTIES. But how to check this?
The solution is rather simple. I'm doing it like this (pseudo-code), and
Mutability Detector easily could do the same with a bit of Reflection:
* Create an instance of A (to be fully sure, this has to be done for EVERY
constructor).
* Use Reflection to inject a value into the Field A.bs.
* Call getter A.getBs().
* Try to modify the result of the getter by invoking the well-known
Collection API's modification methods (ignoring any Exceptions).
* Call getter A.getBs() again.
* Compare both collections.
* If both are equals() then the PROPERTY A.bs is EFFECTIVELY IMMUTABLE --
independent of the fact that a LinkedList was used! --> SUCCESS
* If both are NOT equals() then the PROPERTY A.bs IS MUTABLE -- independent
of the ignored reasons masking the FIELD! --> FAIL
Yes, this is complex, but working. :-)
Regards
Markus
> -----Original Message-----
> From: mutabilit...@googlegroups.com [mailto:mutability-
> dete...@googlegroups.com] On Behalf Of Graham Allan
> Sent: Mittwoch, 8. August 2012 14:52
> To: mutabilit...@googlegroups.com
> Subject: Re: Mutability Detector says my class is immutable, but
> effectively it is not!
>
Graham Allan
Aug 18, 2012, 11:25:34 AM8/18/12
to mutabilit...@googlegroups.com
Forgive my ignorance of using JAXB, but I see "injecting members into
a private collection field" as a bit magic. Magic just meaning that it
uses something like reflection, we all know the horrors that can be
achieved with reflection. If reflection can be used to modify the
characters of a String instance, Mutability Detector has to declare
defeat at some point :)
Forgetting JAXB in particular, treating it as "Framework F which uses
reflection" there is a general problem in considering class A
immutable. Consider:
F constructs A
F injects members
A returns populated list of B
F injects some more members
A returns same list, but with different contents
Program has observed value of A changing over time.
Now, presumably, once JAXB has done its initial thing, it doesn't get
involved again. But that's only guaranteed by JAXB, not the class A.
If the general pattern across frameworks (JAXB, JPA, Hibernate, etc)
is "construct by reflection, then do nothing else" then I create an
analysis that will allow use of those frameworks, while still
performing the checks you mention. It will be more risky and
time-consuming (effectiveness requires alias and escape analysis), so
I would maybe implement it as an opt-in, second-pass analysis during
the unit test.
I'm not particularly keen on the technique of reflectively
instantiating and checking for different results. Constructing
arbitrary objects in a valid way - think non-empty constructors, where
params also have to be properly constructed, which could depend on
arbitrary business logic. As I found in ThreeTen, the devs were happy
using that technique, but they wrote test cases which constructed
objects manually.
I'll ruminate on the idea of allowing a second-pass, opt-in analysis
specific to those frameworks which have a post constructor
initialisation step. Will let you know if I come up with anything. In
the meantime, it sounds like you've found a solution that works for
you.
Thanks for the time you've spent explaining and documenting your use case!
Regards,
Graham
a private collection field" as a bit magic. Magic just meaning that it
uses something like reflection, we all know the horrors that can be
achieved with reflection. If reflection can be used to modify the
characters of a String instance, Mutability Detector has to declare
defeat at some point :)
Forgetting JAXB in particular, treating it as "Framework F which uses
reflection" there is a general problem in considering class A
immutable. Consider:
F constructs A
F injects members
A returns populated list of B
F injects some more members
A returns same list, but with different contents
Program has observed value of A changing over time.
Now, presumably, once JAXB has done its initial thing, it doesn't get
involved again. But that's only guaranteed by JAXB, not the class A.
If the general pattern across frameworks (JAXB, JPA, Hibernate, etc)
is "construct by reflection, then do nothing else" then I create an
analysis that will allow use of those frameworks, while still
performing the checks you mention. It will be more risky and
time-consuming (effectiveness requires alias and escape analysis), so
I would maybe implement it as an opt-in, second-pass analysis during
the unit test.
I'm not particularly keen on the technique of reflectively
instantiating and checking for different results. Constructing
arbitrary objects in a valid way - think non-empty constructors, where
params also have to be properly constructed, which could depend on
arbitrary business logic. As I found in ThreeTen, the devs were happy
using that technique, but they wrote test cases which constructed
objects manually.
I'll ruminate on the idea of allowing a second-pass, opt-in analysis
specific to those frameworks which have a post constructor
initialisation step. Will let you know if I come up with anything. In
the meantime, it sounds like you've found a solution that works for
you.
Thanks for the time you've spent explaining and documenting your use case!
Regards,
Graham
Markus Karg
Aug 19, 2012, 11:07:18 AM8/19/12
to mutabilit...@googlegroups.com
Graham,
thank you for taking the time considering "Framework F".
In fact, there are two things to understand about these "F"s: (1) Whether or
not they will do further modifications depends on the framework F, not on
the class A (JAXB will not further touch the object after the initial
injection following the construction, while JPA *can* do that if the caller
of the JPA API explicitly enforces it -- which obviously is not possible on
an immutable object). Some of these frameworks are even able to modify
classes that ARE immutable, how scary! So, it would be unfair to judge class
A to be effectively mutable or effectively immutable basing on the used
framework solely. The solution must be as second pass, as you write, that
checks what actually happens when using the interface of class A,
independend of the fact that a field is mutable (blackbox principle). This
should certainly be an opt-in, just as "Allowance Matchers" are an opt-in.
(2) "Framework F" often is part of the official Java EE, and lots of people
will use them. I see it as an essential part of Mutability Detector to work
in the real world of Java EE, not only in the academic theory. As a result,
to prevent Mutability Detector to learn about all parts of magic in Java EE,
an opt-in solution is essential to turn Mutability Detector into a
production-level tool that can be used for "real" applications (JPA for
example is one such framework, and it gets more and more popular). We cannot
close our eyes and ignore this trend.
And please remember: This discussion started because of a false-POSITIVE so
I do not have a solution actually. What I am currently doing is NOT USING
Mutability Detector due to that! :-(
Regards and thank you for the great tool!
thank you for taking the time considering "Framework F".
In fact, there are two things to understand about these "F"s: (1) Whether or
not they will do further modifications depends on the framework F, not on
the class A (JAXB will not further touch the object after the initial
injection following the construction, while JPA *can* do that if the caller
of the JPA API explicitly enforces it -- which obviously is not possible on
an immutable object). Some of these frameworks are even able to modify
classes that ARE immutable, how scary! So, it would be unfair to judge class
A to be effectively mutable or effectively immutable basing on the used
framework solely. The solution must be as second pass, as you write, that
checks what actually happens when using the interface of class A,
independend of the fact that a field is mutable (blackbox principle). This
should certainly be an opt-in, just as "Allowance Matchers" are an opt-in.
(2) "Framework F" often is part of the official Java EE, and lots of people
will use them. I see it as an essential part of Mutability Detector to work
in the real world of Java EE, not only in the academic theory. As a result,
to prevent Mutability Detector to learn about all parts of magic in Java EE,
an opt-in solution is essential to turn Mutability Detector into a
production-level tool that can be used for "real" applications (JPA for
example is one such framework, and it gets more and more popular). We cannot
close our eyes and ignore this trend.
And please remember: This discussion started because of a false-POSITIVE so
I do not have a solution actually. What I am currently doing is NOT USING
Mutability Detector due to that! :-(
Regards and thank you for the great tool!
Markus
> -----Original Message-----
> From: mutabilit...@googlegroups.com [mailto:mutability-
> dete...@googlegroups.com] On Behalf Of Graham Allan
> Sent: Samstag, 18. August 2012 17:26
> To: mutabilit...@googlegroups.com
> Subject: Re: Mutability Detector says my class is immutable, but
> effectively it is not!
>
> -----Original Message-----
> From: mutabilit...@googlegroups.com [mailto:mutability-
> dete...@googlegroups.com] On Behalf Of Graham Allan
> Sent: Samstag, 18. August 2012 17:26
> To: mutabilit...@googlegroups.com
> Subject: Re: Mutability Detector says my class is immutable, but
> effectively it is not!
>
Reply all
Reply to author
Forward
0 new messages