Skip to content

F507: Fix false negative for non-tuple RHS in %-formatting#24142

Merged
MichaReiser merged 2 commits intoastral-sh:mainfrom
RenzoMXD:fix/f507-non-tuple-rhs-count-mismatch
Mar 24, 2026
Merged

F507: Fix false negative for non-tuple RHS in %-formatting#24142
MichaReiser merged 2 commits intoastral-sh:mainfrom
RenzoMXD:fix/f507-non-tuple-rhs-count-mismatch

Conversation

@RenzoMXD
Copy link
Copy Markdown
Contributor

@RenzoMXD RenzoMXD commented Mar 23, 2026

Summary

Fixes #24031

F507 (percent-format-positional-count-mismatch) previously only checked tuple right-hand sides in %-format expressions. This meant literal non-tuple values like '%s %s' % 42 were silently ignored.

This PR extends F507 to also flag non-tuple RHS values when the format string expects a different number of positional arguments, using ResolvedPythonType::from() to infer the type of the RHS expression.

What gets flagged now

  • Literal values: 42, 3.14, "hello", b"hello", True, None, ..., f"hello {name}"
  • Compound expressions with known types: -1, 1 + 2, not x, "a" + "b", 1 if True else 2

What is intentionally NOT flagged

Variables, attribute accesses, subscripts, and calls are skipped because they could be tuples at runtime:

x = (1, 2)
'%s %s' % x  # valid Python — % unpacks tuples

Why ResolvedPythonType

Per reviewer suggestion, using ResolvedPythonType::from() instead of manually matching literal expression types catches more cases (unary ops, binary ops, ternary, etc.) and is more maintainable.

Context

The original implementation only checked Expr::Tuple because List and Set were intentionally removed in #5986'%s' % [1, 2, 3] is valid Python (lists aren't unpacked by %). This PR takes the same conservative approach by only flagging cases where we can statically determine the type is not a tuple.

Test plan

  • Added test cases for all literal types (int, float, string, bytes, bool, None, ellipsis, f-string) with multiple placeholders
  • Added test cases for compound expressions resolved via ResolvedPythonType (unary, binary, boolean, ternary)
  • Added test cases for single-placeholder with literal RHS (not flagged)
  • Added test cases for variables, attribute access, subscripts, calls, ternary with unknowns, binary ops with unknowns (not flagged)
  • All 469 existing pyflakes tests pass

@astral-sh-bot astral-sh-bot bot requested a review from ntBre March 23, 2026 17:40
@kaddkaka
Copy link
Copy Markdown

kaddkaka commented Mar 23, 2026

Does this also solve the case where the LHS is an f-string?

f"{banana}" % banana

@RenzoMXD
Copy link
Copy Markdown
Contributor Author

No, it doesn't. That's a separate issue.

The LHS check happens in expression.rs:1442 — it only matches Expr::StringLiteral:

if let Expr::StringLiteral(format_string @ ast::ExprStringLiteral { value, .. }) = left.as_ref()

So when the LHS is an f-string (Expr::FString), none of the F50x rules fire at all. My fix only touches the RHS handling within F507.

I will make a separate PR for that.

@RenzoMXD
Copy link
Copy Markdown
Contributor Author

@ntBre Could you please review my PR?
I will make another PR that solves the case where the LHS is an f-string.

@kaddkaka
Copy link
Copy Markdown

kaddkaka commented Mar 23, 2026

No, it doesn't. That's a separate issue.

The LHS check happens in expression.rs:1442 — it only matches Expr::StringLiteral:

if let Expr::StringLiteral(format_string @ ast::ExprStringLiteral { value, .. }) = left.as_ref()

So when the LHS is an f-string (Expr::FString), none of the F50x rules fire at all. My fix only touches the RHS handling within F507.

I will make a separate PR for that.

OK, but perhaps a case with 0 placeholders and 1 item in right hand side should be considered.

I'm not sure if this case can be covered without risk of false positive:

"no placeholder" % banana

What happens for this case if banana is an empty tuple? I suppose for this particular case, with a literal string LHS, any use of % (no matters what's on RHS) could give a warning.

@RenzoMXD
Copy link
Copy Markdown
Contributor Author

Good point! The current implementation already covers that case — "no placeholder" % 42 would be flagged since num_positional is 0 and a literal counts as 1 substitution (0 != 1).

For "no placeholder" % banana, we intentionally don't flag it because banana could be an empty tuple () at runtime, which would be valid. This is consistent with the conservative approach of only flagging literals where we can be certain about the value.

@MichaReiser MichaReiser added rule Implementing or modifying a lint rule accepted Ready for implementation labels Mar 24, 2026
@MichaReiser MichaReiser changed the title Fix F507 false negative for literal non-tuple RHS in %-formatting F507: Fix false negative for non-tuple RHS in %-formatting Mar 24, 2026
@MichaReiser MichaReiser merged commit 6cff034 into astral-sh:main Mar 24, 2026
41 checks passed
@RenzoMXD RenzoMXD deleted the fix/f507-non-tuple-rhs-count-mismatch branch March 24, 2026 14:20
carljm added a commit that referenced this pull request Mar 25, 2026
* main:
  [ty] Avoid eager TypedDict diagnostics in `TypedDict | dict` unions (#24151)
  `F507`: Fix false negative for non-tuple RHS in `%`-formatting (#24142)
  [ty] Update `SpecializationBuilder` hook to get both lower/upper bounds (#23848)
  Fix `%foo?` parsing in IPython assignment expressions (#24152)
  `E501`/`W505`/formatter: Exclude nested pragma comments from line width calculation  (#24071)
  [ty] Fix Salsa panic propagation (#24141)
  [ty] Support `type:ignore[ty:code]` suppressions (#24096)
  [ty] Support narrowing for extended walrus targets (#24129)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accepted Ready for implementation rule Implementing or modifying a lint rule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mismatching number of %-placeholders in string formatting

4 participants