Have each suppressible-error-prone `Mode` self-contain it's semantics

[kelvinou01]

Background

We recently introduced the concept of Modes. When certain modes are run together, they require special handling through ModeInterferences. This refactor significantly reduced the complexity of SuppressibleErrorPronePlugin.

However, one limitation of the current model is that it isn't immediately obvious, when reading just the mode and interference classes, what it means for the behavior of error-prone. For example, consider what happens when you run -PerrorProneApply and -PerrorProneSuppress together:

public final class ApplyMode implements Mode {
    @Override
    public CommonOptions configureAndReturnCommonOptions(ModeOptionContext context) {
        return new CommonOptions() {
            @Override
            public PatchChecksOption patchChecks() {
                return PatchChecksOption.someChecks(() -> checksToApplySuggestedPatchesFor(context));
            }
        };
    }

    private static Set<String> checksToApplySuggestedPatchesFor(ModeOptionContext context) {
        boolean hasSpecificPatchChecks =
                context.flagValue().isPresent() && !context.flagValue().get().isBlank();

        if (hasSpecificPatchChecks) {
            return Splitter.on(',')
                    .omitEmptyStrings()
                    .splitToStream(context.flagValue().get())
                    .map(String::trim)
                    .collect(Collectors.toSet());
        }

        return context.extension().patchChecksForCompilation(context.javaCompile());
    }
}

public final class SuppressingAndApplyingInterference extends SpecificModeInterference {
    @Override
    protected Set<ModeName> interferingModes() {
        return Set.of(ModeName.SUPPRESS, ModeName.APPLY);
    }

    @Override
    public CommonOptions interfere(Map<ModeName, CommonOptions> modeCommonOptions) {
        CommonOptions suppress = modeCommonOptions.get(ModeName.SUPPRESS);
        CommonOptions apply = modeCommonOptions.get(ModeName.APPLY);

        // If we're applying suggested patches at the same time as suppressing, we still need to tell
        // errorprone to patch all checks, so we can make suggested fixes for suppressions in any check.
        // However, inside our changes to errorprone, we need to get the list of checks that we're going
        // to use the default suggested fixes for, so we can work out which ones to use the suggested
        // fixes for and which to suppress. So we add the PreferPatchChecks argument here, which we can
        // use inside error-prone/the compiler.

        return suppress.naivelyCombinedWith(apply)
                .withExtraErrorProneCheckFlag(
                        "SuppressibleErrorProne:PreferPatchChecks",
                        () -> apply.patchChecks().asCommaSeparated().orElse(""));
    }
}

Without reading the entirety of VisitorStateModifications, it is difficult to understand what PreferPatchChecks and ErrorProneOptions#patchChecks entail.

Furthermore, -PerrorProneRemoveUnused will introduce an additional layer of complexity to VisitorStateModifications. Can we avoid having the modes depend on the implementation details of VisitorStateModifications?

Desired Behavior

Can we establish a clean API between VisitorStateModifications and Modes, such that the semantics of a mode (or a combination of modes) can be entirely determined from reading just the Mode and ModeInterferences?

Approach

Note

This issue is more of a "would be nice if we could achieve this", rather than "we're sure the current design is suboptimal and can be easily replaced by a better, simpler design.". I haven't come up with a solution yet.

Perhaps we can define a SuppressibleErrorProneOptions record that specifies:

• Which checks to autofix, if an autofix is available
• Which checks to suppress, if an autofix is not available
• Which checks to remove rollout for
• Which checks to remove suppressions for

VisitorStateModifications can consume this record to determine its behavior.

[kelvinou01]

Maybe a good first-step is to document each option in CommonOption thoroughly, and using examples of how we combine them to achieve the magic found in more tricky modes, e.g. Suppress + Apply