Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 372801

Summary:

[formatter] 'Never join wrapped lines' inconsistent behavior, breaking other rules

Product:

[Eclipse Project] JDT

Reporter:

Mateusz Matela <mateusz.matela>

Component:

Core

Assignee:

Mateusz Matela <mateusz.matela>

Status:

CLOSED DUPLICATE

QA Contact:

Ayushman Jain <amj87.iitr>

Severity:

normal

Priority:

CC:

amj87.iitr, stephan.herrmann

Version:

3.8

Target Milestone:

4.5 M6

Hardware:

All

OS:

All

Whiteboard:

Attachments:

Description	Flags
Java file with every token in a separate file for testing	none
Proposition of a solution	none
Changes in tests - fist approach	none

Description Mateusz Matela

2012-02-28 16:16:51 EST

Build Identifier: M20120208-0800

When using 'Never join wrapped lines' option, the formatter often leaves out newline characters that break other rules of the formatter. My point is similar to bug 354626, but I want to take a more general view.

I create a simple class for testing. Formatted with built-in 'Java Conventions' profile, it looks like this:
-----
package pl.test;

import java.io.Serializable;

public class Test implements Serializable {
    public int a(int b, int c) {
        if (b < 2) {
            // comment
        } else {
            try {
                b = 3;
            } catch (Exception e) {
                System.out.println("test");
            }
        }
        return 3;
    }
}
-----
Now, for the test, I insert a newline character everywhere java language lets me. The resulting code takes 71 lines and is ugly, so I won't paste it here (I'll add it as an attachment for convenience). I change the built-in formatter to 'Never join wrapped lines' and reformat the whole file. The result is this:
-----
package pl.test;

import java.io.Serializable;

public class Test
        implements
        Serializable
{
    public int
            a
            (
                    int
                    b
                    ,
                    int
                    c
            )
    {
        if (b
        < 2)
        {
            // comment
        }
        else
        {
            try
            {
                b =
                        3;
            } catch (Exception e)
            {
                System.out
                        .
                        println
                        (
                        "test"
                        );
            }
        }
        return 3;
    }
}

-----
As you can see, some lines have been joined, while others have not. For me, the most striking inconsistency is that System.out is joined into single line, while the following '.' and 'println' stay in separate lines. Also, the 'else' keyword is left in a separate line, while the whole 'catch' expression is joined into one line with the preceding bracket. 

What the desired behavior is is not obvious here, but I think it depends on what we understand by 'already wrapped lines'. One way to go is that each linebreak in a file is line wrapping that should not be joined. This leads to conclusion that formatter should leave every newline character intact and my test file would be left with 71 lines of code. This makes using formatter practically pointless, so it's not a good way to go.

The other way is defining 'already wrapped lines' as only the linebreaks put in places where the automatic formatter is allowed to break lines when maximum line width is exceeded. These places are:
- before '.' character in member access
- after ',' character in arguments list and array initializers
- before arithmetic or logical operator
- after '(' in method arguments list
- ... (some other, but not many. Depends mainly on "Line wrapping" section of formatter profile editor.)
Any other linebreaks put in places where the automatic formatter would not, should not be considered lines wrapped by user, but invalid formatting that must be fixed.

This strategy would result in my test file formated to the following form:
----
package pl.test;

import java.io.Serializable;

public class Test implements Serializable
{
    public int a(
            int b,
            int c) {
        if (b
        < 2)
        {
            // comment
        } else {
            try {
                b = 3;
            } catch (Exception e) {
                System
                        .out
                        .println(
                            "test");
            }
        }
        return 3;
    }
}
---- (Actually, I'm not sure about System.out, maybe static field access should not be wrapped - there's no such option in formatter settings.)
I think this is the only way to make the formatter behave consistently and not ignore other rules defined by the user. If I understand it correctly, this was also the intention when 'never join lines' option was initially proposed in bug 198074. Also, other java IDEs (NetBeans, IntelliJ) handle line wrapping this way.

I think 'Never join already wrapped lines' is a very important feature because humans can often wrap lines in a more readable way than autoformat. But current implementation makes it impractical to use. Developers have to manually review code after performing autoformat to make sure they didn't accidentally put any newline characters that break formatting used in the team.

Reproducible: Always

Comment 1 Mateusz Matela

2012-02-28 16:25:54 EST

Created attachment 211765 [details]
Java file with every token in a separate file for testing

Comment 2 Ayushman Jain

2012-02-29 00:32:05 EST

Thanks for the description and suggestions. Will take a look at it as soon as I can, though not sure for 3.8

Comment 3 Mateusz Matela

2012-03-11 19:15:35 EDT

Created attachment 212440 [details]
Proposition of a solution

I've looked into the formatter code and after some trial and error I came up with a very small change that apparently does exactly what I proposed.

After applying the patch, number of failed tests in org.eclipse.jdt.core.tests.formatter package goes up from 33 to 76. I skimmed through the new fails and in most cases the formatter behavior is correct (according to my suggestions), so the tests would have to be updated. There are also some cases where fail is not directly related to this change but reveals some other bugs in formatter (that exist even with "never join lines" option turned off).

Could you take a look at this patch? If it's going in the right direction, I could work on it some more and clean up the tests.

Comment 4 Ayushman Jain

2012-03-12 04:52:59 EDT

(In reply to comment #3)
> [..]
> There are
> also some cases where fail is not directly related to this change but reveals
> some other bugs in formatter (that exist even with "never join lines" option
> turned off).
Thanks for the patch. Did you notice any test failures without your patch? There should be no failures ideally, so if you do see them it may mean that you're using the wrong sources or something is wrong with the setup. Please go through http://wiki.eclipse.org/JDT_Core_Committer_FAQ once to make sure you follow the right steps. Thanks!

Comment 5 Mateusz Matela

2012-03-17 15:30:49 EDT

Do you assume "there should be no failures ideally" or do you actually know that all the tests pass?
I checked everything again, I'm doing things exactly according to the FAQ. In case I miss something, here are the steps I take on a freshly downloaded Eclipse (Build id: I20120314-1800), in completely new workspace:
1. Settings -> Installed JREs -> add j2sdk 1.4.2_19 and jdk 1.6.0_26 next to default jre7
2. Settings -> API Baselines -> add location of Eclipse 3.7.2
3. Installed EGit, cloned the repos (master branches), imported the projects
4. There was a build error: Execution environment references were not checked for 'org.eclipse.jdt.core' because no environment descriptions are installed.
  After some googling I installed API Tools Execution Environment Description Plugin, as advised in https://bugs.eclipse.org/bugs/show_bug.cgi?id=361660#c1
5. Select project org.eclipse.jdt.core.tests.model -> package org.eclipse.jdt.core.test.formatter -> Run as -> Run Configurations -> create item in JUnit Plug-in Test category -> switch Test runner to JUnit4, switch Runtime JRE to jre7 -> Run

Result: out of 2181 tests there are 419 errors and 34 failures.

Comment 6 Ayushman Jain

2012-03-19 03:53:40 EDT

(In reply to comment #5)
> Do you assume "there should be no failures ideally" or do you actually know
> that all the tests pass?
We run the tests with every build so I know that all tests are currently green. :)
Since you get failing tests, it means your changes caused those failures. Note that its not necessary that they are 'failures'. Your patch causes a behaviour change and the failing tests may just be reflecting that new behaviour. So its a good idea to go through the failures one by one and check which of those are 'expected' due to the patch and which of them are regressions. Thanks!

Comment 7 Mateusz Matela

2012-04-04 17:45:28 EDT

Created attachment 213608 [details]
Changes in tests - fist approach

My problem with tests was that they fail if I run all tests from a selected package. RunFormatterTests class should be used instead.

Now my patch introduces 45 failures and only 25 of them could be easily changed to accommodate the new behavior (in attached patch). The rest are either potential regressions or associated with another issues. I divided them into some categories:

1. Wrapping of binary expressions doesn't work if there are less than 5 elements.
example:
	long x1 = 100000000\n
			+ 200000000\n
			+ 300000000;\n
notes:
	it works when at least 2 variables are used instead of literals, for example
	long x1 = longVariable1\n
			+ 200000000\n
			+ longVariable2;\n
affected tests:
	testBug286601b
	
2. In a long list of expressions connected with logical or bit operators, the last two are never wrapped
example:
	Character\n
			.isJavaIdentifierPart(token)\n
			|| '.' == token\n
			|| '-' == token\n
			|| ';' == token\n
affected tests:
	testBug330313_wksp1_01_njl
	testBug330313_wksp1_11_njl
	testBug330313_wksp1_15_njl (probably)
	testBug330313_wksp1_18_njl
	testBug330313_wksp1_19_njl
	testBug330313_wksp1_22_njl
	testBug330313_wksp1_26_njl
	testBug330313_wksp1_28_njl (probably)
	
3. Cannot wrap on relational an equality operators.
example:
	if (fieldBinding.declaringClass.getPackage()\n
			!= currentScope.enclosingSourceType().getPackage()) {
affected tests:
	testBug330313_wksp1_14_njl
	testBug330313_wksp1_16_njl
	testBug330313_wksp1_17_njl
	testBug330313_wksp1_23_njl
	testBug330313_wksp1_27_njl
	
4. Potential regressions:
	testBug330313_wksp1_25_njl (unnecessary newline)
	testBug330313_wksp1_33_njl (instability)
	testBug330313_wksp1_36_njl (unnecessary newline)
	testBug330313_wksp1_41_njl (indentation problem)
	testBug330313_wksp1_52_njl (instability)
	testBug330313_wksp3_X01_njl (indentation problem)

I'll try to look more closely at the last category and see if I can resolve these issues, although the code is quite complicated for me.
As for the rest of the issues, the underlying causes exist even with 'never join wrapped lines' option off so I guess you'll have to decide what to do.

Comment 8 Ayushman Jain

2012-04-05 03:14:05 EDT

(In reply to comment #7)
> Created attachment 213608 [details]
> Changes in tests - fist approach
Thanks for the patch and the detailed explanations Mateusz

> I'll try to look more closely at the last category and see if I can resolve
> these issues, although the code is quite complicated for me.
> As for the rest of the issues, the underlying causes exist even with 'never
> join wrapped lines' option off so I guess you'll have to decide what to do.
So for the rest of the issues, do you mean you see them without your fix too? Or do you see them with your fix but with and without "never join wrapped lines"? We usually prefer to not break existing behaviour, unless its fixing something. So it would be better if all the test failures are sorted out. Thanks!

Comment 9 Mateusz Matela

2012-04-05 14:20:34 EDT

(In reply to comment #8)
> So for the rest of the issues, do you mean you see them without your fix too?
Yes, it just so happens that current tests don't reveal them without my patch, because code samples that should reveal them use the 'never join lines' option. Category 1. and 2. are definitely bugs, although probably not very annoying. 3. is more disputable, but IMO the formatter should sometimes put linebreak before operators like !=, <, etc.

BTW, I later noticed that problem 2. only exists when the expression is inside of more than one level of parenthesis, for example
if ((a || b || c || d)) {
                 ^ will not break here even when line is too long
if (a || b || c || d) {
                 ^ will break, no problem

> Or do you see them with your fix but with and without "never join wrapped
> lines"? We usually prefer to not break existing behaviour, unless its fixing
> something. So it would be better if all the test failures are sorted out.
None of the tests affected by my patch have the 'never join lines' option off, so I think we're safe here :)

Comment 10 Ayushman Jain

2012-04-06 03:59:36 EDT

I'm not sure to what extent but patch in bug 303519 might fix some ambiguities in line wrapping. It may help to release this patch only when that bug is fixed, though its good to have all other issues ironed out in this patch and preserve it till then. :)

Comment 11 Mateusz Matela

2015-06-11 08:16:42 EDT

Fixed with bug 303519

Comment 12 Stephan Herrmann

2016-06-11 14:37:22 EDT

Bookkeeping, inboxes don't fix bugs :)

*** This bug has been marked as a duplicate of bug 303519 ***