Fixed ByteStr not padding within its Display trait when no specific alignment is mentioned

[asder8215]

Fixes #152804. Path's Display uses ByteStr's Display, which is where the problem was occurring.

The issue was coming from ByteStr implementation of fmt() in this particular area:

        let Some(align) = f.align() else {
            return fmt_nopad(self, f);
        };
        let nchars: usize = self
            .utf8_chunks()
            .map(|chunk| {
                chunk.valid().chars().count() + if chunk.invalid().is_empty() { 0 } else { 1 }
            })
            .sum();
        let padding = f.width().unwrap_or(0).saturating_sub(nchars);
        let fill = f.fill();
        let (lpad, rpad) = match align {
            fmt::Alignment::Left => (0, padding),
            fmt::Alignment::Right => (padding, 0),
            fmt::Alignment::Center => {
                let half = padding / 2;
                (half, half + padding % 2)
            }
        };

The docs for the align implies that Alignment::Left, Alignment::Right, Alignment::Center comes from :<, :>, and :^ respectively with align() returning None if neither of those symbols are used in the formatted string. However, while padding is taken care of in the aligned cases, we could still have padding for things that don't use alignment like:

assert_eq!(format!("{:10}", Path::new("/foo/bar").display()), "/foo/bar  ");

We shouldn't write to f and return from there when there's no alignment; we should also include any potential padding/filling bytes here.

r? @joboet

[rustbot]

joboet is currently at their maximum review capacity.
They may take a while to respond.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[asder8215]

@rustbot ready

[joboet]

Great, thanks!
@bors r+

[rust-bors]

📌 Commit d6ff921 has been approved by joboet

It is now in the queue for this repository.

[JonathanBrouwer]

@rust-timer build 96b04ea

[rust-timer]

Finished benchmarking commit (96b04ea): comparison URL.

Overall result: ❌ regressions - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.2%, 0.8%]	3
Regressions ❌ (secondary)	0.8%	[0.8%, 0.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.2%, 0.8%]	3

Max RSS (memory usage)

Results (primary 2.6%, secondary 6.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	6.4%	[6.4%, 6.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.6%	[2.6%, 2.6%]	1

Cycles

Results (primary 2.3%, secondary 13.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	16.9%	[3.6%, 25.6%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	2.3%	[2.3%, 2.3%]	1

Binary size

Results (secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	-	-	0

Bootstrap: 481.835s -> 481.254s (-0.12%)
Artifact size: 397.84 MiB -> 395.89 MiB (-0.49%)

[asder8215]

I'm assuming LLVM optimizes away dead for loops for l_pad in (0, padding) or r_pad in (padding, 0). But I can see in most cases that Alignment:None has no padding (r_pad would be 0 in most cases), which I'm not sure if that right padding for loop would be optimized away.

I'm wondering if this is something that we can optimize in the code?

[JonathanBrouwer]

Note that the regressions are all in doc builds, not sure why