Refresh Syntactic/SemanticDocument between rule evaluations to better handle overlapping patches

[bjaglin]

When several rules are requested on the same invocation (either explicitly or implicitly via the .scalafix.conf rules attribute), and there is at least one semantic rule that is not in the first position, that one might observe a source file that does not match the semanticDB available in the classpath (raising a StaleSemanticdbError), because another rule patched that file after compilation, the Syntactic/SemanticDocument passed as input is the same for all rules, regardless of patches applied by other rules. In other words, rules appear to run in parallel and not in the sequential order one would expect based on the config/CLI args. That causes unexpected Patch behavior when different rules touch the same tokens, leading to invalid source code after the invocation.

The workaround is to run these conflicting rules in separate invocations to force several patch applications.

Mitigation ideas:

1. We could fail when encountering overlapping patches and suggest to run scalafix several times.
2. We could change the processing model, to apply patches iteratively rather than once at the end. However, there are a few challenges:
   • Implementation is trivial when patches are actually applied on disk (ScalafixArguments.run() in IN_PLACE mode), but for other modes (ScalafixArguments.evaluate() or CHECK/STDOUT), we will need temporary files.
   • If there are semantic rules, we'll need to recompile sources (which is sanely only possible in IN_PLACE mode). ScalafixArguments would need to take an optional callback that would instruct the client to recompile sources after the patches of a rule is applied. For sbt-scalafix, given the execution model of sbt, the safe way to do that would be from a command since compile would have to be triggered multiple times within the same invocation. If we want the current input keys to benefit from that, we should investigate to which extend we could use Extracted#runTask, which has limitations.

[mlachkar]

If there are multiple rules, the fixed output is only written once at the end, and each rule.fix is computed on the same source file before the change.
I do agree that this error StaleSemanticdbError can be seen if you run twice scalafix on a file, and the first run has changed the file (before you have recompiled your project).

We could maybe before compting the new fixed output, relying on this assertion, trigger a new compilation?

[bjaglin]

We could maybe before compting the new fixed output, relying on this assertion, trigger a new compilation?

Maybe I am misunderstanding, but this is indeed what I was suggesting by

ScalafixArguments could take an optional callback that would instruct the client to recompile sources if/when a patch is applied.

[adpi2]

I have experienced some StaleSemanticdbError as well in an sbt project. When so, I run clean and then it is working fine.

@bjaglin Have you ever seen this StaleSemanticdbError on the single invocation, on a clean project? If so do you have an example?

[bjaglin]

@adpi2 I think I have. I will try to write a scripted.

[bjaglin]

@adpi2 I think I have. I will try to write a scripted.

Apologies, the StaleSemanticdbError was specific to my very adhoc setup, caused by concurrent runs of scalafix.

[bjaglin]

Looking at the code and seeing that patches are applied after execution of all rules, I am wondering if we should update this issue and keep it open. It seems that rules (no matter if they are semantic or syntactic) all run against the same state (source & semanticdb). @olafurpg was that a design decision or a result of the most simple implementation?

Making syntactic rules actually incremental/successive would only require some implementation changes in scalafix-core, but for semantic rules we would need the hook mentioned in the description.