In `ListProperty`:

/**
 * Adds zero or more elements to the property value.
 *
 * <p>The given provider will be queried when the value of this property is queried.
 * This property will have no value when the given provider has no value.
 *
 * @param provider Provider of elements
 */
void addAll(Provider<? extends Iterable<? extends T>> provider);

Is it really intended that if the

Provider

passed to the *add*All is value-less that the entire list gets dropped (including anything previously add’ed or addAll’ed)?

I recall I saw some related issue 🤔

That’s not the same thing though. Thats suggesting that:

list.convention([foo]);
list.add(bar);

should result in

[foo, bar]

when it currently results in

[bar]

only. I’m saying that:

list.add(foo)
list.add(emptyProvider())

should result in

[foo]

but actually results in

[]

.

Sounds strange to me, even though it is documented like that. I'd thumbs-up an issue if you create it. Doesn't really sound sensible to do.

I’m not sure though. Resetting a property to its convention value is done by setting it to null, which, in my opinion, is semantically different from setting it to an empty collection. Whether that’s behaviour that’s wanted in all cases is another matter and I’m not sure there’s a right answer. At least currently it’s consistent.

Not really. If you have a

set

where you set an empty provider, it is expected that the result is an empty provider. But the point here is, that an

add

or

addAll

is called and it feels very wrong to me, that an

add

or

addAll

removes all content.

It might be strange, but I still feel it’s consistent in the sense that a convention should only apply if a user is not interfering with the value, or explicitly resetting it. Once you call

add

or

addAll

you are interfering and telling the convention to back off. If you for some reason start adding nothing, that feels more like a user error. This is very much a grey area and both sides have merit though 🤷

Not really, because actually you are talking about convention value. OP here talks about previously `add`ed or `addAll`ed values, not about convention values.

In practice it results in an error:

tasks.register("listPropTest") {
    doLast {
        val list = objects.listProperty<String>()
        list.add("test")
        println("initial: ${list.get()}")
        val otherList = objects.listProperty<String>()
        otherList.addAll("one", "two", "three")
        list.addAll(otherList)
        println("with other: ${list.get()}")
        val emptyList = objects.listProperty<String>()
        list.addAll(emptyList)
        println("with empty: ${list.get()}")
        list.add(objects.property<String>())
        println("with empty prop: ${list.get()}")
    }
}

has output

> Task :listPropTest FAILED
initial: [test]
with other: [test, one, two, three]
with empty: [test, one, two, three]

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':listPropTest'.
> Cannot query the value of this property because it has no value available.

Not sure if that’s better than what’s described above 🤔

I think you’re just running in to the brain-twisting unset/empty contracts of the collection property types. Maybe this is clearer?:

def list = project.objects.listProperty(String)
list.add('foo')
list.add(providers.gradleProperty('foo'))
if (list.isPresent()) {
  println "List: ${list.get()}"
} else {
  println 'List: <not-present>'
}

$ gradle -Pfoo=bar
List: [foo, bar]

$ gradle
List: <not-present>

Right, but that’s just masking the behaviour. It’s not actually emptying the list, it’s just hiding away the otherwise surfaced error. As I said though, very much unclear whether that’s better 😛

Hm, yeah, that's actually questionable and gray again. Was fooled by "results in `[]`". So what you need is probably something like

list.addAll(project.objects.listProperty(String).orElse([]))

Oh I found a solution… I just think the contract is bizarre and unhelpful. Ultimately an “add” to a collection should not remove anything from the collection. The ‘convention’ exception to this can I think be somewhat justified (although I’m really not sure there either) but no way should an add remove a previously added value.

again, the add is not actually removing anything. The

isPresent()

method is delegating to any providers contained within the property, which does give you the impression that things have disappeared.

Exactly, see my suggestion for

addAll

. For

add

it is not exactly as straightforward, as you need to add "something" from the provider, you cannot add "nothing" from the provider.

Just because the behavior can be described by appealing to internal implementation details of the type does not mean the contract is a good one.

well, that’s just untrue. It is a list, and it is adding. It’s just not resulting in the behaviour you expect.

Could you define “add” for me? What are the axioms of an add? Keep in mind that the public type is a

Provider<List<T>>

and not a

Provider<List<Provider<List<T>>>

.

if your state of the listproperty is

list [ entry1, provider { entry2 } ]

and you add an empty provider, the result is

list [ entry1, provider { entry2 }, provider { } ]

that looks awfully like a

List<Provider<List<T>>

or maybe a

List<Provider<T>>

but not a

Provider<List<T>>

but from the consumer’s perspective the type is

Provider<List<T>>

, the fact that there’s lazy resolution involved is an internal implementation detail, which granted leads to confusing behaviour.

Right… but if we want to talk about correctness (and bugs) in the API… then we have to push all of the implementation details out of our minds. Once we fix the API (if we can) we’re just going to reclassify all those implementation details as bugs. To my mind this API is broken, the only question is whether it’s broken enough to justify fixing it.

I can very much agree with that. Probably the least surprising behaviour when dealing with the listproperty in isolation would be to suppress errors from “internal” providers that are empty and iterate as if they don’t exist. But that could lead to inconsistent behaviour elsewhere these empty providers are used. I’m struggling to envision correct behaviour on this.

Circling back to this finally: https://github.com/gradle/gradle/issues/20266