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

Poor formatting of newline return types in existing tests #4373

Open
ayazhafiz opened this issue Aug 8, 2020 · 10 comments
Open

Poor formatting of newline return types in existing tests #4373

ayazhafiz opened this issue Aug 8, 2020 · 10 comments
Assignees
Labels
bug Panic, non-idempotency, invalid code, etc.
Milestone

Comments

@ayazhafiz
Copy link
Contributor

A number of our existing tests (targeting rustfmt on master) have poor formatting of return types that don't align with the changes made in #4368:

pub fn parse_conditional<'a, I: 'a>()
-> impl Parser<Input = I, Output = Expr, PartialState = ()> 'a

pub extern "C" fn Java_com_exonum_binding_storage_indices_ValueSetIndexProxy_nativeContainsByHash()
-> bool {

fn my_function_name()
-> HashMap<(String, String, (String, String)), (String, String, String, String)> {

fn setup_happy_path()
-> Result<String, CustomTypeA>

I think these should be fixed to use return type indentation, a formatting regression fixed in #4368. These tests appear to have been added after that regression.

@topecongiro @calebcartwright if this sounds reasonable, feel free to assign me to this issue, I have some ideas of how to fix this and simplify the monolith function that handles function signature formatting (at least w.r.t. return types).

@ayazhafiz ayazhafiz added the bug Panic, non-idempotency, invalid code, etc. label Aug 8, 2020
@calebcartwright
Copy link
Member

I missed this entirely in #4368, but this is the current behavior when version = "Two" in released versions rustfmt as well.

I think we need to confirm that the v2 behavior is unintentional/unexpected first, but I personally agree that the suggested indentation would seem to be the expected/preferable option.

@calebcartwright calebcartwright added this to the 2.0.0 milestone Aug 8, 2020
@ayazhafiz
Copy link
Contributor Author

Yes, the cause of the regression (I believe) as mentioned there, is that logic handling correct indentation of the return type was gated behind Version One, which was removed after some time.

All of these tests that I could find appear to have been added after the Version One gate was removed. Is there a better way to confirm that the behavior is not intentional?

@calebcartwright
Copy link
Member

is that logic handling correct indentation of the return type was gated behind Version One, which was removed after some time

Because the gate was added there, we know it was because the author was modifying rustfmt in such a way that was known to break existing formatting and thus required users to explicitly opt into it. The current behavior simply became the default when the gates were removed.

Of course that doesn't necessarily mean this part was intentional, as this could have been an unforeseen side effect of some other objective. Best way to try to get back to root intent would be to find the original issue that resulted in the code changes with the gate. Not sure whether that will be easier in this case with git's history or GitHub's search features.

Just a side note here, this is a great example of challenges posed by the version gates, especially when they can be very long lived.

@ayazhafiz
Copy link
Contributor Author

Yeah so #3731 originally added the gate, and #3891 removed it (in src/items.rs). Neither commit includes tests for the particular case that a return type is placed entirely on its own line (i.e. -> T is separated by a newline from the closing paren of fn params), so my guess is this was just an accidental oversight of a corner case.

@calebcartwright
Copy link
Member

my guess is this was just an accidental oversight of a corner case.

I agree, thanks for digging into it! Will assign this to you if you'd like to carry on addressing these, though will want to get confirmation from @topecongiro before merging that this was indeed unintentional as we suspect.

@topecongiro
Copy link
Contributor

Sorry for the late reply.

Regarding #4366; we should break parameters into multiple lines before anything else if the function signature does not fit in a single line:

// rustfmt-max_width: 80
trait GetMetadata {
    fn metadata(
        loc: ApiEndpointParameterLocation,
    ) -> Vec<ApiEndpointParameter>;
}

If the function has no parameter then we put the return type on its own line:

trait GetMetadata {
    fn metadata()
    -> Vec<ApiEndpointParameter>;
}

It is intentional that rustfmt doesn't indent the line starting with -> ReturnType when the function has no parameter. The formatting guide doesn't yet mention how we do this, though.

@lroux-at-jellysmack
Copy link

Hello, is there any update or help wanted on this issue ?

Thanks :)

@calebcartwright
Copy link
Member

hi @lroux-at-jellysmack could you expand a bit on your question? Is this something you are encountering using a released version of rustfmt with the version option set to Two, or are you building the experimental v2.0 rustfmt from source control, and/or are you just interested in contributing to rustfmt in general 😄, or something else entirely?

@ytmimi
Copy link
Contributor

ytmimi commented Jul 26, 2022

@calebcartwright is my understanding correct that we should be indenting the return types and we're currently not doing that?

@lroux-at-jellysmack
Copy link

@calebcartwright: Hello, sorry it was a long time ago and my comment had poor value, I don't remember what problem I encountered at the time :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Panic, non-idempotency, invalid code, etc.
Projects
None yet
Development

No branches or pull requests

5 participants