Decide what to do for plugins modifying upgraded properties at execution time

[asodja]

It seems some plugins modify property value in doFirst, e.g. ErrorPronePlugin does:

tasks.withType<JavaCompile>().configureEach {
  doFirst("Configure forking for errorprone") {
     options.isFork = true
  }
}

see source.

And after property upgrade we get:

* What went wrong:
Execution failed for task ':compileJava'.
> The value for task ':compileJava' property 'options' property 'fork' is final and cannot be changed any further.

Should we/can we handle that somehow? I guess in the end such plugins will need an update to be compatible

Let's decide what to do here for old plugins. Right now it seems difficult to make such plugins work with property upgrades.

Should we already display some warning at execution time even for "raw" type properties?

[asodja]

We actually do the same in our code:

gradle/subprojects/language-java/src/main/java/org/gradle/api/tasks/compile/JavaCompile.java

Lines 239 to 241 in b9ac289

	if (isModule && !isSourcepathUserDefined) {
	compileOptions.setSourcepath(getProjectLayout().files(sourcesRoots));
	}

This is related to finalization of properties.

[lptr]

I think this is a good thing. We introduced property finalization partially because we wanted to prevent this very use case. We couldn't do it for JavaBean properties, but by converting them to lazy props we get this validation.

If you look at it like that (i.e. a new kind of validation), it is clear what we should do: introduce it as a deprecation warning, and break in the next major.

We can implement this by treating @UpgradedPropertys specially, so if someone tries to modify their value we can emit a deprecation warning. We might already have some functionality implemented to do this... 🤔

[asodja]

So ErrorProne basically wants to make sure fork = true is really used. So it try to configure the task last with doFirst:

tasks.withType<JavaCompile>().configureEach {
  doFirst("Configure forking for errorprone") {
     options.isFork = true
  }
}

I guess with Property solution would be:

tasks.withType<JavaCompile>().configureEach {
     options.isFork = true
     options.isFork.finalizeValue()
}

I wonder if we should have Property.finalizeValue that fails on change and Property.finalizeValueNoException that would just warn if value tries to be changed.

[lptr]

Discussed this with @adammurdoch today. Tasks not modifying their inputs is definitely our goal in the foreseeable future. We should not break tasks that do this without warning, though. There seems to be two approaches available to us:

1. For @UpgradedPropertys we initially only "soft-finalize", i.e. we show deprecation when property is modified after finalization. In the next major we change the deprecation to an exception (=hard finalization) as usual. Note that we already had this kind of soft-finalization in place when we originally started finalizing Property inputs, so it is probably quite easy to reinstate this for @UpgradedPropertys.

2. We instrument setters and start soft-finalizing them very soon. Doing means we can already do hard finalization when upgrading properties in the next major.

An additional benefit of 2) is that it applies to every JavaBean property, e.g. to properties of custom tasks introduced in builds etc. so we can phase out all the bad usage patterns out there. Meanwhile 1) only addresses bad usage patterns for @UpgradedPropertys.

OTOH, with 2) we would only address modifications via setters. For example, it wouldn't cover mutating multi-valued properties like lists wrt adding/removing items. Deprecating these mutations too could be possible, but would mean more work.

[lptr]

ErrorProne should probably configure fork = true via normal configuration, and then add a check during runtime, and fail if the user somehow forced it back to false.

[asodja]

I've opened tbroyer/gradle-errorprone-plugin#97 for ErrorProne.

[lptr]

Can we close this now since we introduced lenient finalization for upgraded properties?

[asodja]

Yes we can, thanks!

We implemented lenient finalization for migrated properties in #30715, that fixes also this issue.