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.
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T7046: Add a linter rule for detecting missing variables for `pht`
- Commits
- rARCd8ba7e6f719f: Add a linter rule to detect mismatched parameters for formatted strings
This seems to work but needs some polish.
Diff Detail
- Repository
- rARC Arcanist
- Branch
- master
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 4332 Build 4345: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Catch this exception: InvalidArgumentException: xsprintf() does not support the "%0" modifier
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. |
- 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. |