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

Correctly indent function return types on new lines #4379

Closed

Conversation

ayazhafiz
Copy link
Contributor

There are two commits in this PR. The first fixes the incorrect return type indentation discussed in #4373. The second simplifies the logic of the code ambient to the fix to try lower the number of branches and amount of work needed to reason about what is happening. The biggest change w.r.t. the latter commit is that we can think about indentation of the return type only when the return type should be on its own newline; otherwise, there is no reason to indent it.

There is a blocker that need to be discussed before this could be landed:

  • (cc @topecongiro) that this change is the indented behavior for a return type when the indent style is block

To expand on this, visual indent styles would have the return type inline with the parameter indent:

fn lorem(ipsum: usize,
         dolor: usize,
         sit: usize,
         amet: usize,
         consectetur: usize,
         adipiscing: usize,
         elit: usize)
         -> usize {
}

I think this is a reasonable difference between visual and block indent, but we need to confirm this.


tangent: in the process, I discovered what I think is another bug w.r.t. fn signature indentation with the visual indent style. Under this mode, a code would format as

    fn foo(
        )
        -> BoxFuture<'static,
                     Result<Box<dyn dirents_sink::Sealed>,
                            Status,
                            BoxFuture<'static, Result<Box<dyn dirents_sink::Sealed>, Status>>>>;

but I think the desired formatting is more like

    fn foo() -> BoxFuture<'static,
                          Result<Box<dyn dirents_sink::Sealed>,
                                 Status,
                                 BoxFuture<'static, Result<Box<dyn dirents_sink::Sealed>, Status>>>>;

However, this is a rather esoteric case that I don't think should be prioritized until more users begin submitting reports of it in real cases.


I think the function formatting function as a whole could use some refactoring for better reasoning and less branches; currently it is exceptionally large and difficult to understand.

@calebcartwright
Copy link
Member

calebcartwright commented Aug 11, 2020

I think the function formatting function as a whole could use some refactoring for better reasoning and less branches; currently it is exceptionally large and difficult to understand.

It's a bit of an exercise trying to keep mental map of that one isn't it 😄 I'm inclined to agree, though I do think we'd want to have some bench marks should it become a major refactor (which I suspect it would). Although it's been a while, there have been some perf issues with rustfmt in the past, so it'd be great to have that insight alongside the functional validation from the testing for any potential big refactor.

@calebcartwright
Copy link
Member

To expand on this, visual indent styles would have the return type inline with the parameter indent:

fn lorem(ipsum: usize,
         dolor: usize,
         sit: usize,
         amet: usize,
         consectetur: usize,
         adipiscing: usize,
         elit: usize)
         -> usize {
}

Are you asking if this is the correct/desired formatting with Visual indent style or saying the changes here modify the Visual formatting from X to this?

Not sure if my Visual indent_style mental formatter is accurate, but I believe the above formatting is how rustfmt already would format that fn with Visual style, so my guess is that your question is the former?

@ayazhafiz
Copy link
Contributor Author

Are you asking if this is the correct/desired formatting with Visual indent style or saying the changes here modify the Visual formatting from X to this?

My question is whether the changes to the block indentation are correct, because this change would mean the newline return type indentation is different from how it would be in a visual indent style. Which I think is reasonable and makes sense to me, but would like some external validation.

@calebcartwright
Copy link
Member

Going to close given the age and 2.0 branch target, as we're not actively developing for 2.0 any more

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.

2 participants