Skip to content

Formatter: discarded checked_sub never reserves separator width in one-per-line SeparatedExprs #6085

Description

@prql-bot

The nightly code-quality survey flagged a discarded computation in the code formatter that looks like it was meant to reserve width for a trailing separator but doesn't.

In SeparatedExprs::write, the one-per-line branch does:

opt.reset_line()?;
opt.rem_width.checked_sub(self.line_end.len() as u16)?;   // result discarded

r += &expr.write(opt.clone())?;
r += self.line_end;

codegen/mod.rs:168-172

checked_sub is non-mutating, so its result is thrown away — the line only functions as an underflow guard (via ?), it never actually reserves the line_end width before expr.write. Compare the sibling write_inline path, which uses the mutating consume_width to charge width against rem_width.

line_end is "," for tuples and arrays (ast.rs:103-114). Because the separator width is never reserved, an element that exactly fills the remaining width leaves no room for the trailing ,, so that formatted line can exceed max_width by one character.

This is a minor cosmetic edge case, not a correctness bug in the emitted PRQL. But the current line is either a latent off-by-line_end.len() bug (should be opt.consume_width(self.line_end.len() as u16)?; to actually reserve the separator) or the reservation is unwanted and the line is redundant. Which of the two is the intended behavior is a formatter-design call, and changing it would shift fmt snapshot output for elements near the width boundary — so flagging for a maintainer decision rather than opening a speculative PR.

Found during the nightly survey of codegen/mod.rs.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions