Lint visibility rules for stale rule selectors

[AlexTereshenkov]

Is your feature request related to a problem? Please describe.

When declaring visibility rules for build targets, it's possible that a particular selector rule was applicable at the time of definition. E.g. src/project/app.py at some point needed to depend on src/libA/ which is why it was listed among allowed dependencies:

__dependencies_rules__(
    (
        "src/project/app.py",
        [
            "//src/libA/**",
            "//src/libB/**",
            "!*",
        ],
    ),
)

However, imagine that over time a decision has been made that this app should not depend on libA and the refactoring was done. The engineer who made the refactoring forgot to update the visibility rules so now they don't really reflect the real restrictions imposed on the app.

Having this rule stale means that someone can again start depending on the libA and the build would pass since the visibility rules are not violated. There should be a way to highlight the presence of stale rules (applies both to dependencies and dependents rules).

Describe the solution you'd like

It might be helpful to make sure that the rules reflect the current state of the dependency graph, i.e. if app.py is said to be allowed to depend on a libA and libB, this should be reflected in the graph, that is, there should be actual dependencies discovered. If that's not the case, a user would get a warning/failure during the build forcing them to keep the visibility rules up-to-date.

It's possible that having visibility rules that don't directly reflect the current state will be acceptable which is why linting for this should be configurable (the usual ignore, warn, error choice perhaps), e.g.

$ pants lint --only=visibility --redundant-rules=warning

Describe alternatives you've considered

This information is currently available via the peek goal:

$ pants peek --include-dep-rules src/project::

and the output can be processed to collect all dependencies of all the modules under a package for which a rule applies and identify whether there are any redundancies in the visibility rules. However, this would require every user to write JSON processing programs whereas this information is readily available in memory when evaluating the visibility rules.

[AlexTereshenkov]

@kaos what do you think of this? :)

[kaos]

I can see this being a thing, for sure. However I think it's tricky to get this right and a global flag is likely too coarse, as you'll likely have general/generic rules that are OK if they aren't currently enforcing anything while at the same time in other parts have specific rules that should be pruned if no longer applicable.

I imagine having some kind of marker for each rule, that indicates if it is a general rule, or more ephemeral, or "specific" (meaning, it should go away if it becomes un-useful).

Perhaps this marker could be inspired by regular expressions, so a leading + could be used to indicate a specific rule (i.e. should be applicable for 1 or more targets), while the default is for general rules (i.e. similar to regexp *; applicable for 0 or more targets).

__dependencies_rules__(
    (
        "src/project/app.py",
        [
            "+//src/libA/**",  # specific rule, error if there are no deps on modules in libA
            "//src/libB/**",  # general rule, will not complain regardless of what deps there are
            "!*",

            # for completeness, the dict version could look like:
            {
               "path": "//src/libA/**",
               "specific": True,
            },
        ],
    ),
)

This syntax has the added benefit of being backwards compatible, defaulting to all rules being general/non-specific by default, not breaking existing users. (assuming there are no path rules beginning with +.. 😬)

Then is the matter of how to check these rules' applicability, but that's mostly an implementation detail.

[AlexTereshenkov]

Excellent feedback, Andreas. You are absolutely right that there will most likely be rules that should be around even though a target doesn't depend on something out of it. So some kind of separation would indeed be necessary if we want to be able to distinguish between them at all. I'll think more about it :)