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

Allow Provider#toString() to be consider as an error #16948

Open
lacasseio opened this issue Apr 22, 2021 · 4 comments
Open

Allow Provider#toString() to be consider as an error #16948

lacasseio opened this issue Apr 22, 2021 · 4 comments
Labels
a:feature A new functionality in:provider-api property lazy provider MapProperty ListProperty DirectoryProperty p:lazy-migration Issues covered by migration to an all-lazy API

Comments

@lacasseio
Copy link
Contributor

lacasseio commented Apr 22, 2021

In almost all use cases, calling Provider#toString() is the wrong intention. When migrating builds to use the provider API, there are a lot of cases where build authors would build strings out of property that now becomes providers. For example, in Groovy, code could look like this: propA = "${buildDir}/${propB}" where propA and propB are converted into Provider(s)/Property(s). We can think of other real use cases where the execution flow would implicitly and naturally call Object#toString(). The problem here is the implicit call to Provider#toString() is almost certainly a bug if not properly accounted for. Taking the previous example, the real intention would be something like the following: propA = propB.map { "${buildDir}/${it}" }.

It is understandable that Provider#toString() cannot rightfully throw an exception given the Object#toString() contract. However, there should be a way that Gradle provides to ease the transition and catch those cases where the intent is wrong.

Note: I'm well aware that some cases can be rewritten in different ways just like the example above but often it's unfeasible to rewrite or rethink all of the build logic. An incremental approach is much easier to implement, has a higher chance of success, easier to review, etc. The original code is most often good enough but lacks the capability to defer any execution to speed up configuration time.

Expected Behavior

Gradle can and should notify users when this particular case arises to avoid wrong intentions as well as making the transition easier. We can solve this in various ways, two of which are:

  1. Parse the compiled AST of build scripts and plugin code for calls to Provider#toString() (cases, where the object type is only known at runtime, may not be possible to catch)
  2. Provide a feature flag to break the Provider#toString() contract and fail when called.

Current Behavior

When adopting Gradle code to use the provider API, implicit calls to Provider#toString() are easily missed producing wrong results.

Context

Anyone adopting the provider API will most certainly work through bugs caused by a similar scenario.

@lacasseio lacasseio added a:feature A new functionality to-triage labels Apr 22, 2021
@bmuskalla bmuskalla added in:provider-api property lazy provider MapProperty ListProperty DirectoryProperty and removed to-triage labels May 3, 2021
@stale
Copy link

stale bot commented Jun 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be automatically closed if no further activity occurs. If you're interested in how we try to keep the backlog in a healthy state, please read our blog post on how we refine our backlog. If you feel this is something you could contribute, please have a look at our Contributor Guide. Thank you for your contribution.

@stale stale bot added the stale label Jun 12, 2022
@lacasseio
Copy link
Contributor Author

Please don't close this! This would be a major usability improvement for Provider.

@stale stale bot removed the stale label Jun 24, 2022
@jbartok jbartok added the p:lazy-migration Issues covered by migration to an all-lazy API label May 25, 2023
@ljacomet
Copy link
Member

In the context of deprecating Project.buildDir in favour of Project.layout.buildDirectory, I ended up having to replace things like:
project.file("$project.buildDir/$name")
with
project.layout.buildDirectory.dir("$name").get().asFile

which is very different although making a lot of sense when you get your head around it. (yes the .get().asFile is not nice but also needed when what you assign only expects a File)

As part of that work, I was tripped a couple times initially by this toString issue when attempting to do things like:
project.file("$project.layout.buildDirectory/$name") which does not work at all.

I wonder if having toString being get().toString() would help these kind of migrations. Or if it would actually hamper adopting the (better) chain based logic.

Anyway, sharing this as one more element for the resolution of that issue.

@lptr
Copy link
Member

lptr commented Nov 30, 2023

Just a quick thought: we could deprecate the toString() method, so at least it stands out in the IDE (when the IDE understands the type).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:feature A new functionality in:provider-api property lazy provider MapProperty ListProperty DirectoryProperty p:lazy-migration Issues covered by migration to an all-lazy API
Projects
None yet
Development

No branches or pull requests

6 participants