cleanup to make use of text blocks leaves out many cases

[carstenartur]

Many cases are not covered by the cleanup to make use of text blocks

                StringBuilder buf= new StringBuilder();
		buf.append("package test1;\n");
		buf.append("public class E1 {\n");
		buf.append("    FI fi= (Integer i, String[][] s) -> {};\n"); // no varargs
		buf.append("}\n");
		buf.append("interface FI {\n");
		buf.append("    void foo(Integer i, String[]... s);\n");
		buf.append("}\n");
		assertNumberOfProposals(proposals, 2);
		assertCorrectLabels(proposals);

		String expected1= buf.toString();

Many more examples at
#1702

[jjohnstn]

Looking at the examples, there are two situations:

1. There is a line comment in the append that specifies a particular append line. This cannot be put into a text block and is meaningless if moved. NLS comments are supported if on all lines.
2. There are calls to indexOf() after the toString() call. The current logic doesn't convert if the buffer is used for other things or other calls to it. It happens that indexOf() is also supported by a String so this could be done.

[carstenartur]

Looking at the examples, there are two situations:

1. There is a line comment in the append that specifies a particular append line. This cannot be put into a text block and is meaningless if moved. NLS comments are supported if on all lines.

So for long sections it would be possible to either have multiple text blocks or use something similar to printf on the resulting block to insert the particular append line. I agree this is no easy addition, maybe not worth it.

2. There are calls to indexOf() after the toString() call. The current logic doesn't convert if the buffer is used for other things or other calls to it. It happens that indexOf() is also supported by a String so this could be done.

Yes, cool approach.

I closed the PR that shows the use cases. I think it is still visible but not in the list of active prs.

[jjohnstn]

Looking at the examples, there are two situations:

1. There is a line comment in the append that specifies a particular append line. This cannot be put into a text block and is meaningless if moved. NLS comments are supported if on all lines.

So for long sections it would be possible to either have multiple text blocks or use something similar to printf on the resulting block to insert the particular append line. I agree this is no easy addition, maybe not worth it.

I don't think breaking up the text block is the answer. I don't know how but it might be worth contacting the language standard people to let then know about such a scenario and see if they could put in a line comment equivalent that would be ignored by the text block.

The other way of doing this would be to subsume comments into the text block since in this case we are dealing with code sequences and adding a line comment within the string would be reasonable. Impossible to guess with a cleanup but perhaps could be done as a quick fix so the user could tell you manually they want that.

For example:

    StringBuilder buf = new StringBuilder();
    buf.append("package p;\n");
    buf.append("public class A {\n");
    buf.append("    public void foo() {\n");
    buf.append("        int a = b;\n");  // here is where the compiler error is
    buf.append("    }\n");
    buf.append("}\n");
    String x = buf.toString();

gets converted to:

    String x = """
        package p;
        public class A {
            public void foo() {
                int a = b;  // here is where the compiler error is
            }
        }
        """;
  

2. There are calls to indexOf() after the toString() call. The current logic doesn't convert if the buffer is used for other things or other calls to it. It happens that indexOf() is also supported by a String so this could be done.

Yes, cool approach.

I closed the PR that shows the use cases. I think it is still visible but not in the list of active prs.

I alread7 have a patch for the indexOf support. I am on vacation but will push it up shortly. If the indexOf() call is in the middle of a bunch of appends it won't convert but it allows the indexOf to be before or after the toString()/implicit toString() call.

[carstenartur]

There are some more cases where the cleanup is failing.
Try it on the file
https://github.com/carstenartur/sandbox/blob/main/sandbox_functional_converter_test/src/org/sandbox/jdt/ui/tests/quickfix/Java8CleanUpTest.java
or just look at it right now as it is already the result of a partly fail.
You see that starting with line 772 it did not work.

[jjohnstn]

Hi @carstenartur It appears that adding the empty string at the end is causing the cleanup to bail (removal of empty strings as last concatenation makes it work). I will make a patch to fix this.

[jjohnstn]

Hi @carstenartur let me know if there are other scenarios. It would be preferable to open separate specific issues for them and close this one as it makes it easier for end-users to track if their problem is the same.

[carstenartur]

Yes, of course. Let's close this one for being able to distinguish different use cases. There are more issues e.g. a case where the the formatting cleanup makes a sequence of text blocks indent each one a little bit more until the text blocks end up with hundreds of leading spaces. When I come to that point I can create another issue.
Thank you very much so far!

[jjohnstn]

Thanks @carstenartur. I am hoping to get your big explicit encoding feature into M2 this week.