- formatting of import long lines

[davidBar-On]

Fixes #4746

Suggested fix to format long import lines. The main issue was that there was no check whether the last import segment in the formatted output will not exceed the maximum width. In case the import line breaks the next line starts with the original next line indentation.

No braces are added when breaking an import line, and it seems that this is not required.

The two test files are the same, except that one is for imports_granularity Crate and the other is for Preserve.

The fix changes issue_3033.rs test, as the target test file included an import line with length of 106 bytes. To keep the original target file it is now used as the test source file (there was no source test file for this issue).

[ytmimi]

should this be

Suggested change

	added_len += 2; // 3 == "::"
	added_len += 2; // 2 == "::"

[davidBar-On]

Changed (the 3 was because originally I also added {)

[ytmimi]

I don't think this is formatting that's permitted based on the formatting rules for large imports. I think we'd need to add braces in order to introduce this line break.

[ytmimi]

As mentioned in #4746 (comment) I think these changes need to be version gated.

I can also see cases where users are fine with their imports exceeding the max_width and wouldn't want rustfmt to break it over multiple lines. Not sure if we should introduce a new config to opt-in to the new line break behavior being proposed here.

[davidBar-On]

I can also see cases where users are fine with their imports exceeding the max_width and wouldn't want rustfmt to break it over multiple lines.

The UseTree does not keep the original line breaks or braces when a single import is break between line. As far as I understand, this information is not available in the AST. In addition, already now, rustfmt does not keep the original line breaks or braces.

For example, the UseTree of the following three imports is the same and currently rusftfmt formats all as use a::b::c::d::e;:

use a::b::c::d::e;
use a::b::c::
    d::e;
use a::b::c::{
    d::e
};

[davidBar-On]

@ytmimi, I improved the formatting, so when an import path is broken between lines, nested imports will always be in a new line, even if they could be in the same line. This may be more in line of the formatting rules.

I also prepared a private version that always adds braces when import path is broken between lines. It produces the following Crate and Preserve target outputs for the source test cases. I hope this can help with the decision whether braces should or should not be added whenever the import line is broken between lines.

[ytmimi]

For example, the UseTree of the following three imports is the same and currently rusftfmt formats all as use a::b::c::d::e;

The first two are the same because the rustc_parser doesn't treat whitespace as significant so both of those get parsed as a::b::c::d::e; despite the whitespace. The 3rd example you point out is a little different. it technically gets parsed as a::b::c::{d::e};, but we can trivially normaize this to a::b::c::d::e;. This might be an oversight when reorder_imports=false, but if you set reorder_imports=false we don't perform that normalization so you're left with:

use a::b::c::d::e;
use a::b::c::d::e;
use a::b::c::{d::e};

I don't expect us to tackle this in this PR. It's unclear whether this is a bug in the reorder_imports=false case or if it's a bug in the reorder_imports=true case since the default value of imports_granularity=Preserve isn't supposed to make changes to your imports.

[ytmimi]

@ytmimi, I improved the formatting, so when an import path is broken between lines, nested imports will always be in a new line, even if they could be in the same line. This may be more in line of the formatting rules.

Just so we're on the same page are you referring to the nested import rules?

I also prepared a private version that always adds braces when import path is broken between lines. It produces the following Crate and Preserve target outputs for the source test cases. I hope this can help with the decision whether braces should or should not be added whenever the import line is broken between lines.

Thanks for continuing to work on this. I think including the braces are more inline with the current rules in the style guide, however, I think it might be best to try and get an explicit rule added to the style guide for long imports that can't be formatted on a single line. There are some good edge cases illustrated by your test cases, for example,

use z123::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::{
    z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::{
        z1234567::z1234567::z1234567::{
            baz, foo
        }
    }
};

// VS

use z123::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::
    z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::
    z1234567::z1234567::z1234567::{baz, foo};

@calebcartwright what are your thoughts on this?

[davidBar-On]

Just so we're on the same page are you referring to the nested import rules?

Yes, practically... Actually I assumed that from the Large list imports rules, but I meant exactly what is written in Nested imports rules.

use z123::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::
z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::
z1234567::z1234567::z1234567::{baz, foo};

With the last change referred above (when not adding braces for each line break) this is now formatted as:

use z123::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::
    z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::z1234567::
    z1234567::z1234567::{
        baz, foo
    };

It's unclear whether this is a bug in the reorder_imports=false case or if it's a bug in the reorder_imports=true case since the default value of imports_granularity=Preserve isn't supposed to make changes to your imports.

I see that setting reorder_imports=false causes several issues in the test cases. I will try to at least understand what is causing that.

[ytmimi]

With the last change referred above (when not adding braces for each line break) this is now formatted as:

Thanks for clarifying!

I see that setting reorder_imports=false causes several issues in the test cases. I will try to at least understand what is causing that.

If I had to guess it's because we're not doing any normalization of the imports when reorder_imports=false is set as demonstrated above with the a::b::c::d::e; example, however without a deeper dive into the code I can't say for certain.

[davidBar-On]

I see that setting reorder_imports=false causes several issues in the test cases. I will try to at least understand what is causing that.

Issue was mainly related to indentation. Now fixed. Added related test cases.

During the issue evaluation, I found that use-trees merge does not take reorder_imports config into account. Submitted a separate PR #5621 to resolve this issue. Therefore, the "use-trees merge" cases were removed from the crate_no_reorder.rs test file, since these cases will produce wrong output until PR #5621 (or other fix) will be merged.

If I had to guess it's because we're not doing any normalization of the imports when reorder_imports=false is set as demonstrated above with the a::b::c::d::e example, however without a deeper dive into the code I can't say for certain.

See the discussion about the "a::b::c::d::e example" issue in the description of PR #5621, including an optional suggested fix.