warn on empty precision

[hkBst]

View all comments

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

EDIT: Warnings for common case of "{:.}" are now disabled, so this should have a clean crater run.

[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

[Amanieu]

We discussed this in the libs-api meeting and decided that the best path forward would be a forward-compatibility warning which will give us the possibility of making this a hard error in the future. We definitely shouldn't make it an error immediately due to the widespread potential breakage.

[traviscross]

Probably this FCW should go ahead and just start by linting in deps.

[m-ou-se]

I'm not convinced that this needs a (forward-compatibility) warning.

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.

[Noratrieb]

I think a clippy lint would be reasonable, it's a needlessly complicated way to write {}. But I agree that we shouldn't be emitting future compat warnings for it, especially not in deps.

[traviscross]

The other option, of course, is to document the current behavior. We do get this right for {:}, because we document:

format := '{' [ argument ] [ ':' format_spec ] [ ws ] * '}'
format_spec := [[fill]align][sign]['#']['0'][width]['.' precision]type
type := '' | '?' | 'x?' | 'X?' | identifier

So since type includes '', we must accept {:}. But, for {:.}, we'd look at:

format := '{' [ argument ] [ ':' format_spec ] [ ws ] * '}'
argument := integer | identifier
format_spec := [[fill]align][sign]['#']['0'][width]['.' precision]type
precision := count | '*'
count := parameter | integer
parameter := argument '$'

Here, this bottoms out at requiring something after the dot.

We could say, instead, if we want, e.g.:

precision := count | '*' | ''

Since we do also accept today, e.g. {:.x}, that seems right. Or alternatively, and maybe more directly, we could say:

format_spec := [[fill]align][sign]['#']['0'][width]['.' [precision]]type

[Dylan-DPC]

@hkBst any updates on this? thanks

[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.
EDIT: And I've now exempted "{:.}" from being warned on to address this concern.

[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?

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[hkBst]

@rfcbot concern

I've addressed previously listed concerns with this implementation (such that the most common occurrence ("{:.}") is now not warned against any more) but have seen nothing to indicate anyone took this into consideration.

It is also false that this does not catch bugs, as a bug was caught during the initial crater run. My correction of this statement also seems not to have gotten any attention.

Given these facts, I cannot shake the feeling that some votes might be based on outdated or wrong information...

Of course it is possible that even with a full picture this solution is not preferred, but the alternatives do seem objectively worse:

• fix the documentation to allow for this (does not catch the bug this impl does)
• do this in clippy (seems to require that clippy does its own separate parsing of format strings)

[samueltardieu]

* do this in clippy (seems to require that clippy does its own separate parsing of format strings)

Or as I wrote in this comment provide Clippy with some extra information to do the job efficiently.

[Amanieu]

r? @m-ou-se

[m-ou-se]

@hkBst I still think the best course of action is to add this lint to Clippy first.