Page MenuHomePhabricator

Add a linter rule to detect mismatched parameters for formatted strings
ClosedPublic

Authored by joshuaspence on Feb 3 2015, 9:14 PM.
Tags
None
Referenced Files
F14403326: D11661.diff
Mon, Dec 23, 2:18 AM
Unknown Object (File)
Sat, Dec 21, 3:27 PM
Unknown Object (File)
Fri, Dec 20, 7:15 PM
Unknown Object (File)
Wed, Dec 18, 6:23 AM
Unknown Object (File)
Wed, Dec 18, 1:15 AM
Unknown Object (File)
Sun, Dec 15, 2:20 PM
Unknown Object (File)
Sun, Dec 15, 8:22 AM
Unknown Object (File)
Mon, Dec 9, 10:02 PM
Subscribers
Tokens
"Mountain of Wealth" token, awarded by epriestley.

Details

Summary

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

Repository
rARC Arcanist
Branch
master
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/lint/linter/ArcanistXHPASTLinter.php:3164XHP16TODO Comment
Unit
Tests Passed
Build Status
Buildable 4327
Build 4340: [Placeholder Plan] Wait for 30 Seconds

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.

src/lint/linter/ArcanistXHPASTLinter.php
184–185

Maybe document that map value is function parameter index.

3159

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".)

3169–3178

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
src/lint/linter/ArcanistXHPASTLinter.php
3159

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.