Fix a splat legacy demangling display bug

[teor2345]

View all comments

Tracking issue: #153629

This PR fixes a splat legacy demangling display bug, where the comma is in the wrong place in the argument list.

It also does a cleanup from @folkertdev's PR #159643 review:

• move rustfmt::skip to main() and deduplicate it: Add support for splatted function pointers #159643 (comment)

@rustbot label +F-splat +C-bug +A-name-mangling

Edit: this PR now accepts 8-16 character crate hashes in normalisation regexes, so it no longer depends on rust-lang/rustc-demangle#94 being reviewed, merged, and updated in rustc's deps

[rustbot]

r? @camelid

rustbot has assigned @camelid.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 74 candidates
• Random selection from 19 candidates

[teor2345]

@rustbot ready

[camelid]

There should be a space after the comma too.

Suggested change

	write!(self, ",")?;
	write!(self, ", ")?;

View changes since the review

[teor2345]

Spacing unfortunately depends on the pretty-printing context, so I think I'll need to modify the trait definition and implementers. But it should be less code overall, which is nice.

rust/compiler/rustc_middle/src/ty/print/pretty.rs

Lines 292 to 300 in b5be620

	/// Prints comma-separated elements.
	fn comma_sep<T>(&mut self, mut elems: impl Iterator<Item = T>) -> Result<(), PrintError>
	where
	T: Print<Self>,
	{
	if let Some(first) = elems.next() {
	first.print(self)?;
	for elem in elems {
	self.write_str(", ")?;

rust/compiler/rustc_symbol_mangling/src/legacy.rs

Lines 490 to 498 in b5be620

	// Identical to `PrettyPrinter::comma_sep` except there is no space after each comma.
	fn comma_sep<T>(&mut self, mut elems: impl Iterator<Item = T>) -> Result<(), PrintError>
	where
	T: Print<Self>,
	{
	if let Some(first) = elems.next() {
	first.print(self)?;
	for elem in elems {
	self.write_str(",")?;

[teor2345]

I ended up adding a comma_sep_has_space method, refactoring the near-duplicate comma_sep methods, and adding comments where we might want to do a similar fix in future.

(We don't currently have non-mangling pretty-printing tests for splat, and that's ok, it's an incomplete feature.)

@rustbot ready

[teor2345]

Failed due to a 15 digit crate hash in v0 demangling, which might be a bug to fix in demangling. This failure isn't splat related, it might happen on 1/16 of PRs (or changes to this test) anyway.

Asked for advice here:
https://rust-lang.zulipchat.com/#narrow/channel/386786-rustc-codegen-gcc/topic/crate.20hash.20drops.20leading.20zeroes/with/613817866

[teor2345]

It looks like we're stalled on the rustc-demangle side here, so I might redesign these tests so they're not blocked on it.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[teor2345]

@rustbot ready

I changed this PR to accept 8-16 character crate hashes in normalisation regexes, so it no longer depends on rust-lang/rustc-demangle#94 being reviewed, merged, and updated in rustc's deps. This is consistent with the normalisation approach taken in the UI test framework.