fix: rework argvalue, allowing named arguments lookup#111
fix: rework argvalue, allowing named arguments lookup#111CrackTC wants to merge 1 commit intoantgroup:mainfrom
Conversation
CrackTC
requested review from
AntJiuFo,
Arielwyy and
alipaydeshui
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the representation of function arguments from arrays to records (Record<number | string, any>) across the analysis engine, including core analyzers for Java, JavaScript, Python, and Go. This change enables more robust handling of named arguments and variadic parameters. The review feedback identifies several critical issues resulting from this transition: a potential TypeError when iterating over records with for...of, premature loop termination when encountering falsy argument values, and overly restrictive type checks that might skip valid data. Addressing these points is essential to ensure the analyzer correctly processes all argument types without runtime errors.
| rest_argvalues.forEach((element: any, index: any) => { | ||
| rest_val[index.toString()] = element | ||
| }) | ||
| for (let j = 0; argvalues[i + j]; j++) { |
There was a problem hiding this comment.
The loop condition argvalues[i + j] will terminate the loop prematurely if an argument value is falsy (e.g., null or undefined). In JavaScript, variadic parameters should collect all remaining arguments even if they are null or undefined. It is safer to check for the existence of the index in the record.
| for (let j = 0; argvalues[i + j]; j++) { | |
| for (let j = 0; (i + j) in argvalues; j++) { |
| val = argvalues[index] | ||
| if (!val) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
The check if (!val) will skip arguments that are null or undefined. While undefined might indicate a missing argument, null is a valid value that should be passed to the function. Additionally, if val is a PrimitiveValue object representing 0 or false, it will be truthy, but if the system ever passes raw primitives here, they would be incorrectly skipped. It is safer to check if the key exists in the record.
| val = argvalues[index] | |
| if (!val) { | |
| continue | |
| } | |
| val = argvalues[index] | |
| if (val === undefined && !(index in argvalues)) { | |
| continue | |
| } |
|
|
||
| for (let i = 0; i < argvalues.length; i++) { | ||
| const arg = argvalues[i] | ||
| for (const arg of argvalues) { |
There was a problem hiding this comment.
argvalues is now typed as a Record, which can be a plain object. Using for...of on a plain object will throw a TypeError because objects are not iterable. To support both arrays and objects safely, use Object.values(argvalues) for iteration.
| for (const arg of argvalues) { | |
| for (const arg of Object.values(argvalues)) { |
| if (config.entryPointMode === 'ONLY_CUSTOM') return // 不路由自采集 | ||
| if (!(fclos._qid in registerServerPoints)) return // 处理Register_xxx_Server函数，即实现类注册点 | ||
| if (!Array.isArray(argvalues) || argvalues.length < 1) return | ||
| if (!Array.isArray(argvalues) || Object.keys(argvalues).length < 1) return |
There was a problem hiding this comment.
The Array.isArray(argvalues) check is too restrictive now that argvalues is a Record. If argvalues is passed as a plain object (as seen in the Python analyzer changes), this check will cause an early return and skip processing. You should check if it is truthy and has keys instead.
| if (!Array.isArray(argvalues) || Object.keys(argvalues).length < 1) return | |
| if (!argvalues || Object.keys(argvalues).length < 1) return |
1 participant
No description provided.