Retain trailing comments in module when using rustfmt::skip attribute

[ytmimi]

Refs #5033

Trailing comments at the end of the root Module were removed because the
module span did not extend until the end of the file.

The root Module's span now encompasses the entire file, which ensures
that no comments are lost when using #![rustfmt::skip]

[calebcartwright]

because the module span did not extend until the end of the file.

That's interesting, excellent find! Not too long ago the span on the Crate AST nodes did run through the last token/end of file because the node had a Mod instance which contained the inner span that ran through the end of the last item. However, when the AST structure was changed to no longer include a Mod the Crate span became that inner span 🤔

I think this is probably the best approach to take, since the span change provides some benefits in the compiler for error handling and it wouldn't make sense at this point to revert back. I wonder if it'd be worthwhile to include any of the same skipping logic from #4297 along with the span correction proposed here?

[ytmimi]

I wonder if it'd be worthwhile to include any of the same skipping logic from #4297 along with the span correction proposed here?

Is there a lot of extra overhead to updating the root module's span that you'd like to avoid? Would love to pick your brain on why you think it would be worthwhile just since this was the first time I'm poking around this part of the codebase.

Also, one thing I noticed when trying to fix this issue is that the // leading comment in tests/target/issue-5033/minimum_example.rs that came before #![rustfmt::skip] went through the comment rewrite steps. Is that supposed to happen or should we not even rewrite a file if it contains the #![rustfmt::skip] attribute? If we could bail without rewriting the file then we probably wouldn't need to update the span at all.

[calebcartwright]

Is there a lot of extra overhead to updating the root module's span that you'd like to avoid? Would love to pick your brain on why you think it would be worthwhile just since this was the first time I'm poking around this part of the codebase.

No and to be clear, I do want to go ahead with our full-file version of the span, because that's the "right" span representation for rustfmt. I'm speaking to anything additional that might be worthwhile, e.g. are we still doing any pointless visiting/walking work that something like #4297 could help avoid. I'm a big fan of bailing as early as possible when possible!

Is that supposed to happen or should we not even rewrite a file if it contains the #![rustfmt::skip] attribute? If we could bail without rewriting the file then we probably wouldn't need to update the span at all.

Great question, and no it shouldn't. If a file's been designated to be skipped, either via the inner skip attribute, the @generated marker, in the config ignore list, etc. then it should be left completely untouched

[calebcartwright]

I'm feeling inclined to just merge this since it does provide a useful improvement on the span, and then deal with the more holistic skip in a follow up. What do you think @ytmimi?

[ytmimi]

I'm feeling inclined to just merge this since it does provide a useful improvement on the span, and then deal with the more holistic skip in a follow up.

I'm 100% in alignment with merging what we have now and following up with the optimization afterwards.

Should we open up a new issue to track the follow up optimization?

[calebcartwright]

Should we open up a new issue to track the follow up optimization?

Yeah that sounds good to me. Would you mind getting that created?

[ytmimi]

Yeah that sounds good to me. Would you mind getting that created?
Just opened #5065 to track the backport