Define behaviour for adding a ConfigurableFileCollection to itself

[ljacomet]

Currently, if one adds a ConfigurableFileCollection to itself, directly or indirectly, this creates a StackOverflowError when resolving the file collection, as can be seen in #8748.

Gradle should detect such situations and either fail with a clear exception indicating the problem or ignore that element.

This probably applies to FileCollection as well.

[adammurdoch]

The behaviour should also make sense for setting the value of a lazy type using something derived from itself. Some examples:

• adding a DomainObjectCollection to itself
• adding a Property<List<T>> to itself
• setting the value of a Property<T> to a Provider<T> derived from the property
• adding a TaskDependency to itself.

[adammurdoch]

This is fixed for a common case where += is used from the Groovy DSL: https://github.com/gradle/gradle/pull/12842

[jjohannes]

We would like to restore this change in the JavaExec task: https://github.com/gradle/gradle/commit/22ca8e2e3714209e9000b4825b349c2ba246ad47

For that, we have to make at lease this test pass (on top of what was done in #12842): https://github.com/gradle/gradle/commit/22ca8e2e3714209e9000b4825b349c2ba246ad47#diff-f4ec7d2e34e668d08bf7369bcf62f9faR177

We should also add test coverage for other ways of composing a file collection with 'cycles'.

[mlopatkin]

Here is the summary of the current behaviors of the lazy types.

ConfigurableFileCollection

Can be added to itself, a derived file collection can be added to itself. It works by introspecting the added value and replacing the reference to the collection with the snapshot before addition (aka shallow copy).

Cannot contain a provider derived from itself (circular evaluation of the provider):

def col = files("build.gradle")
def provider = col.elements.map { file(it.name + ".kts") }
col.from(provider)

Properties and Providers

These do not support self-references. The only exceptions are providers returned by NamedDomainObjectContainer, in particular, TaskProvider. These can resolve themselves in their configuration callbacks, though this exposes partially configured Task instance.

DomainObjectCollection

Supports adding to itself with addAll, which evaluates the argument eagerly. Lazy addition with addAllLater with a provider that resolves the collection internally only works for NamedDomainObjectSet implementations. These rely on the Set's element uniqueness property to achieve that. In general, sets without user-specified order can have themselves as sub-elements, because the nested evaluation is guaranteed to add no elements. However, when the contents of the collection is used to derive an element, the logic doesn't apply: the derivation function obviously cannot observe its own result, so it only sees some subset of the collection contents. This makes the evaluation unreliable or hard to reason about, consider the following snippet:

class NamedString { String name }

def container = objects.namedDomainObjectSet(NamedString)

Provider<Iterable<String>> p1 = provider {
    container.toList().empty ? [new NamedString(name: "a")] : [new NamedString(name: "b")]
}
Provider<Iterable<String>> p2 = provider {
    container.toList().empty ? [new NamedString(name: "c")] : [new NamedString(name: "d")]
}
container.addAllLater(p1)
container.addAllLater(p2)
assert container.toList().collect { it.name } == ["b", "c", "d"]

Note: an issue with the evaluation order causes p2 to be computed and added twice.

The problem is easier when we're talking about configure-like callbacks that apply some configurations to the element. These callbacks are allowed to observe the contents of the collection, though, obviously, some observed elements do not have their final configuration. There are tests that verify this behavior.

TaskDependency

From a cursory glance, having a self-referencing TaskDependency is supported, because dependency walking doesn't descend into visited nodes once more. Circular dependencies between tasks are, however, forbidden and fail the build.

Conclusion

The "replace with shallow copy" approach adds predictability but can only work with types that allow introspection of their structure, like FileCollection. For everything else, we'd have to store shallow copies before each mutation, and even then we're not safe. For example, a Provider.map function can reference a mutable Provider field that holds the very same provider:

def ref = new AtomicReference<Provider<String>>()
def p = provider { "foo" }.map { it + ref.get().get() }
ref.set(p)
println(p.get())

Set container types can work around direct self-references by ignoring them, but the ability to add derived elements still gets them into a weird state. This approach cannot be employed for other containers where the ordering matters.

[lptr]

What is missing for this so that we can close it?

[mlopatkin]

What is missing for this so that we can close it?

An actual decision if/how we need to unify the behavior further and a potential follow up to make the code conform to it, I think. This is something we can discuss next week.