Poor formatting of newline return types in existing tests

[ayazhafiz]

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:

rustfmt/tests/target/issue-3278.rs

Lines 1 to 2 in f817383

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

rustfmt/tests/target/long-fn-1.rs

Lines 18 to 19 in f817383

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

rustfmt/tests/target/issue-2672.rs

Lines 30 to 31 in f817383

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

rustfmt/tests/target/issue-2672.rs

Lines 48 to 49 in f817383

	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).

[calebcartwright]

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.

[ayazhafiz]

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]

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]

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]

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]

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]

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

Thanks :)

[calebcartwright]

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]

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

[lroux-at-jellysmack]

@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 :/