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

Improve error message for Collections being wrapped without safe copy #52

Closed
marcgs opened this Issue Oct 19, 2013 · 9 comments

Comments

Projects
None yet
3 participants
@marcgs

marcgs commented Oct 19, 2013

Currently the error message for a collection field being wrapped without a safe copy is "Attempts to wrap mutable collection type without safely performing a copy first". It would be useful to provide a hint how a safe copy can be performed for the current use case. For instance for a List this can be achieved with:

Collections.unmodifiableList(new ArrayList(sourceList));

The goal of this issue is to improve the current error message with such hints how the copy can be done so that Mutability Detector does not report an error anymore.

Here a test which asserts the current error message:

    public final static class ClassWrappingCollectionWithoutCopy {
        private final Collection<String> collection;

        public ClassWrappingCollectionWithoutCopy(Collection<String> collection) {
            this.collection = Collections.unmodifiableCollection(collection);
        }
    }

    @Test
    public void isImmutableMutableTypeToField_ClassWrappingCollectionWithoutCopy() throws Exception {
        try {
            assertImmutable(ClassWrappingCollectionWithoutCopy.class);
            fail("Error should be thrown");
        } catch (MutabilityAssertionError e) {
            assertThat(e.getMessage(), is("\n"+
                    "Expected: org.mutabilitydetector.ErrorLocationTest$ClassWrappingCollectionWithoutCopy to be IMMUTABLE\n" +
                    "     but: org.mutabilitydetector.ErrorLocationTest$ClassWrappingCollectionWithoutCopy is actually NOT_IMMUTABLE\n" +
                    "    Reasons:\n" +
                    "        Attempts to wrap mutable collection type without safely performing a copy first. [Field: collection, Class: org.mutabilitydetector.ErrorLocationTest$ClassWrappingCollectionWithoutCopy]\n" +
                    "    Allowed reasons:\n" +
                    "        None."));
        }
    }
@Grundlefleck

This comment has been minimized.

Contributor

Grundlefleck commented Oct 19, 2013

Thanks. Applied the newcomer tag.

@badgersow

This comment has been minimized.

Contributor

badgersow commented Jan 5, 2016

Hi, is anyone working on this?
If none, can I grab this issue for myself?
Thanks.

@Grundlefleck

This comment has been minimized.

Contributor

Grundlefleck commented Jan 5, 2016

Hi @badgersow, nobody is currently working on this, so feel free! Let me know if you have any questions or problems getting started.

@badgersow

This comment has been minimized.

Contributor

badgersow commented Jan 10, 2016

Hi, @Grundlefleck
Could you please give me an advice.
I implemented hint generation like the following:

Collections.unmodifiableList(new ArrayList(sourceList));

But I want to add type parameter to constructor call because java compiler generates warning:

Collections.unmodifiableList(new ArrayList<String>(sourceList));

To do this I need to resolve type parameter String in sourceList field declaration.
I have ASM Type class instance, and I need to get type parameter out of this.
Do you know easy (possible already implemented) way to do this?

I already googled ASM signature visitors, but I hope there is easier way.
Thanks a lot for your time.

@Grundlefleck

This comment has been minimized.

Contributor

Grundlefleck commented Jan 10, 2016

Hi @badgersow,

There is existing code elsewhere which retrieves the generic type of a collection, when assigned to a field. In that case it's used to say e.g. ImmutableList<String> is immutable, but ImmutableList<Date> is not. If that sounds like what you want, take a look at the code here:

https://github.com/MutabilityDetector/MutabilityDetector/blob/master/src/main/java/org/mutabilitydetector/checkers/CollectionWithMutableElementTypeToFieldChecker.java#L110

Let me know if that helps :)

@badgersow

This comment has been minimized.

Contributor

badgersow commented Jan 11, 2016

Thank you for fast reply.

This is almost what I need.
But I will change CollectionField class because now

CollectionField.from("SomeType<A, B>")

is equal to

CollectionField.from("SomeType<A<B>>")

which is bad for my particular case.

Anyway, many thanks for providing starting point for me.

@badgersow

This comment has been minimized.

Contributor

badgersow commented Jan 16, 2016

Hi, @Grundlefleck
Thanks for your help, today I succeded with SignatureVisitor and finally I can retrieve full generics information from field declaration. Yay!

Please, help me with one more question.
In case we have field declaration with wildcards, e.g.

private List<? extends A> list;

or

private List<? super A> list;

What hint do you think will be the most correct?

I can think of two cases here:

  1. Collections.unmodifiableList(new ArrayList<A>(list))
  2. Collections.unmodifiableList(new ArrayList(list))

Personally I tend to chose answer 1, but anyway please tell me your opinion.
Thank you in advance.

@Grundlefleck

This comment has been minimized.

Contributor

Grundlefleck commented Jan 17, 2016

Sounds good to me, the parts that matter are the wrapping in unmodifiable, and constructing a new collection, I think it's fine to let the user figure out their generics, as that doesn't affect the hint.

Seems like you're progressing very well :)

Grundlefleck added a commit that referenced this issue Jan 25, 2016

Merge pull request #70 from badgersow/master
issue #52 Provide hint for wrapping-with-copy idiom
@Grundlefleck

This comment has been minimized.

Contributor

Grundlefleck commented Jan 25, 2016

I think this issue has been addressed by @badgersow, the output will now look like:

Attempts to wrap mutable collection type without safely performing a copy first. You can use this expression: Collections.unmodifiableSortedSet(new TreeSet<ImmutableExample>(unmodifiable))

Which is a great improvement.

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