No value ListProperty and SetProperty shouldn't allow adding elements

[lacasseio]

Expected Behavior

ListProperty<String> property = project.objects.listProperty(String)
property.add("some-value") // fail and all variant should as well

Current Behavior

ListProperty<String> property = project.objects.listProperty(String)
property.add("some-value")
property.get() // fail with no value

Context

See: #7472

Given that the collectors list gets cleared when a value is set, wouldn't it be better to fail upon add and addAll on a ListProperty with no value? We can check if the value is NO_VALUE_COLLECTOR to assert this mutation. I went as far as running the debugger and saw that both collectors (no value and the one I added) were present and still couldn't obviously found the issue.

Steps to Reproduce (for bugs)

Your Environment

• Build scan URL:

[oehme]

It seems really confusing to me that a List property starts out with "no value", just like I would never expect a List-returning method to return null. Shouldn't it be an empty list by default and fail if I try to null it?

[lacasseio]

Just a though, not arguing against. If we consider every other properties, we allow them to have no value to force people to set a least a default value. With that regards, it does make sense to start out with no value.

[eskatos]

What about allowing build logic authors to say either "this is a property of type X and its initial value is Y", or, "this is a property of type X and please use the default value for this type as initial value"
where the latter sets boolean properties to false, numbers to zero, collections to empty collections etc.. ? distinguishing "initial value" and "default value".

[lacasseio]

@eskatos The initial value can be set using Property#value(T) or, in this case, ListProperty#empty(). I think it's good to have no value when the Property is created, but we should throw IllegalStateException for invalid operations.

[eskatos]

Yep, agreed @lacasseio. I was suggesting to go a bit further and provide a way to say "use the default for this type as initial value". But I guess that explicit initial value with Property#value(T) or the ListProperty#empty() is good enough.

[big-guy]

We also want to make clear the difference between "this is the initial value" and "this is the default". if the default is hard/expensive to calculate, we want to only use it when no other opinion has been given.

[JLLeitschuh]

Related to the issue that @big-guy and I spent 45 min debugging when I was changing the AbstractArchiveTask to expose the provide AP in #7435.

The JvmComponentPlugin set its own default value and then the BasePlugin tries to set it's own default.

The fix required adding a check to see if the destinationDirectory was already set.

gradle/subprojects/plugins/src/main/java/org/gradle/api/plugins/BasePlugin.java

Lines 90 to 92 in d09ae3c

	// This is here because the JvmComponentPlugin Jar task configuration runs before this action.
	// It has already set the destination directory for this task. The BasePlugin needs to respect that.
	if (!task.getDestinationDirectory().isPresent()) {

Would it be valuable on the Provider to be able to differentiate whether the value you would get by calling Provider::get would be the default value or a value set elsewhere?

Something like Provider::isDefaultValue?

[wolfs]

@lacasseio Do we have some discussion why collection properties should start out with no value? I would have expected that collection properties always have a value, the value being empty if no elements have been added. For the same reasons I expect any method which returns a List<T> to never return null.

I don't see much value in distinguishing between the "not present" value and the "empty" value of a collection property. What is the use case for it?

We also have a ConfigurableFileCollection which is the equivalent to ListProperty<File>, right? That also starts out as empty, do we want to change this, too?

[lacasseio]

That is a great question. If I remember correctly, it started out when we removed the default values of the property of the primitive box type. @adammurdoch can give more feedback on this decision.

[mkobit]

This is also a breaking change in 5.0-rc-1 for early adopters. I find the new behavior for lists not being an empty collection by default surprising and counterintuitive. Now, whenever I create a ListProperty I need to essentially call .empty() each time.

[mkobit]

Any considerations for changing the behavior back before 5.0? I haven't seen anything related to this, so I assume this behavior would go forward since there are already existing release candidates out.

[mkobit]

Please consider changing the behavior, or at least fixing this issue. Even though I knew about this issue and commented on it several times I still missed a few areas in our plugins that we released in support for 5.0 that break in surprising user-facing ways.

Given a plugin that has an optional list property:

    @get:Input
    @get:Optional
    val things: ListProperty<String> = objectFactory.listProperty()

A user can add to that list:

things.add("thing")
things.add(provider { "thing2" })

But the value is considered not present:

println(things.isPresent) // false

So, if a plugin implementation doesn't initialize the property to empty(), then their input is never even considered if the implementation checks for presence before consuming the value. It requires more burden on either build authors to call .empty() or .set(...), or on plugin authors to understand the implications of @Optional properties and when to use .empty().

I can sort of understand the consistency across properties around having "no value", but the behavior today is surprising and seems needlessly complex. What benefits are there to having multi-valued properties like ListProperty, SetProperty, and eventually MapProperty to behave this way?

[adammurdoch]

We'll try for a fix in 5.1, probably by just changing the default back to an empty collection and maybe some way for a plugin to create a collection property with no value if it really wants to.

[adammurdoch]

Default value is now an empty list/set/map: #7963

[martinbonnin]

I'm reviving this thread because even after re-reading it several times, it's unclear what the expected behavior should be. I don't mind the default value being an empty list but I need to be able to distinguish between EMPTY vs UNSET because a user might override a previously set property with an empty one.

Right now I'm doing this:

mapProperty.set(null as Map<String, String>?) // unset the property
mapProperty.put("key", "value") // I would expect that to either throw an Error or set a value
mapProperty.isPresent // false => how can that be while I just put something in my property ?

Can we make .put() behave more consistently and have it set the property if unset ?

See apollographql/apollo-kotlin#1987 for more context.