Add textEdits method to ScalafixPatch #1737

LaurenceWarne · 2023-02-23T12:13:39Z

Hi! I'd like to be able to obtain (LSP style) text edit like information (essentially representing a diff as a start position, end position, and insert text) from the scalafix api (specifically a given ScalafixFileEvaluation) - AFAICS something like this is currently not exposed (?).

I've created an implementation (the method is similar to PatchInternals.tokenPatchApply). Though I've found some rules don't give the greatest results, for example patches from the OptionWhenUnless rule (from https://github.com/xuwei-k/scalafix-rules):

Concat(Concat(Concat(Concat(Concat(Concat(Concat(Concat(Concat(Concat(Concat(Concat(Concat(Concat(Concat(Concat(Concat(Concat(Remove(if∙[142..144)),Remove(∙∙[144..145))),Remove((∙[145..146))),Remove(4∙[146..147))),Remove(∙∙[147..148))),Remove(>∙[148..149))),Remove(∙∙[149..150))),Remove(3∙[150..151))),Remove()∙[151..152))),Remove(∙∙[152..153))),Remove(Some∙[153..157))),Remove((∙[157..158))),Remove(43∙[158..160))),Remove()∙[160..161))),Remove(∙∙[161..162))),Remove(else∙[162..166))),Remove(∙∙[166..167))),Remove(None∙[167..171))),Add(if, if [142..144), ifOption.when(4 > 3)(43)))

Result in multiple overlapping text edits. Though I've had good results for many of the built in rules, for example a typical LeakingImplicitVal patch:

Add(val, val [202..205), private val)

results in a singular text edit.

Anyway, let me know if you are open to something like this, or if something like this is already implemented, thanks 🙂

bjaglin · 2023-04-16T18:23:37Z

Hi @LaurenceWarne! Thanks for putting this together, apologies for the (long) delay.

First of all, I am not very knowledgeable about patch internals as I never got the chance to change anything in there, so take my words with a pinch of salt.

I understand your idea is to let a user interactively review & apply patches, and that sounds like a cool feature! I see 4 options for the implementation:

Iterate over on what you started, adding logic to merge contiguous TokenPatchs when possible (most rules use Patch.replaceTree(), which creates one patch per token - this is what you observed with OptionWhenUnless)
- The signature makes it impossible to identify/group several TextEdits belonging to the same patch (FTR, ScalafixFileEvaluationImpl.patches are all atomic since Respect the intent of rules authors when returning atomic patches #1261) - say many functions/variables are renamed by a rule, how could the user apply a consistent set of TextEdits if you don't want all changes?
Keep the same signature, but rely on the overall file diff (like previewPatchesAsUnifiedDiff() does), and extract a set of TextEdits based on that only (I assume 3rd party libraries exist for that?)
- Same limitation as the last one
- Merging contiguous changes would come for free (in terms of development cost and also run time as the target is already generated so we wouldn't need to traverse the tree again - this is the most performant option)
- Risk of stitching unrelated changes, making it potentially impossible for the user to programmatically accept changes at the beginning of a line but rejecting the changes at the end of it (in the case where they came from several atomic patches)
Exposing TextEdits on each scalafix.interfaces.ScalafixPatch
- Removes the limitation from (1) and (2)
- This would provide the biggest flexibility for the client to flatten them (effectively bringing back the first behavior) or let the user review patches independently
Exposing a unified diff on each scalafix.interfaces.ScalafixPatch
- A different developer experience than the previous one, that might be more generic, but requires some more logic on the client side
- No risk of stitching unrelated changes like in (2) since each patch is supposed to be atomic

My current thoughts:

I would rather explore (3) or (4) as the extra effort is likely limited
I have a small preference for (4) as unidiff seems to be a much more generic/adopted format than a set of TextEdits

Does this help? Let me know what you think. In any case, I'll be happy to help you with reviews (in a much more timely manner than this first one 😅), but not much more considering the limited bandwidth I have.

LaurenceWarne · 2023-04-22T11:48:19Z

Hi @LaurenceWarne! Thanks for putting this together, apologies for the (long) delay.

Hi @bjaglin, no problem at all, thanks for the detailed response!

Iterate over on what you started, adding logic to merge contiguous TokenPatchs when possible (most rules use Patch.replaceTree(), which creates one patch per token - this is what you observed with OptionWhenUnless)
The signature makes it impossible to identify/group several TextEdits belonging to the same patch (FTR, ScalafixFileEvaluationImpl.patches are all atomic since #1261) - say many functions/variables are renamed by a rule, how could the user apply a consistent set of TextEdits if you don't want all changes?

I think taking advantage of atomic patches might be enough for my use case actually. (you might have already guessed 🙂 ) I'm playing around with extending metals' scalafix integration to make it more fine-grained - and I realised I can use a workspace edit so multiple text edits can be applied at once (for a given code action) and they needn't overlap in any sense - we would only need to map a given atomic patch to a set of text edits if that makes sense.

So we could return a nested collection of text edits, with each individual text edit set corresponding to one atomic patch without any need to resort to heuristics around combining overlapping token patches, etc. How does that sound?

One problem with this method I found is that it relies on rule authors using .atomic, for example, OptionWhenUnless doesn't use this (is there a reason that you might not want to use .atomic for a rule?).

I think the implementation is a lot easier as well, will push a commit 🙂

bjaglin · 2023-04-27T14:10:40Z

So we could return a nested collection of text edits, with each individual text edit set corresponding to one atomic patch without any need to resort to heuristics around combining overlapping token patches, etc. How does that sound?

Good - that's effectively what I was suggesting in (3).

Could you just change the signature to expose an Array[TextEdit] on scalafix.interfaces.ScalafixPatch instead of having a nested array at the root?
Unfortunately, we don't have a great unit test coverage of the patch APIs, but could you add an integration test in ScalafixArgumentsSuite?

One problem with this method I found is that it relies on rule authors using .atomic, for example, OptionWhenUnless doesn't use this (is there a reason that you might not want to use .atomic for a rule?).

As documented, rules must use .atomic for suppressions to work.

One could argue that Patch.replaceTree() should be atomic by default, but I assume there were good reasons not to do it from the beginning. Challenging that would require an investigation of the potential impact on existing rules defining atomic boundaries at a higher level.

So I recommend that you open a PR to add .atomic around each Patch.replaceTree() for that particular rule, adding suppressions in the test suite to showcase the benefit... And repeat for any other rule that wouldn't play well with the Metals feature you are envisioning.

bjaglin · 2023-04-27T14:24:19Z

So I recommend that you open a PR to add .atomic around each Patch.replaceTree() for that particular rule, adding suppressions in the test suite to showcase the benefit...

Interestingly, it seems that .atomic was removed on rules of the same repo. If I had to guess why, it would be because the atomic was not granular enough, leading to false positives when suppressing patches.

LaurenceWarne · 2023-04-30T09:11:22Z

Could you just change the signature to expose an Array[TextEdit] on scalafix.interfaces.ScalafixPatch instead of having a nested array at the root?
Unfortunately, we don't have a great unit test coverage of the patch APIs, but could you add an integration test in ScalafixArgumentsSuite?

Done! The test currently checks against how many edits are returned per patch, I can change this to the actual content of the edits f you think this would be better.

So I recommend that you open a PR to add .atomic around each Patch.replaceTree() for that particular rule, adding suppressions in the test suite to showcase the benefit... And repeat for any other rule that wouldn't play well with the Metals feature you are envisioning.

Interestingly, it seems that .atomic was removed on rules of the same repo. If I had to guess why, it would be because the atomic was not granular enough, leading to false positives when suppressing patches.

Yeah I saw that too, I can ask the question at least 🙂 - Xuwei's rules are one of the most popular repos I think, but I'll try and test with others. Do you maintain a curated list of community rules by any chance (I couldn't see mention of it on the microsite)?

Another nice to have would be if ScalafixPatches, had some kind of reference to their rule (I think RuleDiagnostics have this, but not patches?), but that's another issue 🙂 .

bjaglin

The test currently checks against how many edits are returned per patch, I can change this to the actual content of the edits f you think this would be better.

I think it would be good do check content, in order to guard the 0-based behavior of ranges.

Do you maintain a curated list of community rules by any chance (I couldn't see mention of it on the microsite)?

There is a list, but it hasn't been updated for a while.

Another nice to have would be if ScalafixPatches, had some kind of reference to their rule (I think RuleDiagnostics have this, but not patches?), but that's another issue slightly_smiling_face .

Agreed, let's keep that for another PR? For the record, I will cut a new minor release of Scalafix as soon as the new 2.12 and 2.13 patch releases are out (within a couple of weeks if I understand well, given that they depend on the release date of 3.3.0, for which RC5 seems final), so there is an opportunity to get this PR out very soon!

bjaglin · 2023-04-30T20:31:11Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixPatch.java

For consistency with other interfaces

Suggested change

default ScalafixEdit[] getTextEdits() {

throw new UnsupportedOperationException("getTextEdits() is not implemented");

default ScalafixEdit[] textEdits() {

throw new UnsupportedOperationException("textEdits() is not implemented");

bjaglin · 2023-04-30T22:02:21Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixEdit.java

@@ -0,0 +1,13 @@
+package scalafix.interfaces;
+
+public interface ScalafixEdit {


Even if a bit pedantic,

Suggested change

public interface ScalafixEdit {

public interface ScalafixTextEdit {

would be more consistent

bjaglin · 2023-04-30T21:46:27Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixEdit.java

+
+    int endColumn();
+
+    String text();


related to comment above, maybe

Suggested change

String text();

String newText();

scalafix-tests/unit/src/test/scala/scalafix/tests/interfaces/ScalafixArgumentsSuite.scala

bjaglin · 2023-04-30T22:05:37Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixEdit.java

+    int startLine();
+
+    int startColumn();
+
+    int endLine();
+
+    int endColumn();


I initially thought that this was https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textEdit, however not only naming, but also the way the range is expressed differs.

Could we reuse ScalafixPosition which is quite well documented?

sure, sounds good to me.

bjaglin · 2023-04-30T22:09:30Z

scalafix-tests/unit/src/test/scala/scalafix/tests/interfaces/ScalafixArgumentsSuite.scala

+    // CommentFileNonAtomic produces two patches which in turn produce one
+    // token patch each, whilst CommentFileAtomic produces one patch which
+    // in turn produces two token patches


Add textEdits method to ScalafixPatch as an alternative way of obtaining the results of a Scalafix evaluation. The results of applying a patch are exposed as a set of LSP style 'text edits', which consists of a start position, end position, and insert text.

LaurenceWarne · 2023-05-01T19:57:02Z

There is a list, but it hasn't been updated for a while.

Cool, I though I remembered one.

Agreed, let's keep that for another PR? For the record, I will cut a new minor release of Scalafix as soon as the new 2.12 and 2.13 patch releases are out (within a couple of weeks if I understand well, given that they depend on the release date of 3.3.0, for which RC5 seems final), so there is an opportunity to get this PR out very soon!

Yes, sorry that's what I meant, and nice!

bjaglin

Thanks @LaurenceWarne !

bjaglin · 2023-06-05T21:15:14Z

Sorry that it took longer than expected to cut the release with that!

Somewhat related effort: https://contributors.scala-lang.org/t/roadmap-for-actionable-diagnostics/6172

LaurenceWarne · 2023-06-07T08:29:53Z

Sorry that it took longer than expected to cut the release with that!

No problem 🙂

Somewhat related effort: https://contributors.scala-lang.org/t/roadmap-for-actionable-diagnostics/6172

Interesting, I hadn't seen this. Thanks for linking, I'll be sure to take a look.

LaurenceWarne force-pushed the add-edit-to-api branch from 41260fa to cbeb5cc Compare April 22, 2023 11:49

LaurenceWarne force-pushed the add-edit-to-api branch from cbeb5cc to a5564c9 Compare April 30, 2023 08:58

bjaglin changed the title ~~Add getTextEdits method to ScalafixFileEvaluation~~ Add getTextEdits method to ScalafixPatch Apr 30, 2023

bjaglin reviewed Apr 30, 2023

View reviewed changes

LaurenceWarne force-pushed the add-edit-to-api branch from a5564c9 to 285a014 Compare May 1, 2023 19:54

bjaglin changed the title ~~Add getTextEdits method to ScalafixPatch~~ Add textEdits method to ScalafixPatch May 4, 2023

bjaglin approved these changes May 4, 2023

View reviewed changes

bjaglin merged commit b691272 into scalacenter:main May 4, 2023

LaurenceWarne deleted the add-edit-to-api branch June 7, 2023 08:29

bjaglin mentioned this pull request Jun 30, 2023

Provide an option to look at linting errors without doing any rewrites #956

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add textEdits method to ScalafixPatch #1737

Add textEdits method to ScalafixPatch #1737

LaurenceWarne commented Feb 23, 2023

bjaglin commented Apr 16, 2023 •

edited

Loading

LaurenceWarne commented Apr 22, 2023

bjaglin commented Apr 27, 2023

bjaglin commented Apr 27, 2023

LaurenceWarne commented Apr 30, 2023

bjaglin left a comment •

edited

Loading

bjaglin Apr 30, 2023

bjaglin Apr 30, 2023

bjaglin Apr 30, 2023

bjaglin Apr 30, 2023

LaurenceWarne May 1, 2023

bjaglin Apr 30, 2023

LaurenceWarne commented May 1, 2023

bjaglin left a comment

bjaglin commented Jun 5, 2023

LaurenceWarne commented Jun 7, 2023

-    default ScalafixEdit[] getTextEdits() {
-        throw new UnsupportedOperationException("getTextEdits() is not implemented");
+    default ScalafixEdit[] textEdits() {
+        throw new UnsupportedOperationException("textEdits() is not implemented");

		@@ -0,0 +1,13 @@
		package scalafix.interfaces;

		public interface ScalafixEdit {

	public interface ScalafixEdit {
	public interface ScalafixTextEdit {

Add textEdits method to ScalafixPatch #1737

Add textEdits method to ScalafixPatch #1737

Conversation

LaurenceWarne commented Feb 23, 2023

bjaglin commented Apr 16, 2023 • edited Loading

LaurenceWarne commented Apr 22, 2023

bjaglin commented Apr 27, 2023

bjaglin commented Apr 27, 2023

LaurenceWarne commented Apr 30, 2023

bjaglin left a comment • edited Loading

Choose a reason for hiding this comment

bjaglin Apr 30, 2023

Choose a reason for hiding this comment

bjaglin Apr 30, 2023

Choose a reason for hiding this comment

bjaglin Apr 30, 2023

Choose a reason for hiding this comment

bjaglin Apr 30, 2023

Choose a reason for hiding this comment

LaurenceWarne May 1, 2023

Choose a reason for hiding this comment

bjaglin Apr 30, 2023

Choose a reason for hiding this comment

LaurenceWarne commented May 1, 2023

bjaglin left a comment

Choose a reason for hiding this comment

bjaglin commented Jun 5, 2023

LaurenceWarne commented Jun 7, 2023

bjaglin commented Apr 16, 2023 •

edited

Loading

bjaglin left a comment •

edited

Loading