Instrument xml2js, bug fixes, fix linter warnings

timokoessler · timokoessler · commit 6a35ec5a9234 · 2025-06-05T14:10:27.000+02:00
diff --git a/instrumentation-wasm/src/js_transformer/helpers/get_name_str_for_member_expr.rs b/instrumentation-wasm/src/js_transformer/helpers/get_name_str_for_member_expr.rs
@@ -5,18 +5,13 @@ pub fn get_name_str_for_member_expr<'a>(
     allocator: &'a Allocator,
     member_expr: &MemberExpression,
 ) -> Option<&'a str> {
-    let obj_name_str: &str;
-    let prop_name_str: &str;
-
-    match member_expr {
+    let prop_name_str = match member_expr {
         MemberExpression::StaticMemberExpression(static_member_expr) => {
-            prop_name_str = static_member_expr.property.name.as_str()
+            static_member_expr.property.name.as_str()
         }
         MemberExpression::ComputedMemberExpression(computed_member_expr) => {
             match &computed_member_expr.expression {
-                Expression::Identifier(identifier_ref) => {
-                    prop_name_str = identifier_ref.name.as_str();
-                }
+                Expression::Identifier(identifier_ref) => identifier_ref.name.as_str(),
                 _ => {
                     // Unsupported AST type
                     return None;
@@ -27,17 +22,15 @@ pub fn get_name_str_for_member_expr<'a>(
             // Unsupported AST type
             return None;
         }
-    }
+    };
 
-    match member_expr.object() {
-        Expression::Identifier(identifier_ref) => {
-            obj_name_str = identifier_ref.name.as_str();
-        }
+    let obj_name_str = match member_expr.object() {
+        Expression::Identifier(identifier_ref) => identifier_ref.name.as_str(),
         _ => {
             // Unsupported AST type
             return None;
         }
-    }
+    };
 
     if obj_name_str.is_empty() || prop_name_str.is_empty() {
         return None;
@@ -47,5 +40,5 @@ pub fn get_name_str_for_member_expr<'a>(
         return Some(allocator.alloc_str(&format!("{}[{}]", obj_name_str, prop_name_str)));
     }
 
-    Some(&allocator.alloc_str(&format!("{}.{}", obj_name_str, prop_name_str)))
+    Some(allocator.alloc_str(&format!("{}.{}", obj_name_str, prop_name_str)))
 }
diff --git a/instrumentation-wasm/src/js_transformer/helpers/insert_code.rs b/instrumentation-wasm/src/js_transformer/helpers/insert_code.rs
@@ -1,10 +1,10 @@
 use oxc_allocator::{Allocator, Box, Vec as OxcVec};
 use oxc_ast::{
+    AstBuilder, NONE,
     ast::{
         Argument, ArrayExpressionElement, AssignmentOperator, AssignmentTarget, Expression,
         FunctionBody,
     },
-    AstBuilder, NONE,
 };
 use oxc_span::SPAN;
 
diff --git a/instrumentation-wasm/src/js_transformer/helpers/insert_import_statement.rs b/instrumentation-wasm/src/js_transformer/helpers/insert_import_statement.rs
@@ -1,12 +1,12 @@
 use oxc_allocator::{Allocator, Vec as OxcVec};
 use oxc_ast::{
+    AstBuilder, NONE,
     ast::{
         Argument, BindingPatternKind, BindingProperty, ImportDeclarationSpecifier,
         ImportOrExportKind, Statement, VariableDeclarationKind,
     },
-    AstBuilder, NONE,
 };
-use oxc_span::{SourceType, SPAN};
+use oxc_span::{SPAN, SourceType};
 
 use crate::js_transformer::instructions::{FileInstructions, FunctionInstructions};
 
@@ -15,7 +15,9 @@ const INSTRUMENT_INSPECT_ARGS_METHOD_NAME: &str = "__instrumentInspectArgs";
 const INSTRUMENT_MODIFY_ARGS_METHOD_NAME: &str = "__instrumentModifyArgs";
 const INSTRUMENT_MODIFY_RETURN_VALUE_METHOD_NAME: &str = "__instrumentModifyReturnValue";
 
-const IMPORT_METHODS: [(&str, fn(&FunctionInstructions) -> bool); 3] = [
+type ImportMethodPredicate = fn(&FunctionInstructions) -> bool;
+
+const IMPORT_METHODS: [(&str, ImportMethodPredicate); 3] = [
     (
         INSTRUMENT_INSPECT_ARGS_METHOD_NAME,
         |f: &FunctionInstructions| f.inspect_args,
@@ -51,7 +53,7 @@ pub fn insert_import_statement<'a>(
 
         for (method_name, predicate) in IMPORT_METHODS.iter() {
             // Only import the function if it is used in the file
-            if file_instructions.functions.iter().any(|f| predicate(f)) {
+            if file_instructions.functions.iter().any(predicate) {
                 binding_properties.push(builder.binding_property(
                     SPAN,
                     builder.property_key_static_identifier(SPAN, *method_name),
@@ -109,7 +111,7 @@ pub fn insert_import_statement<'a>(
 
     let mut specifiers: OxcVec<'a, ImportDeclarationSpecifier<'a>> = builder.vec_with_capacity(3);
     for (method_name, predicate) in IMPORT_METHODS.iter() {
-        if file_instructions.functions.iter().any(|f| predicate(f)) {
+        if file_instructions.functions.iter().any(predicate) {
             specifiers.push(builder.import_declaration_specifier_import_specifier(
                 SPAN,
                 builder.module_export_name_identifier_name(SPAN, *method_name),
diff --git a/instrumentation-wasm/src/js_transformer/helpers/insert_instrument_method_calls.rs b/instrumentation-wasm/src/js_transformer/helpers/insert_instrument_method_calls.rs
@@ -22,7 +22,7 @@ pub fn insert_instrument_method_calls<'a>(
             allocator,
             builder,
             &instruction.identifier,
-            &arg_names,
+            arg_names,
             body,
             instruction.modify_arguments_object,
         );
diff --git a/instrumentation-wasm/src/js_transformer/helpers/transform_return_statements.rs b/instrumentation-wasm/src/js_transformer/helpers/transform_return_statements.rs
@@ -1,7 +1,7 @@
 use oxc_allocator::{Allocator, Box, Vec as OxcVec};
 use oxc_ast::{
-    ast::{Argument, FunctionBody, Statement},
     AstBuilder, NONE,
+    ast::{Argument, FunctionBody, Statement},
 };
 use oxc_span::SPAN;
 
@@ -15,14 +15,18 @@ pub fn transform_return_statements<'a>(
     for statement in &mut body.statements {
         match statement {
             Statement::ReturnStatement(return_stmt) => {
-                let mut instrument_args: OxcVec<'a, Argument<'a>> = builder.vec_with_capacity(2);
+                let mut instrument_args: OxcVec<'a, Argument<'a>> = builder.vec_with_capacity(3);
 
                 // Add the identifier to the arguments
                 instrument_args.push(Argument::StringLiteral(builder.alloc_string_literal(
                     SPAN,
                     allocator.alloc_str(identifier),
                     None,
                 )));
+
+                // Also pass the function arguments
+                instrument_args.push(builder.expression_identifier(SPAN, "arguments").into());
+
                 // Add original return value as the second argument
                 let arg_expr = return_stmt
                     .argument
diff --git a/instrumentation-wasm/src/js_transformer/transformer.rs b/instrumentation-wasm/src/js_transformer/transformer.rs
@@ -28,16 +28,16 @@ pub fn transform_code_str(
 
     let source_type = select_sourcetype_based_on_enum(src_type);
 
-    let parser_result = Parser::new(&allocator, &code, source_type).parse();
+    let parser_result = Parser::new(&allocator, code, source_type).parse();
 
-    if parser_result.panicked || parser_result.errors.len() > 0 {
+    if parser_result.panicked || !parser_result.errors.is_empty() {
         return format!("#ERR: {:?}", parser_result.errors);
     }
 
     let program = allocator.alloc(parser_result.program);
 
     // 2 Semantic Analyze
-    let semantic = SemanticBuilder::new().build(&program);
+    let semantic = SemanticBuilder::new().build(program);
 
     if !semantic.errors.is_empty() {
         return format!("#ERR: {:?}", semantic.errors);
@@ -50,7 +50,7 @@ pub fn transform_code_str(
     let t = &mut Transformer {
         allocator: &allocator,
         file_instructions: &file_instructions,
-        pkg_version: &pkg_version,
+        pkg_version,
         ast_builder: &ast_builder,
     };
 
@@ -74,7 +74,7 @@ pub fn transform_code_str(
             // Todo add source map using source_map_path
             ..CodegenOptions::default()
         })
-        .build(&program);
+        .build(program);
 
     js.code
 }
diff --git a/instrumentation-wasm/src/js_transformer/transformer_impl.rs b/instrumentation-wasm/src/js_transformer/transformer_impl.rs
@@ -22,7 +22,7 @@ impl<'a> Traverse<'a> for Transformer<'a> {
         node: &mut MethodDefinition<'a>,
         _ctx: &mut TraverseCtx<'a>,
     ) {
-        if !node.key.is_identifier() || !node.value.body.is_some() || !node.key.name().is_some() {
+        if !node.key.is_identifier() || node.value.body.is_none() || node.key.name().is_none() {
             return;
         }
 
@@ -135,7 +135,7 @@ impl<'a> Traverse<'a> for Transformer<'a> {
             return;
         }
 
-        if !node.id.is_some() || !node.body.is_some() {
+        if node.id.is_none() || node.body.is_none() {
             // No identifier or body, nothing to instrument
             return;
         }
diff --git a/instrumentation-wasm/src/wasm_bindings/mod.rs b/instrumentation-wasm/src/wasm_bindings/mod.rs
@@ -10,5 +10,5 @@ pub fn wasm_transform_code_str(
     instructions_json: &str,
     source_type: i32,
 ) -> String {
-    return transform_code_str(pkg_name, pkg_version, code, instructions_json, source_type);
+    transform_code_str(pkg_name, pkg_version, code, instructions_json, source_type)
 }
diff --git a/library/agent/hooks/instrumentation/codeTransformation.test.ts b/library/agent/hooks/instrumentation/codeTransformation.test.ts
@@ -957,7 +957,7 @@ t.test("add modifyArgs to method definition (ESM)", async (t) => {
         testFunction(arg1) {
             console.log("test");
 
-            return __instrumentModifyReturnValue("testpkg.test.js.testFunction.MethodDefinition.v1.0.0", arg1);
+            return __instrumentModifyReturnValue("testpkg.test.js.testFunction.MethodDefinition.v1.0.0", arguments, arg1);
         }
     }`
   );
@@ -975,7 +975,7 @@ t.test("add modifyArgs to method definition (ESM)", async (t) => {
         testFunction(arg1) {
             console.log("test");
 
-            return __instrumentModifyReturnValue("testpkg.test.js.testFunction.MethodDefinition.v1.0.0", "test");
+            return __instrumentModifyReturnValue("testpkg.test.js.testFunction.MethodDefinition.v1.0.0", arguments,"test");
         }
     }`
   );
@@ -993,7 +993,7 @@ t.test("add modifyArgs to method definition (ESM)", async (t) => {
         testFunction(arg1) {
             console.log("test");
 
-            return __instrumentModifyReturnValue("testpkg.test.js.testFunction.MethodDefinition.v1.0.0", "test\\"");
+            return __instrumentModifyReturnValue("testpkg.test.js.testFunction.MethodDefinition.v1.0.0", arguments, "test\\"");
         }
     }`
   );
@@ -1011,7 +1011,7 @@ t.test("add modifyArgs to method definition (ESM)", async (t) => {
         testFunction(arg1) {
             console.log("test");
 
-            return __instrumentModifyReturnValue("testpkg.test.js.testFunction.MethodDefinition.v1.0.0", [1, 2]);
+            return __instrumentModifyReturnValue("testpkg.test.js.testFunction.MethodDefinition.v1.0.0", arguments, [1, 2]);
         }
     }`
   );
@@ -1029,7 +1029,7 @@ t.test("add modifyArgs to method definition (ESM)", async (t) => {
         testFunction(arg1) {
             console.log("test");
 
-            return __instrumentModifyReturnValue("testpkg.test.js.testFunction.MethodDefinition.v1.0.0", function() { return 42; });
+            return __instrumentModifyReturnValue("testpkg.test.js.testFunction.MethodDefinition.v1.0.0", arguments, function() { return 42; });
         }
     }`
   );
@@ -1049,7 +1049,7 @@ t.test("add modifyArgs to method definition (ESM)", async (t) => {
         testFunction(arg1) {
             console.log("test");
 
-            return __instrumentModifyReturnValue("testpkg.test.js.testFunction.MethodDefinition.v1.0.0", funcCall({foo: [1], test: Symbol("abc")}));
+            return __instrumentModifyReturnValue("testpkg.test.js.testFunction.MethodDefinition.v1.0.0", arguments, funcCall({foo: [1], test: Symbol("abc")}));
         }
     }`
   );
@@ -1194,7 +1194,7 @@ t.test("Modify function declaration (ESM)", async (t) => {
     function test123(arg1, arg2 = "default") {
         __instrumentInspectArgs("testpkg.application.js.test123.FunctionDeclaration.v1.0.0", arguments, "1.0.0", this);
         console.log("test123");
-        return __instrumentModifyReturnValue("testpkg.application.js.test123.FunctionDeclaration.v1.0.0","abc");
+        return __instrumentModifyReturnValue("testpkg.application.js.test123.FunctionDeclaration.v1.0.0", arguments, "abc");
     }
     const ignore = function test123() {
         console.log("ignore");
diff --git a/library/agent/hooks/instrumentation/loadHook.ts b/library/agent/hooks/instrumentation/loadHook.ts
@@ -135,8 +135,6 @@ function patchBuiltin(
   builtinName: string,
   previousLoadResult: ReturnType<LoadFunction>
 ) {
-  // Todo test if used with import-in-the-middle at the same time
-
   const builtinNameWithoutPrefix = removeNodePrefix(builtinName);
 
   const builtin = shouldPatchBuiltin(builtinNameWithoutPrefix);
diff --git a/library/eslint.config.mjs b/library/eslint.config.mjs
@@ -59,11 +59,4 @@ export default defineConfig([
       "@typescript-eslint/no-unused-vars": "off",
     },
   },
-  // New instrumentation system
-  {
-    files: ["**/agent/hooks/instrumentation/*.ts"],
-    rules: {
-      "no-warning-comments": "off",
-    },
-  },
 ]);
diff --git a/library/sources/XmlMinusJs.ts b/library/sources/XmlMinusJs.ts
@@ -55,6 +55,21 @@ export class XmlMinusJs implements Wrapper {
             return result;
           },
         });
+      })
+      // We don't need to wrap xml2json file with the new hook system, as it uses xml2js internally.
+      .addFileInstrumentation({
+        path: "lib/xml2js.js",
+        functions: [
+          {
+            name: "module.exports",
+            nodeType: "FunctionAssignment",
+            operationKind: "deserialize_op",
+            modifyReturnValue: (args, result) => {
+              this.inspectParse(args, result, false);
+              return result;
+            },
+          },
+        ],
       });
   }
 }

Original file line number	Diff line number	Diff line change
`@@ -22,7 +22,7 @@ impl<'a> Traverse<'a> for Transformer<'a> {`
`22`	`22`	`node: &mut MethodDefinition<'a>,`
`23`	`23`	`_ctx: &mut TraverseCtx<'a>,`
`24`	`24`	`) {`
`25`		`- if !node.key.is_identifier() \|\| !node.value.body.is_some() \|\| !node.key.name().is_some() {`
	`25`	`+ if !node.key.is_identifier() \|\| node.value.body.is_none() \|\| node.key.name().is_none() {`
`26`	`26`	`return;`
`27`	`27`	`}`
`28`	`28`
`@@ -135,7 +135,7 @@ impl<'a> Traverse<'a> for Transformer<'a> {`
`135`	`135`	`return;`
`136`	`136`	`}`
`137`	`137`
`138`		`- if !node.id.is_some() \|\| !node.body.is_some() {`
	`138`	`+ if node.id.is_none() \|\| node.body.is_none() {`
`139`	`139`	`// No identifier or body, nothing to instrument`
`140`	`140`	`return;`
`141`	`141`	`}`
Original file line number	Diff line number	Diff line change
`@@ -10,5 +10,5 @@ pub fn wasm_transform_code_str(`
`10`	`10`	`instructions_json: &str,`
`11`	`11`	`source_type: i32,`
`12`	`12`	`) -> String {`
`13`		`- return transform_code_str(pkg_name, pkg_version, code, instructions_json, source_type);`
	`13`	`+ transform_code_str(pkg_name, pkg_version, code, instructions_json, source_type)`
`14`	`14`	`}`