Page MenuHomePhabricator

Add a linter rule to detect mismatched parameters for formatted strings

Authored by joshuaspence on Feb 3 2015, 9:14 PM.
Referenced Files
Unknown Object (File)
Thu, Mar 16, 12:57 PM
Unknown Object (File)
Thu, Mar 16, 12:33 PM
Unknown Object (File)
Wed, Mar 1, 5:37 AM
Unknown Object (File)
Fri, Feb 24, 4:20 AM
Unknown Object (File)
Wed, Feb 22, 11:00 AM
Unknown Object (File)
Feb 8 2023, 1:51 AM
Unknown Object (File)
Jan 24 2023, 6:16 AM
Unknown Object (File)
Jan 24 2023, 6:16 AM
"Mountain of Wealth" token, awarded by epriestley.



Fixes T7046. Adds a linter rule to detect mismatched parameters for formatted strings. Originally I had considered putting this rule in ArcanistPhutilXHPASTLinter, but I later decided to move it to ArcanistXHPASTLinter as I think that it is general enough to be more widely useful.

Test Plan

This seems to work but needs some polish.

Diff Detail

rARC Arcanist
Lint Not Applicable
Tests Not Applicable

Event Timeline

joshuaspence retitled this revision from to Add a linter rule to detect mismatched parameters for formatted strings.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
joshuaspence edited edge metadata.

Move to ArcanistXHPASTLinter

Make xhpast.printf-functions actually work

Catch this exception: InvalidArgumentException: xsprintf() does not support the "%0" modifier

epriestley edited edge metadata.

This is really nice.


Maybe document that map value is function parameter index.


This could probably warn, I don't think there are any valid use cases for any of these functions with too-few parameters. Can you come up with a plausible case with any of the existing functions or some theoretical future function where the formatting string could be optional?

(Even if we can, we could change the map to <string, wild> and let you specify either an integer for "sane function with reasonable behavior and exactly one format string" or a dictionary of options for "special function which can be called with zero/more-than-one format string".)


This is pretty clever.

Since xsprintf() doesn't support everything that sprintf() does (like %3$d) it will miss issues with a few unusual format strings that sprintf() can handle but xsprintf() can not. However, AFAIK we don't use any of these features anywhere except in translations. I think we're fairly permissive about the parsing, so it should only have false negatives (except maybe %'%d for a %-padded integer? But that's such a ridiculous edge case that I'm not concerned about getting it wrong).

Specifically, I'd expect these cases to be wrong:

// False negative? We *could* catch this statically (but seems reasonable not to).
sprintf('%17$s', 'quack');

// False positive? This is technically correct as written (just ridiculous).
sprintf("%'%6.6d", 123);

Neither case seems important to me.

This revision is now accepted and ready to land.Feb 9 2015, 2:58 PM
joshuaspence edited edge metadata.
  • Clarify help text
  • Use a dummy callback for xsprintf

So this is the case for function calls without format things (e.g. printf() or pht()). I suppose it's reasonable that we raise a linter warning here as well.

  • Check for missing format string
This revision was automatically updated to reflect the committed changes.