Add --transitive flag for all linters

[stuhood]

We should add a --[no]-transitive flag for all lint tasks to support the usecase where you're running directly on changed targets.

Running directly on changed targets happens when you use ./pants --changed lint (for any setting of the --changed-include-dependees option): in that case, you know that only files directly owned by a target are affected. It also happens during arc lint.

Because this affects so many tasks, it seems likely that it should go on a (perhaps lint specific?) Subsystem and be consumed there.

[stuhood]

If this were a scoped subsystem, the scope of the subsystem would get injected into the option, so tasks that already have a --transitive flag would need to change. Assuming a scope name of foobar, the options would need to change to something like:

./pants scalafmt --no-foobar-transitive ..

...which is unfortunate.

And actually, if it were a global subsystem, it would still end up being named --no-foobar-transitive... so the choice of global/scoped here only seems to affect whether it's possible to override for one linter and not another.

[stuhood]

cc @benjyw : thoughts on how to do this, option wise?

[benjyw]

Global option that each task can override in its scope?

Then we could potentially implement globally by using this flag to determine whether invalidation returns the closure or just the root targets?

[stuhood]

@benjyw : I think I agree that it might qualify as a global option... certainly as we move toward #4769 and Tasks need to begin to declare which targets they operate on more precisely. But something to be done very carefully.

[benjyw]

If it's a global option, what happens with tasks such as compile.java, which should always operate on the transitive closure.

Perhaps, instead of a subsystem, this is defined on a task mixin (CanOperateIntransitively, or some much better name than that)? Then individual tasks can easily opt in to this behavior, and we can still fold the usage into invalidation?

[stuhood]

Yea, neither global nor scoped seem like great options. The hope/dream of placing in a scoped subsystem named "foobar" would be that ./pants --no-foobar-transitive lint would affect all of the lint tasks in one go. The unfortunate sideeffect of that though, would be that any other tasks
with a dependency on the subsystem (including compile.java, if it added a dependency there) would be affected.

Would it be feasible to declare the option on a subsystem and then scope the subsystem using the goal name? ie, --no-lint-transitive?

[benjyw]

It should be, but we've never done anything like that before. I'll give it a whirl.

If not, it doesn't seem too terrible if we have to set this task-by-task.

[benjyw]

Actually, I think it's better to set this per-task, on the task's options scope.

A) Users already understand this (or at least, should).

B) In some cases the transitivity concept doesn't apply to every task in a goal. For example, findbugs is currently registered on compile, but --compile-transitive is just confusing, as it doesn't apply to any other compilation task.

C) Existing --transitive options don't have to be migrated.

How important do you think it is to be able to do --lint-transitive instead of --lint-pythonstyle-transitive etc etc?

[benjyw]

See #5303.

[stuhood]

How important do you think it is to be able to do --lint-transitive instead of --lint-pythonstyle-transitive etc etc?

Well, see the original description. The usecase is that if you have a dozen linters (...pants already does), and you have already determined a precise set of affected targets via changed, you want to set the option for all of them.

The (best?) way to do this currently is to have a pants.lint.ini file that either 1) sets a [default] value for the field (and hopes that you have name-wise alignment), or 2) set it for each task individually.

[benjyw]

Well, if the default for --transitive is false, you basically get that with a per-task option, no? For linters, at least, I can't imagine that wanting to run things transitively is going to be that common.

[stuhood]

During a typical build you need to check lints transitively... it's only when you've already determined a precise set of targets to lint that you don't want transitivity.

[benjyw]

OK, so to pull the ideas into one place, possibilities include:

1. A per-task option, and you just have to flip this on or off per-task. Advantage - existing flags don't change.

2. A recursive global option, which tasks can ignore if recursiveness isn't relevant to them. A bit awkward. Existing flags don't change.

3. A recursive option at the lint goal scope (not a Subsystem, just a recursive registration). This is something we haven't done before, but "should work". It would mean moving all the lint-ish tasks that are currently in compile to lint. Then the flag is, e.g., --[no-]lint-errorprone-recursive, but the ./pants lint.errorprone --recursive shortcut works. Can't be reused for non-lint tasks. Existing flags must be migrated. Not sure what this does to the help text generator etc.

4. A subsystem, with a scoped instance for each task. Say the subsystem is called 'TargetRange', then the flag is, e.g., --[no-]-targetrange-lint-errorprone-recursive etc. Verbose, and there's no shortcut. But can be set for all tasks at once succinctly: --[no-]targetrange-recursive. Can be used by all tasks for which recursiveness is a relevant concept. Existing flags must be migrated.

[benjyw]

@stuhood your preference/other ideas?

[stuhood]

Not a preference, but similar to 3:

5. A global Lint subsystem that shadows the lint goal. Linters would explicitly depend on it.

I think either 3 or 4 accomplish the goal, although 4 probably results in a bit more boilerplate to explicitly depend on the subsystem? Having the option implicitly present due to the recursion is a bit magical.

Do values set for recursive options propagate downward to all subscopes? IE would setting --lint-transitive affect all lint tasks the same way it would for a scoped subsystem?

[stuhood]

(sorry, made a few edits to the above that may not have been reflected in email)

[benjyw]

I believe so, yes. I don't like the idea of shadowing the lint scope with a subsystem. The unintended consequences might be horrifyingly confusing. I'd be pretty strongly opposed to that.

[benjyw]

The more I think about it, the more I'm in favor of a global option. The concept of acting transitively or not is fairly widely applicable. For example, it could make sense to run tests either just on target roots or transitively. In practice test targets are unlikely to depend on other test targets, but the concept is at least coherent.

[benjyw]

By the way, does this end up getting complicated by scopes? Is the transitive option actually a whitelist or blacklist of scopes? I guess we can let each target decide.

[benjyw]

Implemented in #5383.