warn on empty precision

[hkBst]

Fixes #131159 by erroring warning on missing precision. Alternatively we could document current behavior.

[rustbot]

r? @petrochenkov

rustbot has assigned @petrochenkov.
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

[oli-obk]

@bors try

[oli-obk]

Please add a ui test for this

[bors]

⌛ Trying commit ef04c64 with merge 77e9660...

[bors]

☀️ Try build successful - checks-actions
Build commit: 77e9660 (77e966048a5976133ba6b8f26d38d558f2a84355)

[oli-obk]

@craterbot check

[craterbot]

👌 Experiment pr-136638 created and queued.
🤖 Automatically detected try build 77e9660
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-136638 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-136638 is completed!
📊 365 regressed and 5 fixed (578841 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[m-ou-se]

I don't think we should merge this, as I mentioned above:

A default placeholder can be written as {} or {:} or {:.}. Should we also warn about {:}? I don't think we should. But then I'm not sure why we would warn about {:.}.

It seems that in every single occurence of {:.}, the author got exactly what they wanted: a placeholder with default settings. A warning would make sense if it is an easy mistake that looks very similar to something with a different meaning, but that's not the case here.

I think instead we should fix the reference, as mentioned by @traviscross above: #136638 (comment)

@rfcbot close

[rfcbot]

Team member @m-ou-se has proposed to close this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @the8472

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[hkBst]

It seems that in every single occurence of {:.}, the author got exactly what they wanted: a placeholder with default settings. A warning would make sense if it is an easy mistake that looks very similar to something with a different meaning, but that's not the case here.

This actually caught a bug, as I previously mentioned here: #136638 (comment)

[m-ou-se]

I think a clippy lint would be more appropriate.

[hkBst]

A default placeholder can be written as {} or {:} or {:.}. Should we also warn about {:}? I don't think we should. But then I'm not sure why we would warn about {:.}.

{:.} is merely a degenerate special case. In general {var:stuff.} will be warned against.

[hkBst]

@rustbot ready

[hkBst]

@rustbot label +A-fmt

[hkBst]

@samueltardieu: could you explain whether it is feasible to lint format macro syntax (like in this PR) within clippy?

[samueltardieu]

@samueltardieu: could you explain whether it is feasible to lint format macro syntax (like in this PR) within clippy?

We can do this as we have access to the parsed format specifier as well as the original source code. However, unless I'm mistaken, this might require an error-prone reparsing because:

• The precision in FormatOptions would be None in the presence of a sole ., so we don't have a span for the precision that we can check for emptyness.
• We have access to the whole placeholder span, but there are a lot of combinations to check for: {:.x} should lint, {:.x$} should not, {:.?} should lint, {:.<10} should not, etc.

This would be easier if the span of the precision was available outside the precision (so that we can get a None precision with an associated Some(_) span containing "."). Would this be worth it? Another option would be to add a FormatCount::Empty variant, but it would complicate the usage logic a bit (I haven't checked the details).

[rustbot]

This PR was rebased onto a different master 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.

[hkBst]

Perhaps an exception for "{:.}" (which would disable the warning) can help land this? Some options are (see latest commit):

• check whether the format string
  • contains "{:.}"
  • starts with "{:.}"
  • is equal to "{:.}" (or "{:.}\n")

[hkBst]

@m-ou-se I think I've addressed all your concerns, but if you're not convinced yet, would a clean crater run convince you?

[hkBst]

@dtolnay @joshtriplett I think I've addressed all @m-ou-se's concerns. Perhaps you'd like to take another look?