From `unions.py`:

Subclassed union bases should inherit the superclass's union members.

but...

@union
class Base: ...

@union
class Sub(Base): ...

class BaseInput(Base): ...
class SubInput(Sub): ...
class Result: ...

# ...

UnionRule(Sub, SubInput)
UnionRule(Base, BaseInput)

# ...

await Get(Result, Sub(...))

is complaining:

No installed rules return the type Result, and it was not provided by potential callers of Query(Result for BaseInput).

Sure enough, if I print the membership of

Sub

I see

<class BaseInput>

.

I guess I have to invert my type hierarchy. Seems goofy tho 🤔

So, at a minimum I think the comment has a typo/misnomer. It should read:

Union subclasses of union base classes should inherit the base class's members

What I want though is the reverse. I want a shared base that I can query and get all of its registered members AND the subclass registered unions. I could invert the type hierarchy, but then my type hints are all backwards as well and the code gets confusing

Actually I think this code has a bug anyways...

for sub_union in union_base.__subclasses__():
    ...

should be:

for sub_union in union_base.__subclasses__():
    if is_union(sub_union):
        ...

The result right now is the union membership has a lot of mapping from our union subclasses (E.g.

BlackRequest

) back to the parent (e.g.

FmtTargetsRequest

) members (so like every formatter...)

print(union_membership[BlackRequest])

09:56:48.25 [INFO] stdout: "FrozenOrderedSet([<class 'pants.backend.python.lint.autoflake.rules.AutoflakeRequest'>, <class 'pants.backend.python.lint.black.rules.BlackRequest'>, <class 'pants.backend.python.lint.docformatter.rules.DocformatterRequest'>, <class 'pants.backend.python.lint.isort.rules.IsortRequest'>, <class 'pants.backend.shell.lint.shfmt.rules.ShfmtRequest'>, <class 'pants.backend.go.lint.gofmt.rules.GofmtRequest'>, <class 'pants.backend.java.lint.google_java_format.rules.GoogleJavaFormatRequest'>, <class 'pants.backend.scala.lint.scalafmt.rules.ScalafmtRequest'>])"

womp womp, lol

And the good news, if we fix this bug, there are no sub unions today in our codebase. I'm going to suggest we invert the relationship. PR will be coming soonish

Funny enough, it's our convention that is causing this 😅 Very tiny (likely not noticeable) performance improvement incoming 😛

Proof:

$ git diff main
diff --git a/src/python/pants/engine/unions_test.py b/src/python/pants/engine/unions_test.py
index f302f180e..40b7d7f32 100644
--- a/src/python/pants/engine/unions_test.py
+++ b/src/python/pants/engine/unions_test.py
@@ -10,10 +10,10 @@ def test_union_membership_from_rules() -> None:
     class Base:
         pass
 
-    class A:
+    class A(Base):
         pass
 
-    class B:
+    class B(Base):
         pass
 
     assert UnionMembership.from_rules([UnionRule(Base, A), UnionRule(Base, B)]) == UnionMembership(

^ That fails because we also map

A

to

B

and vice-versa. Proof of fix:

$ git diff main
diff --git a/src/python/pants/engine/unions.py b/src/python/pants/engine/unions.py
index 13dd10c75..47389c956 100644
--- a/src/python/pants/engine/unions.py
+++ b/src/python/pants/engine/unions.py
@@ -77,9 +77,10 @@ class UnionMembership:
         while len(bases) > 0:
             union_base = bases.pop()
             for sub_union in union_base.__subclasses__():
-                if sub_union not in mapping:
-                    bases.append(sub_union)
-                mapping[sub_union].update(mapping[union_base])
+                if is_union(sub_union):
+                    if sub_union not in mapping:
+                        bases.append(sub_union)
+                    mapping[sub_union].update(mapping[union_base])
         return cls(mapping)
 
     def __init__(self, union_rules: Mapping[type, Iterable[type]]) -> None:

Now the test passes 😛

unions_test

is waaaaaaaaaaaaaaaaaaaaaaaaay too small for the complexity

@curved-television-6568 you implemented the code (and didn't update tests, tsk tsk) https://github.com/pantsbuild/pants/commit/643d8da78b6fbd0e57fcd94dd82e1077106f3d68#diff-517fa581d812162b8b4cf7e16[…]4613c961f83f74daa05a58855706fe6 From the PR:

Otherwise union.get(SomeTarget.PluginFields) wouldn't give you anything when you register a plugin field on Target.

I don't quite follow 😛

Ah OK I think I see. I'll need to invert or change

target.py

with the new behavior I think

Although, I think this does break the model for the Target code. Didn't know plugins were used here. Gonna have think on this one 🙂

So I think based on this use-case the code makes sense, and my use-case needs to flex a little bit. BUT I also think the code needs some TLC. I'll do both

When that is submitted I have a follow-up PR for my use-case?