Try changing how we currently "maintain" types with new where clause position

[calebcartwright]

An idea coming out of #5778, also refs #5580 and #5062/#5044

There are a couple of different techniques utilized within the rustfmt codebase for dealing with something in the AST that rustfmt doesn't yet know how to format (i.e. something the Rust Style Guide doesn't yet have rules for)

Both of the two techniques discussed below ultimately use the same underlying approach of peeking back into source map for that AST component to grab the associated original contents from the input file (to avoid rustfmt "deleting" that content), though there's some differences between the two in how they get there.

While the formatting result ends up being the same for both, in certain cases one technique may be better than the other because sometimes one approach can trigger other rustfmt bugs while the other may not.

We know that rustfmt currently doesn't support type aliases that have a where clause in the "new" position (note that this "new" position has actually been around for a while and is the preferred position, it's just still "new" to rustfmt compared to the prior position) and as such internally rustfmt is applying one of the techniques discussed above. However, that technique is triggering other bugs (as shown in #5778) so I think we should see if using another could avoid triggering that other bug being hit.

If memory serves correctly, this is the part of the codebase that's determining whether there's a where clause in the new position, and if so, bailing using one of the techniques (exiting early with a None):

rustfmt/src/items.rs

Lines 1609 to 1611 in a44c7ea

	if !after_where_predicates.is_empty() {
	return None;
	}

These None's end up bubbling up and hitting some error handling and recovery that ensures the type alias is maintained, but, it's also what's triggering the idempotency bug in this case. As such I'd like to see if using the other technique here works better.

Specifically, let's try directly grabbing the original inputs (e.g. context.snippet(whatever_span_we_need)) and using that as part of a "successful" formatting result by switching that existing early return to return a Some with that snippet instead.

I assume/hope that the span captured as part of that type alias rewrite struct is sufficient and correct, though if not we may need to adjust it ensure the full alias is maintained.

We should be able to add an idempotence test for this to validate (see docs on adding tests for more info) using the two input snippets shared in the description of #5778.

If that test passes and shows this change avoids the idempotency bug 🤞 then we can also run the diff check job out of an abundance of caution to be more confident switching to this technique won't introduce any subtle formatting diffs elsewhere (currently only the rustfmt maintainers can trigger that job, but we're happy to work with anyone that picks this up)

[calebcartwright]

@HarrisonHemstreet based on some of the discussion in Zulip I'm thinking this might be one (of a few issues) we'd suggest starting with.

As noted in the description this is a bit of an experiment that may not pan out, but either way it's probably a worthwhile exercise and should help you start getting familiar with some common concepts within the codebase.

Feel free to ping us here and/or in chat if you have any questions. If this is one you'd like to work on then you can use rustbot to get it assigned to you (just post a comment here that includes the text @rustbot claim)

[HarrisonHemstreet]

@rustbot claim

[HarrisonHemstreet]

Just to give an update:
After working on this issue (specifically focusing efforts in src/items.rs in function rewrite_ty), it seems that this issue may be apart of the larger idempotent bug at play. I'm going to experiment in src/macros.rs lines 1249 - 1290 and see if I can learn more about this idempotent bug

[calebcartwright]

@HarrisonHemstreet - i think this has proven to be an experiment that did not pan out, and that we should accordingly close. Would you concur?

[HarrisonHemstreet]

@calebcartwright I'm happy to close this if you are! 🙂