CC @curved-television-6568 who implemented this.

And for context: https://github.com/pantsbuild/pants/discussions/17253

And https://github.com/pantsbuild/pants/pull/16998

And the reason this is coming up now is an attempt to make the AddressMap and TargetAdaptor classes immutable, to solve https://github.com/pantsbuild/pants/issues/18250

Anyway, from eyeballing the code that handles `_extend_synthetic`, I think it is broken and has never worked.

And the reason we never noticed is that it’s not robustly tested, and we don’t use this feature at all in practice (nor do we document it anywhere)

The problem is that here we run

process_declared_targets()

in a double loop over all

address_maps

and all

synthetic_address_maps

. But that method mutates each input

address_map

on the first iteration of the inner loop, by popping

_extend_synthetic

off the kwargs. So in subsequent iterations that kwarg will not be present.

Therefore that kwarg will have an effect only if the

address_map

happens to match in the first

synthetic_address_map

Or… maybe I’m missing something, so I would like @curved-television-6568 to weigh in

The curse of mutability…

We use these data structures in situations where they are expected to be immutable, when they are not. So that is problem #1. And in this case it has also created problem #2, a bug where the feature itself doesn’t work.

However, as I mentioned, it looks like we don’t use this functionality anywhere

So, to simplify things, could/should we simply get rid of

_extend_synthetic

support?

If not, what is its use case?

First reflection just looking at the two lines of codes linked, I think that the double loop issue is not an issue most of the time, as you only end up popping

_extend_synthetic

more than once if you have >1 BUILD file in a directory. But I'll dig deeper.

Yea, ok. Mutating here is obviously a bad idea. Not sure how it came to be like that. My guess is I wanted to keep it as contained as possible (the synthetic feature) and not spread/leak into other parts of the code base. But I see what's going on, I think 😉

I think it do work however, as long as you have a single BUILD file (apart from the mutating bit, breaking

__hash__

or what-not.) -- so the tests are lacking a case using multiple BUILD files, which would've caught this. In my defense I don't recall we have any test with multiple BUILD files in a directory for any feature.. (there may be some around the core BUILD file parsing.. but that's it) so didn't occur to me to check for that here.

It not being documented is an oversight, and I think it is worth while to keep (I see the fix as not too difficult.) the purpose/idea of this, is to allow overriding/extending a synthetic target with additional field values from a target in an actual BUILD file (so the synthetic target can be augmented, acting as a "default" or "placeholder".) This may be important in certain use cases (outside of the pants repo.) ((I may be wrong, but I have a vague recollection that it may have been you, Benjy, asking for the ability to be able to override a synthetic target as it not be set in stone.)) Synthetic targets are documented here: https://www.pantsbuild.org/stable/docs/writing-plugins/the-target-api/concepts#synthetic-targets-api

The fix is obviously to stop mutating the target adaptor. As such, we'd need another mechanism to convey this intention. I think the default behaviour when this is not set, that a target in the BUILD file takes precedence, replacing the synthetic target completely. Oohh.. right. So it's not actually set in stone, then. It's just if you want the synthetic target, but with some extras... hmm. That's a bit.. niche. Perhaps. 🤔

I've thrown together an idea how we can address the above, but there's more. I left a comment here: https://github.com/pantsbuild/pants/pull/21732/files#r1874081758 and I'm currently out of ideas how to fix that. As the override/extend synthetic targets is kind of mutating by nature..

It may be we need to change how synthetic targets are added to the graph, in that case... not sure. 🤔 Ideas welcome.

I'll need to sleep on it, as it's getting late here. 🛏️

I’ll see how that all goes