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

import_granularity: Don't normalize imports with comments #5311

Merged

Conversation

davidlattimore
Copy link
Contributor

Issue #4991

@ytmimi
Copy link
Contributor

ytmimi commented Apr 18, 2022

Thank for the PR!

Overall the code changes look good to me.

Would you mind adding some additional test cases with nested block comments. For example:

use a::{
    b,
    c,
    // Comment before a::d
    d,
    e, // Comment after a::e
};
use a::{/* before a::f::g? */ f::g, h::{i, /* after a::h::i? */ j}};
use a::{l::{self, m, n::o, p::*} /* after a::l::p::*? */};
use a::q::{self};
use a::q::{/* before */ self};
use a::q::{self /* after */};
// Comment before a::r
use a::r;

@davidlattimore
Copy link
Contributor Author

I tried adding some more tests as described and ran into a few issues. In particular, some of the other import granularities, flatten then regroup imports. This ends up being lossy, especially if there are comments before / after a group. I'm starting to wonder if having any comments within a use statement should result in that use statement being left alone.

There's already code in normalize_use_trees_with_granularity that checks if a use-statement has a comment, and if it does, leaves it as-is, but it only checks for top-level comments, it doesn't check for comments within the use-statement. Perhaps I should just expand that. Thoughts?

@ytmimi
Copy link
Contributor

ytmimi commented Apr 29, 2022

@davidlattimore Thanks for continuing to look into this. I think we should prefer not to flatten use statements if it would result in rustfmt removing comments. I think airing on the side of caution is probably better here. It would be great if you could introduce that logic and add the relevant test cases 😁.

I wonder if following an approach outlined in #3999 would be useful. This was a PR that was merged into an unreleased branch, and also deals with dropping comments from use statements.

@davidlattimore davidlattimore changed the title Preserve comments when flattening use statements import_granularity: Don't normalize imports with comments May 2, 2022
@davidlattimore
Copy link
Contributor Author

I've updated so that use statements that contain comments won't be normalized.

@calebcartwright
Copy link
Member

Just to summarize and ensure I understand correctly, is the consensus that trying to flatten/merge in the presence of comments has far too many complexities and scenarios for rustfmt to be able to format them well, and we're instead just going to bail in the presence of comments?

@davidlattimore
Copy link
Contributor Author

Yep, that was my thinking. The one possible exception might be comments that were after an individual import and on the same line. e.g.

use a::b; // A comment
use a::c;

could be transformed into:

use a::{
   b, // A comment
   c,
};

And vice-versa.

I'm not sure though if it's worth that special case though. If an import is special enough that it deserves a comment, then perhaps it should be left separate from other imports.

All the other cases, e.g. comments before / after a group get really messy.

@calebcartwright
Copy link
Member

I'm not sure though if it's worth that special case though. If an import is special enough that it deserves a comment, then perhaps it should be left separate from other imports.

All the other cases, e.g. comments before / after a group get really messy.

Makes sense, and agreed. Maintaining comments on flat reorders is challenging enough technically but straightforward enough logically. Whereas when changing the granularity rustfmt would be making under informed guesses in even many of the better cases, so this is best left to the developer writing the code since they can temporarily remove/reshuffle such comments and put them back wherever it makes sense in the new granularity post formatting

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants