Updating positional-args check to be more comprehensive#1393
Conversation
oreflow
force-pushed
the
positional-args
branch
2 times, most recently
from
f23947f to
0a68444
Compare
| // Check whether fn.name is a rule | ||
| if fileData.rules[fn.name] { | ||
| // Check whether fn.name is a rule or macro | ||
| if fileData.rulesOrMacros[fn.name] { |
There was a problem hiding this comment.
it's weird that we collect rules and macros in this list, but set report.isMacro to true. The description of this function says "whether the given function is a macro". You need to update the description, the field name and the function name, or split the rulesOrMacros list into 2, probably.
There was a problem hiding this comment.
+1, in essence this comes from the history of this only being called externally on def statements, in which it was being used to determine whether the function implemented was a macro or not, but then recursively it was used on all calls, as a macro is defined as a function which calls a macro or a rule.
Updated all the isMacro references to be isRuleOrMacro
There was a problem hiding this comment.
Realized this comment was more related to this being a rule or not, and the update was related to handling calls to create symbolic macros... in hindsight this should probably go as a separate PR
| "native-sh-binary": NativeShellRulesWarning("sh_binary"), | ||
| "native-sh-library": NativeShellRulesWarning("sh_library"), | ||
| "native-sh-test": NativeShellRulesWarning("sh_test"), | ||
| "positional-args": positionalArgumentsWarning, |
There was a problem hiding this comment.
Out of curiosity: why is this change needed?
There was a problem hiding this comment.
The function definition of the previous is designed to run for a single Expr, this one is for the entire file
- Previous one does not receive the full file as input
- Previous one can only return a single finding.
oreflow
force-pushed
the
positional-args
branch
from
0a68444 to
65e69d3
Compare
- Previous implementation just assumed that macros were called at the root of a BUILD file, and that all calls were to macros/rules except for explicit allowlist. Updating the check to more precisely validate whether something is a rule/macro, and checking all call expressions (including ones nested in dicts/attrs). - Also updating IsMacro implementation to correctly support symblic macro definitions (`my_macro = macro(...)`)
oreflow
force-pushed
the
positional-args
branch
from
65e69d3 to
1652a7a
Compare
2 participants
my_macro = macro(...))