Visibility: support dict rules same way as for selectors. (#20769)

Closes #20733 
Clarifies docs for #20732

---------

Co-authored-by: Jacob Floyd <[email protected]>

docs/docs/using-pants/validating-dependencies.mdx

@@ -9,6 +9,8 @@ Validating your code's dependencies.
 
 Visibility rules are the mechanism by which to control who may depend on whom. It is an implementation of Pants's dependency rules API. With these rules a dependency between two files (or targets) may be set for entire directory trees down to single files. A target may be selected not only by its file path but also by target type, name, and tags.
 
+NB: Visibility rules are applied on a low level, so they can inspect metadata provided directly on a target and via `__defaults__`, but anything applied via `overrides` is opaque to the visibility rules. For example, a rule that selects tagged targets can find tags provided directly on the target using the `tags` field (like `target(tags=[...])`) or via `__defaults__` (like `__defaults__({target:dict(tags=[...])})`) but any tags in `overrides` (like `target(overrides={"foo": dict(tags=[...])})`) cannot be inspected by the visibility rules.
+
 To jump right in, start with [enabling the backend](./validating-dependencies.mdx#enable-visibility-backend) and add some rules to your BUILD files.
 
 ## Example visibility rules
@@ -203,13 +205,13 @@ __dependencies_rules__(
 
 The selector and rule share a common syntax (refered to as a **target rule spec**), that is a dictionary value with properties describing what targets it applies to. Together, this pair of selector(s) and rules is called a **rule set**. A rule set may have multiple selectors wrapped in a list/tuple and the rules may be spread out or grouped in any fashion. Grouping rules like this makes it easier to re-use/insert rules from macros or similar.
 
-:::note An empty selector (`{}` or `""`) will never match anything and is as such pointless and will result in an error.
+:::note An empty selector (`{}` or `""`) will never match anything and is as such pointless and is ignored.
 For every dependency link, only a single set of rules will ever be applied. The first rule set
 with a matching selector will be the only one used and any remaining rule sets are never
 consulted.
 :::
 
-The **target rule spec** has four properties: `type`, `name`, `tags`, and `path`. From the above example, when determining which rule set to apply for the dependencies of `src/a/main.py` Pants will look for the first selector for `src/a/BUILD` that satisifies the properties `type=python_sources`, `tags=["apps"]`, and `path=src/a/main.py`. The selection is based on exclusion so only when there is a property value and it doesn't match the target's property it will move on to the next selector; the lack of a property will be considered to match anything. Consequently an empty target spec would match all targets, but this is disallowed and will raise an error if used because it is conceptually not very clear when reading the rules.
+The **target rule spec** has four (five for rules) properties: `type`, `name`, `tags`, `path`, and `action` (only rules consult `action`). The `action` is one of `allow` (default if not specified), `warn`, or `deny`. From the above example, when determining which rule set to apply for the dependencies of `src/a/main.py` Pants will look for the first selector for `src/a/BUILD` that satisifies the properties `type=python_sources`, `tags=["apps"]`, and `path=src/a/main.py`. The selection is based on exclusion so only when there is a property value and it doesn't match the target's property it will move on to the next selector; the lack of a property will be considered to match anything. Consequently an empty target spec would match all targets, but this is disallowed and will raise an error if used because it is conceptually not very clear when reading the rules.
 
 The values of a **target rule spec** supports wildcard patterns (or globs) in order to have a single selector match multiple different targets, as described in [glob syntax](./key-concepts/targets-and-build-files.mdx#glob-syntax). When listing multiple values for the `tags` property, the target must have all of them in order to match. Spread the tags over multiple selectors in order to switch from _AND_ to _OR_ as required. The target `type` to match against will be that of the type used in the BUILD file, as the path (and target address) may refer to a generated target it is the target generators type that will be used during the selector matching process.
 
@@ -289,6 +291,25 @@ The previous example, using this alternative syntax for the selectors, would loo
   )
 ```
 
+Similarily, the rules may also be expressed using the dict syntax:
+
+```python
+  (
+    ( # Selectors block
+      ...
+    ),
+    (  # Grouping rules for readability
+      (  # Deny rules
+        {"path": "tests/**", "action": "deny"},  # No tests
+        {"path": "src/*/*/**", "action": "deny"}, # Nothing deeply nested
+      ),
+      (  # Allow rules
+        {"path": "*"},  # Allow everything else (allow is default action)
+      ),
+    )
+  )
+```
+
 ### Glob syntax
 
 The visibility rules are all about matching globs. There are two wildcards: the `*` matches anything except `/`, and the `**` matches anything including `/`. (For paths that is non-recursive and recursive globbing respectively.)

src/python/pants/backend/visibility/rule_types.py

@@ -68,20 +68,25 @@ class VisibilityRule:
     @classmethod
     def parse(
         cls,
-        rule: str,
+        rule: str | dict,
         relpath: str,
     ) -> VisibilityRule:
-        if not isinstance(rule, str):
-            raise ValueError(f"expected a path pattern string but got: {rule!r}")
-        if rule.startswith("!"):
-            action = DependencyRuleAction.DENY
-            pattern = rule[1:]
-        elif rule.startswith("?"):
-            action = DependencyRuleAction.WARN
-            pattern = rule[1:]
-        else:
-            action = DependencyRuleAction.ALLOW
+        pattern: str | dict
+        if isinstance(rule, str):
+            if rule.startswith("!"):
+                action = DependencyRuleAction.DENY
+                pattern = rule[1:]
+            elif rule.startswith("?"):
+                action = DependencyRuleAction.WARN
+                pattern = rule[1:]
+            else:
+                action = DependencyRuleAction.ALLOW
+                pattern = rule
+        elif isinstance(rule, dict):
+            action = DependencyRuleAction(rule.get("action", "allow"))
             pattern = rule
+        else:
+            raise ValueError(f"invalid visibility rule: {rule!r}")
         return cls(action, TargetGlob.parse(pattern, relpath))
 
     def match(self, address: Address, adaptor: TargetAdaptor, relpath: str) -> bool:
@@ -153,7 +158,7 @@ def parse(cls, build_file: str, arg: Any) -> VisibilityRuleSet:
         relpath = os.path.dirname(build_file)
         try:
             selectors = cast("Iterator[str | dict]", flatten(arg[0], str, dict))
-            rules = cast("Iterator[str]", flatten(arg[1:], str))
+            rules = cast("Iterator[str | dict]", flatten(arg[1:], str, dict))
             return cls(
                 build_file,
                 tuple(TargetGlob.parse(selector, relpath) for selector in selectors),
@@ -173,8 +178,8 @@ def peek(self) -> tuple[str, ...]:
         return tuple(map(str, self.rules))
 
     @staticmethod
-    def _noop_rule(rule: str) -> bool:
-        return not rule or rule.startswith("#")
+    def _noop_rule(rule: str | dict) -> bool:
+        return not rule or isinstance(rule, str) and rule.startswith("#")
 
     def match(self, address: Address, adaptor: TargetAdaptor, relpath: str) -> bool:
         return any(selector.match(address, adaptor, relpath) for selector in self.selectors)

src/python/pants/backend/visibility/rule_types_test.py

@@ -45,7 +45,7 @@ def parse_address(raw: str, description_of_origin: str = repr("test"), **kwargs)
     return parsed.dir_to_address() if "." not in raw else parsed.file_to_address()
 
 
-def parse_rule(rule: str, relpath: str = "test/path") -> VisibilityRule:
+def parse_rule(rule: str | dict, relpath: str = "test/path") -> VisibilityRule:
     return VisibilityRule.parse(rule, relpath)
 
 
@@ -158,6 +158,7 @@ def test_flatten(expected, xs) -> None:
         (True, "<target>", "src/a", ""),
         (False, "<target>[src/b]", "src/a", ""),
         (False, "<file>", "src/a", ""),
+        (True, {"path": "src/a"}, "src/a", ""),
     ],
 )
 def test_visibility_rule(expected: bool, spec: str, path: str, relpath: str) -> None:
@@ -206,6 +207,23 @@ def test_visibility_rule(expected: bool, spec: str, path: str, relpath: str) ->
             ),
             ("<target>", "src/*", "src/a:lib"),
         ),
+        (
+            VisibilityRuleSet(
+                build_file="test/path/BUILD",
+                selectors=(TargetGlob.parse("<target>", ""),),
+                rules=(
+                    parse_rule("!src/*"),
+                    parse_rule("?src/a:lib"),
+                    parse_rule("(this, is, ok)"),
+                ),
+            ),
+            (
+                "<target>",
+                {"action": "deny", "path": "src/*"},
+                {"action": "warn", "path": "src/a", "name": "lib"},
+                {"tags": ["this", "is", "ok"]},
+            ),
+        ),
     ],
 )
 def test_visibility_rule_set_parse(expected: VisibilityRuleSet, arg: Any) -> None: