@union bases can be used as @rule params

[cosmicexplorer]

Problem

@union and UnionRule make it possible to yield Get(A, B, c), where A is a concrete type, but B is a @union type, and C = type(c) is some unknown type which was registered with UnionRule(B, C) somewhere. This allows for dynamic dispatch on the caller end, but "unions" as a concept don't yet apply to the definition of @rules themselves or their arguments.

Solution

• If an @rule takes a parameter B which is an @union base, that will get converted into a version of the rule for each defined union member C.

Result

@rules can now be defined which accept a @union.

[coveralls]

Coverage remained the same at 0.0% when pulling edaf9f6 on cosmicexplorer:union-rule-args into 219cd42 on pantsbuild:master.

[benjyw]

Revisiting this, I'm not sure what actual problem it solves? I greatly prefer to focus our effort on end-user features, performance and stability, and any refactoring etc. should be in support of those. Does this tie into such a thing?

[cosmicexplorer]

It would mean we could avoid duplicating the logic that currently exists to process TransitiveDependenciesRequest and TransitiveDependenciesRequestLite in transitive_targets() and transitive_targets_lite() in graph.py. See discussion at #10923 (comment). I have also updated the description. I will demonstrate that this can work around #10917.

[Eric-Arellano]

It would mean we could avoid duplicating the logic that currently exists to process TransitiveDependenciesRequest and TransitiveDependenciesRequestLite in transitive_targets() and transitive_targets_lite() in graph.py.

Keep in mind that this duplication is only due to a graph bug. We don't intend to duplicate two rules like this; it's a hacky workaround. If we wanted to, we could have factored code up. But because it's a hacky workaround, I didn't worry too much about DRY.

[cosmicexplorer]

It was an example. There are other ones. We currently wrap every single @union base in a "Request" object before applying an await Get(...) that actually performs the polymorphism. This lets us avoid that indirection and reduce the number of classes a plugin developer has to import.

[cosmicexplorer]

@benjyw:

I greatly prefer to focus our effort on end-user features

I'm not proposing anyone spend any effort on this, unless it is outside of your power to review a +33/-2 line change. I don't see how this applies. I will add a few test cases to demonstrate this is useful, if that's what you mean.

I also don't think that the priorities of Toolchain Labs should justify blocking other contributions, if that's instead what you mean. Right now, there is no other way I can have features like this in pants besides making a pull request to add it. If you think the feature negatively affects the plugin API, that is a reasonable argument. If you personally don't care about it, but are otherwise neutral, then why does it matter if it's merged?

[cosmicexplorer]

And if you don't actually have the time to review right now, but will later, after the 2.0 release is out, then please just make that clear. I have no issue waiting a few weeks for any changes like this. I just want a better answer than "does this actually solve a problem" for a PR that's been open for a year from an active contributor with absolutely no feedback despite multiple requests for it. It makes me feel very hurt to hear that.

[benjyw]

I'm sorry that you feel that "does this actually solve a problem" is not a valid concern for a PR. In my mind it is the most valid concern for any PR (in the spirit of "features are an asset, code is a liability"). I would gently remind you that the PR template that starts with "Problem:" was your creation. My guess is that this change hasn't been reviewed for so long because no reviewer had visibility into the motivation for its creation in the first place.

In other words, the "Problem:" statement in the PR description doesn't seem to describe anything that I currently understand to be a problem.

Now, this followup comment:

"We currently wrap every single @union base in a "Request" object before applying an await Get(...) that actually performs > the polymorphism. This lets us avoid that indirection and reduce the number of classes a plugin developer has to import."

DOES sound more like a good premise for this change. Still a bit abstract though. I think a before/after example would be really helpful in motivating this change. Maybe the only thing that needs reworking here is the PR description.

[benjyw]

More generally, it's good to remember that an OSS project is still a project. It still has priorities and a roadmap and goals and users and issues. PRs motivated by those concerns are more likely to get reviewed and merged than changes motivated by general-purpose code improvement (which is often in the eye of the beholder). Every change has a cost - in flux, in potential disruption, in reviewer time, in ongoing maintenance - and that cost needs to be justified.

Feel free to reach out to me by DM if you'd like to discuss further.

[thejcannon]

@cosmicexplorer I'm gonna close this stale PR. GitHub ensures it is immortalized, so if it ever needs to be picked up again, your changes haven't gone anywhere.