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

Improve CollectionFactory to allow for single statement collection creation #28025

Closed
snicoll opened this issue Feb 10, 2022 · 6 comments
Closed
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement

Comments

@snicoll
Copy link
Member

snicoll commented Feb 10, 2022

Right now CollectionFactory allows to create a collection type, trying to find the most suitable implementation based on an concrete instance.

We have some use cases in AOT where we'd like to be able to streamline the creation of a collection in a single statement. We've tried something along those lines:

Stream.of(new RuntimeBeanReference("myCommandHandler")).collect(Collectors.toCollection(ManagedList::new))

Unfortunately the indirection with the collector can easily confuse the compiler. A more direct use where the collectionFactory would be provided with a bunch of elements could be nice.

@snicoll snicoll added the type: enhancement A general enhancement label Feb 10, 2022
@snicoll snicoll added this to the 6.0.0-M3 milestone Feb 10, 2022
@snicoll snicoll modified the milestones: 6.0.0-M3, 6.0.0-M4 Mar 16, 2022
@sbrannen sbrannen added the in: core Issues in core modules (aop, beans, core, context, expression) label May 9, 2022
@sbrannen sbrannen self-assigned this May 9, 2022
@sbrannen
Copy link
Member

sbrannen commented May 9, 2022

Based on the requirements you've stated here and in #28094 (comment), I've come up with the following simple prototype.

@SafeVarargs
public static <E, C extends Collection<E>> C collectionOf(Supplier<C> collectionFactory, E... elements) {
	Assert.notNull(collectionFactory, "'collectionFactory' must not be null");
	C collection = collectionFactory.get();
	Collections.addAll(collection, elements);
	return collection;
}

With the following test cases passing.

@Test
void collectionOfWorksCorrectly() {
	List<String> list = collectionOf(ArrayList::new, "foo", "bar");
	assertThat(list).isInstanceOf(ArrayList.class).containsExactly("foo", "bar");

	Set<Integer> set = collectionOf(HashSet::new, 4, 3, 2, 1);
	assertThat(set).isInstanceOf(HashSet.class).containsExactlyInAnyOrder(1, 2, 3, 4);

	Set<Integer> orderedSet = collectionOf(LinkedHashSet::new, 4, 2, 3, 1);
	assertThat(orderedSet).isInstanceOf(LinkedHashSet.class).containsExactlyInAnyOrder(4, 2, 3, 1);

	SortedSet<Integer> sortedSet = collectionOf(TreeSet::new, 4, 3, 2, 1);
	assertThat(sortedSet).isInstanceOf(TreeSet.class).containsExactly(1, 2, 3, 4);
}

@snicoll, is that what you had in mind?

Please note that the tests had to be written that way (with the created collection stored in a local variable), since there were compiler issues with AssertJ's assertThat() methods.

@sbrannen sbrannen modified the milestones: 6.0.0-M4, 6.0.0-M5 May 9, 2022
@snicoll
Copy link
Member Author

snicoll commented May 9, 2022

Not quite. The single statement works obviously but not the type inference.

trying to find the most suitable implementation based on an concrete instance.

This is linked to AOT where we have a collection instance we'd like to write back. Taking the type of the collection is what I had in mind but I haven't dig enough. Phil also may have made that irrelevant with the latest update of code generation.

@snicoll
Copy link
Member Author

snicoll commented May 9, 2022

We could call that inference algorithm based on the instance and get back the nearest type and use that for code generation. So I take it back, looks like that would work. Thanks.

@sbrannen
Copy link
Member

sbrannen commented May 9, 2022

So I take it back, looks like that would work. Thanks.

Are you saying that the proposed collectionOf() method looks good to go?

If so, I'd be happy to push it to main for 6.0 M4.

However, there's no rush on my side. So if it's not OK as-is, we can iterate over alternate solutions for 6.0 M5.

sbrannen added a commit to sbrannen/spring-framework that referenced this issue May 17, 2022
This commit serves as a proof of concept for factory methods in
CollectionFactory that allow a collection to be created as a one-liner
using var-args.

createCollection(Class<C>, Class<E>, E...) internally delegates to
createCollection(Class<?>, Class<?>, int) to create the collection
based on the supplied collectionType and populates the collection from
the var-args. The elementType is only required when creating an EnumSet
and could therefore conceivably be omitted if we do not want to support
EnumSet for this collection factory method.

collectionOf(Supplier<C>, E...) creates the collection using the
supplied collectionFactory and populates the collection from the
var-args.

See spring-projectsgh-28025
@sbrannen
Copy link
Member

sbrannen commented May 17, 2022

@snicoll, I've pushed a proof of concept to the following branch.

https://github.com/sbrannen/spring-framework/commits/issues/gh-28025-CollectionFactory-collectionOf

I think the new createCollection(Class<C>, Class<E>, E...) method may suit your needs, but I'd like to get your feedback before pushing anything to main.

Thanks!

@snicoll
Copy link
Member Author

snicoll commented Aug 9, 2022

This is no longer needed as the AOT use cases became irrelevant.

@snicoll snicoll closed this as not planned Won't fix, can't repro, duplicate, stale Aug 9, 2022
@snicoll snicoll added the status: declined A suggestion or change that we don't feel we should currently apply label Aug 9, 2022
@snicoll snicoll removed this from the 6.0.0-M6 milestone Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants