Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bad formatting sometimes in `use` imports #1400

Closed
chriscoomber opened this issue Mar 23, 2017 · 7 comments · Fixed by #1407

Comments

@chriscoomber
Copy link

commented Mar 23, 2017

Symptoms (example)

use agent_framework::{AgentFrameworkBuilder, AgentError, AgentFrameworkRegisterInterface,
                      HttpConfig};

gets turned into

use agent_framework::{AgentFrameworkBuilder, AgentError, AgentFrameworkRegisterInterface, HttpConfig};

which exceeds the line length (102 vs 100). This is one of those cases where something that's fine gets changed by rustfmt into something that rustfmt doesn't like.

Cause (guess)

I think this is because it's not taking into account the }; in the calculation of line length. Indeed:

use agent_framework::{AgentFrameworkBuilder, AgentError, AgentFrameworkRegisterInterface, HttpConfig, a};

is correctly formatted to

use agent_framework::{AgentFrameworkBuilder, AgentError, AgentFrameworkRegisterInterface,
                      HttpConfig, a};
@chriscoomber chriscoomber changed the title `use` imports miscalculate line lengths Bad formatting sometimes in `use` imports Mar 23, 2017
@nrc

This comment has been minimized.

Copy link
Member

commented Mar 23, 2017

This would be a good first bug (not super-easy, but typical and would give you insight into how Rustfmt works). If anyone wants to try it, I'd be happy to mentor.

@alobb

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2017

I've briefly looked through the code that does the formatting for this import statement, and initially thought the problem was at imports.rs:327; the comment on the line above implies that the width takes into account the curly braces, but not the semicolon.

Changing the 2 to be 3 does fix this issue, but I don't think the underlying behavior is fixed; if I remove 1 character from the given example (i.e., remove the 'H' in 'Http', making the length of it 101 characters

use agent_framework::{AgentFrameworkBuilder, AgentError, AgentFrameworkRegisterInterface,
                                       ttpConfig};

rustfmt will still format it as

use agent_framework::{AgentFrameworkBuilder, AgentError, AgentFrameworkRegisterInterface, ttpConfig};

Am i correct in thinking that my example should also be on two lines (with the default config, nothing changed)?

Additionally, the comment on import.rs:365 seems to imply that the semicolon is taken into account, but it is added to the indent of the Shape, not the width. Does the above sound like I'm on the right track?

@nrc

This comment has been minimized.

Copy link
Member

commented Mar 24, 2017

@alobb

Does the above sound like I'm on the right track?

Yes.

In Rustfmt we track the space that code must be formatted into. That is usually expressed as a Shape object, which is basically just a width and an indent. You should examine the various shapes in that function to see what their values are, you should be able to predict what the shape ought to be and see if there is a discrepancy.

Looking at line 366/7 it does look a bit wrong to me - if the literal 1 is for ; I would expect to subtract that from the width (probably on line 327 as you mention) rather than add it to the indent. However, the 1 might be for the opening brace { (in which case, the comment is wrong).

Looking at the code in this issue it looks like ttpConfig is over-indented. I would expect it to be aligned with AgentFrameworkBuilder, so it looks like line 367 is adding something that is already accounted for in the shape.

Am i correct in thinking that my example should also be on two lines (with the default config, nothing changed)?

Yes

@alobb

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2017

Oops, aligned the second example without previewing, should be:

use agent_framework::{AgentFrameworkBuilder, AgentError, AgentFrameworkRegisterInterface,
                      ttpConfig};

I'll take a look at the Shape objects next!

@alobb

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2017

So after some digging into the Shape objects and how they are constructed, I think I found the problem; the semicolon was being accounted for at imports.rs:254, so the comment at import.rs:365 was misleading. Additionally, the remaining width did not take into account the colon offset. I've fixed these issues, added a new test that confirms the fix works, and changed the comments to explain what is happening.

Some things that came up while fixing this:

  1. In my test file, I added a test case that attempts to format
use aaaaaaaaaaaaaaa::bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb;

This string is 101 characters long, and gets reformatted as

use aaaaaaaaaaaaaaa::
bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb;

Is it worth opening a new issue for that case? That looks incorrect to me.

  1. Running a single test file was difficult; I didn't see a way to do so other than modifying idempotence_tests in system.rs. Long term, would it make sense to switch to something that generates test at build-type (either using macros or build.rs)? I could see this being useful to generate a single test case for each source->target test and idempotent test. I think this would simplify tracking and fixing errors. I apologize if this has already been asked and answered.
@nrc

This comment has been minimized.

Copy link
Member

commented Mar 24, 2017

Is it worth opening a new issue for that case? That looks incorrect to me.

I guess a block indent would be good there. Honestly I expect this to never happen in real code, and so it is not worth worrying about. So, up to you, if you care about it, feel free to open an issue, if not, I'd just forget about it.

Running a single test file was difficult ...

True. My workflow is usually to copy the text out into it's own file and use that as a test case - I find it easier to debug outside of any test infra and what I usually want is a fraction of a test rather than a single test. So, I don't really feel this pain, although I agree it is sub-optimal.

I think it would be better to be able to specify the name of a test somehow in order to run a single test, rather than moving to generate tests. I fear anything that makes adding/reading tests harder (although I admit I don't quite see how your proposal would function in practice, if we can do it without making it harder to add/read tests, then it would be pure win).

alobb added a commit to alobb/rustfmt that referenced this issue Mar 25, 2017
This fixes how line lengths for use statements with multiple
items don't extend beyond the maximum line length.

Fixes rust-lang#1400
@alobb

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2017

Re: Generating tests:

Note: Looking into Cargo, I think this would require rust-lang/cargo#1581, but if the idea sounds good, I'd be happy to look into that.

My idea was that, either in the root build.rs (or, if the above issue is fixed, in a test-specific one), we could generate tests similar to all of the functions marked with #[test] in system.rs. I think this could be preferable to the current way because it gives better integration with Cargo for testing (i.e., when working on this issue, trying to re-run a single idempotence test was impossible, because all files are run under one test). This shouldn't make it more difficult to add tests.

@nrc nrc closed this in #1407 Mar 26, 2017
nrc added a commit that referenced this issue Mar 26, 2017
This fixes how line lengths for use statements with multiple
items don't extend beyond the maximum line length.

Fixes #1400
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.