SONARARMOR-817 Parser should serialize TSModuleDeclaration nodes #5226

roberto-orlandi-sonarsource · 2025-03-20T17:00:52Z

Part of

roberto-orlandi-sonarsource · 2025-03-24T09:54:36Z

There is one issue on Next which, according to the way the file is implemented, I am not sure I should fix. Please let me know if I can accept it.

zglicz

Not much to add about the newly added functionality. It follows the same pattern as before.

It would be interesting to revisit how all of this code is generated and maintained. There are many 1000s of repetitive lines that do not contain difficult logic. These comments are outside of the scope of the PR though.

zglicz · 2025-03-24T14:54:40Z

packages/jsts/src/parsers/ast.ts

+function visitTSModuleBlock(node: TSESTree.TSModuleBlock) {
+  return {
+    body: node.body.map(visitNode),
+  };
+}
+
+function visitTSModuleDeclaration(node: TSESTree.TSModuleDeclaration) {
+  return {
+    id: visitNode(node.id),
+    body: visitNode(node.body),
+    kind: node.kind,
+  };
+}
+


As I'm reading and looking through this code, there is just so much redundancy and re-inventing the wheel. This seems like it all could be generated or semi-generated. Do you think at some point you would re-think this implementation?

I agree that this could be improved. Do you think it should be us to rethink this implementation? As far as I know, the Web squad owns and is responsible for this part of the code. I may be wrong because I was not part of the effort when this was created. In such a case, we could consider it in the future (but probably not very soon since we have a lot of other priorities at the moment)

zglicz · 2025-03-24T15:00:33Z

packages/jsts/tests/parsers/ast.test.ts

+    const tSModuleDeclaration = protoMessage.program.body[0].tSModuleDeclaration;
+    expect(tSModuleDeclaration.kind).toEqual('namespace');
+    expect(tSModuleDeclaration.id.type).toEqual(NODE_TYPE_ENUM.values['IdentifierType']);
+    expect(tSModuleDeclaration.id.identifier.name).toEqual('Foo');
+    expect(tSModuleDeclaration.body.type).toEqual(NODE_TYPE_ENUM.values['TSModuleBlockType']);
+    expect(tSModuleDeclaration.body.tSModuleBlock.body[0].type).toEqual(
+      NODE_TYPE_ENUM.values['VariableDeclarationType'],
+    );


what is this tSModuleDeclaration ? is it a class or a plain object? If its an object and you are asserting many properties, would it be more pleasant to compare it like so? With this setup, it's more visible to the reader of the code of the exact nesting and/or structure. However, as I was typing this out, it did feel a bit longish..

Suggested change

const tSModuleDeclaration = protoMessage.program.body[0].tSModuleDeclaration;

expect(tSModuleDeclaration.kind).toEqual('namespace');

expect(tSModuleDeclaration.id.type).toEqual(NODE_TYPE_ENUM.values['IdentifierType']);

expect(tSModuleDeclaration.id.identifier.name).toEqual('Foo');

expect(tSModuleDeclaration.body.type).toEqual(NODE_TYPE_ENUM.values['TSModuleBlockType']);

expect(tSModuleDeclaration.body.tSModuleBlock.body[0].type).toEqual(

NODE_TYPE_ENUM.values['VariableDeclarationType'],

);

expect(protoMessage.program.body[0].tSModuleDeclaration).toStrictEqual({ // or toEqual or toMatchObject

kind: 'namespace',

id: {

type: NODE_TYPE_ENUM.values['IdentifierType'],

identifier: {

name: 'Foo',

}

},

body: {

type: NODE_TYPE_ENUM.values['TSModuleBlockType'],

tSModuleBlock: {

body: [

{

type: NODE_TYPE_ENUM.values['VariableDeclarationType']

}

]

}

}

});

While I like the idea you suggest, having this comparison would make this test super-verbose because to satisfy the equality, there are a lot of other fields that need to be added to your suggestion (locations and the well-constructed VariableDeclaration, which has quite a few nested fields as well, for example).
I think that considering this, the current implementation makes more sense. WDYT?

zglicz · 2025-03-24T15:03:34Z

packages/jsts/tests/parsers/ast.test.ts

+    const serialized = serializeInProtobuf(ast as TSESTree.Program, 'foo.ts');
+    const deserializedProtoMessage = deserializeProtobuf(serialized);
+    compareASTs(protoMessage, deserializedProtoMessage);


is this the same in the entire test suite? Can this be extracted someplace or at least a single call across all of these tests

Good idea, I extracted those three lines to a function

zglicz · 2025-03-24T15:05:55Z

sonar-plugin/api/src/test/java/org/sonar/plugins/javascript/api/estree/ESTreeTest.java

  @Test
  void test() {
    Class<?>[] classes = ESTree.class.getDeclaredClasses();
-    assertThat(classes).hasSize(113);
+    assertThat(classes).hasSize(116);

    //filter all classes that are interface
    var ifaceCount = Arrays.stream(classes).filter(Class::isInterface).count();
-    assertThat(ifaceCount).isEqualTo(27);
+    assertThat(ifaceCount).isEqualTo(28);

    var recordCount = Arrays.stream(classes).filter(Class::isRecord).count();
-    assertThat(recordCount).isEqualTo(81);
+    assertThat(recordCount).isEqualTo(83);
  }


What is this testing? You add unit tests to verify the business logic and that should be enough.

Should this be removed? It's not adding value. It didn't catch that you are doing anything wrong, just added noise by making you update this.

This was an existing test. I just adapted the result based on the modifications I did.

Maybe lets remove?

While I agree with not seeing value in those UTs, I would not make removing them part of this PR. I don't feel comfortable removing tests that I did not write, and that I don't know why they were added in the first place.

zglicz · 2025-03-24T15:06:26Z

sonar-plugin/api/src/test/java/org/sonar/plugins/javascript/api/estree/ESTreeTest.java

  @Test
  void test_node_subclasses() {
    Class<?> sealedClass = ESTree.Node.class;
    Class<?>[] permittedSubclasses = sealedClass.getPermittedSubclasses();
-    assertThat(permittedSubclasses).hasSize(24);
+    assertThat(permittedSubclasses).hasSize(25);
  }


same question. Perhaps, I'm not understanding something about the value of these.

Same here. This test was just adapted.

zglicz · 2025-03-24T15:09:33Z

sonar-plugin/bridge/src/test/java/org/sonar/plugins/javascript/bridge/ESTreeFactoryTest.java

+  @Test
+  void should_create_ts_module_block() {
+    Node emptyStatementNode = Node.newBuilder()
+      .setType(NodeType.EmptyStatementType)
+      .setEmptyStatement(EmptyStatement.newBuilder().build())
+      .build();
+    TSModuleBlock tsModuleBlock = TSModuleBlock.newBuilder().addBody(emptyStatementNode).build();
+    Node protobufNode = Node.newBuilder()
+      .setType(NodeType.TSModuleBlockType)
+      .setTSModuleBlock(tsModuleBlock)
+      .build();
+
+    ESTree.Node estree = ESTreeFactory.from(protobufNode, ESTree.Node.class);
+    assertThat(estree).isInstanceOfSatisfying(ESTree.TSModuleBlock.class, block -> {
+      assertThat(block.body()).hasSize(1);
+      assertThat(block.body().get(0)).isInstanceOf(ESTree.EmptyStatement.class);
+    });
+  }
+
+  @Test
+  void should_create_ts_module_declaration() {
+    TSModuleDeclaration tsModuleDeclaration = TSModuleDeclaration.newBuilder()
+      .setId(Node.newBuilder().setType(NodeType.IdentifierType).build())
+      .setBody(Node.newBuilder().setType(NodeType.TSModuleBlockType).build())
+      .setKind("module")
+      .build();
+
+    Node protobufNode = Node.newBuilder()
+      .setType(NodeType.TSModuleDeclarationType)
+      .setTSModuleDeclaration(tsModuleDeclaration)
+      .build();
+
+    ESTree.Node estree = ESTreeFactory.from(protobufNode, ESTree.Node.class);
+    assertThat(estree).isInstanceOfSatisfying(ESTree.TSModuleDeclaration.class, module -> {
+      assertThat(module.id()).isInstanceOf(ESTree.Identifier.class);
+      assertThat(module.body()).isPresent();
+      assertThat(module.body().get()).isInstanceOf(ESTree.TSModuleBlock.class);
+      assertThat(module.kind()).isEqualTo("module");
+    });
+  }
+
+  @Test
+  void should_create_ts_module_declaration_without_body() {
+    TSModuleDeclaration tsModuleDeclaration = TSModuleDeclaration.newBuilder()
+      .setId(Node.newBuilder().setType(NodeType.IdentifierType).build())
+      .setKind("module")
+      .build();
+
+    Node protobufNode = Node.newBuilder()
+      .setType(NodeType.TSModuleDeclarationType)
+      .setTSModuleDeclaration(tsModuleDeclaration)
+      .build();
+
+    ESTree.Node estree = ESTreeFactory.from(protobufNode, ESTree.Node.class);
+    assertThat(estree).isInstanceOfSatisfying(ESTree.TSModuleDeclaration.class, module -> {
+      assertThat(module.id()).isInstanceOf(ESTree.Identifier.class);
+      assertThat(module.body()).isEmpty();
+      assertThat(module.kind()).isEqualTo("module");
+    });
+  }


wow, this is so verbose. Almost make me think that a simpler, more maintainable way would be to have a generic node, with a field: type (for the TS/JS node name) and props (for the map of properties, which can again be primitives or further nodes) - almost like a json.

Very verbose indeed. I went along with the existing UTs style. Finding a more maintainable implementation could be nice in the future.

roberto-orlandi-sonarsource · 2025-03-24T16:12:27Z

Not much to add about the newly added functionality. It follows the same pattern as before.

It would be interesting to revisit how all of this code is generated and maintained. There are many 1000s of repetitive lines that do not contain difficult logic. These comments are outside of the scope of the PR though.

Thanks for the review. As you mentioned, I think your comments are valid but probably out of the scope of this PR (at least the ones regarding bigger refactoring or changing tests not strictly related to this PR). I will merge as soon as the PR is green again, but happy to continue the open discussions.

sonarqube-next · 2025-03-25T12:18:37Z

Quality Gate failed

Failed conditions
66.7% Coverage on New Code (required ≥ 90%)

See analysis details on SonarQube

roberto-orlandi-sonarsource force-pushed the rob/SONARARMOR-817 branch from b165599 to 1306b70 Compare March 20, 2025 21:43

Base automatically changed from rob/SONARARMOR-820 to master March 24, 2025 08:48

roberto-orlandi-sonarsource force-pushed the rob/SONARARMOR-817 branch 2 times, most recently from adcda81 to 8a06da2 Compare March 24, 2025 09:32

roberto-orlandi-sonarsource marked this pull request as ready for review March 24, 2025 09:54

roberto-orlandi-sonarsource requested a review from a team March 24, 2025 09:56

zglicz approved these changes Mar 24, 2025

View reviewed changes

roberto-orlandi-sonarsource force-pushed the rob/SONARARMOR-817 branch 3 times, most recently from 45aed56 to 75472b3 Compare March 25, 2025 09:38

roberto-orlandi-sonarsource added 3 commits March 25, 2025 12:34

SONARARMOR-817 Parser should serialize TSModuleDeclaration nodes

092482e

review

31223e7

Trigger Build

00301e5

zglicz force-pushed the rob/SONARARMOR-817 branch from 40fccfe to 00301e5 Compare March 25, 2025 11:35

zglicz enabled auto-merge (squash) March 25, 2025 11:35

zglicz added 2 commits March 25, 2025 13:04

fix test

e829fe7

rebase

370c821

zglicz merged commit fb70e6b into master Mar 25, 2025
15 of 17 checks passed

zglicz deleted the rob/SONARARMOR-817 branch March 25, 2025 12:21

-    const tSModuleDeclaration = protoMessage.program.body[0].tSModuleDeclaration;
-    expect(tSModuleDeclaration.kind).toEqual('namespace');
-    expect(tSModuleDeclaration.id.type).toEqual(NODE_TYPE_ENUM.values['IdentifierType']);
-    expect(tSModuleDeclaration.id.identifier.name).toEqual('Foo');
-    expect(tSModuleDeclaration.body.type).toEqual(NODE_TYPE_ENUM.values['TSModuleBlockType']);
-    expect(tSModuleDeclaration.body.tSModuleBlock.body[0].type).toEqual(
-      NODE_TYPE_ENUM.values['VariableDeclarationType'],
-    );
+    expect(protoMessage.program.body[0].tSModuleDeclaration).toStrictEqual({ // or toEqual or toMatchObject
+      kind: 'namespace',
+      id: {
+        type: NODE_TYPE_ENUM.values['IdentifierType'],
+        identifier: {
+          name: 'Foo',
+        }
+      },
+      body: {
+        type: NODE_TYPE_ENUM.values['TSModuleBlockType'],
+        tSModuleBlock: {
+          body: [
+            {
+              type: NODE_TYPE_ENUM.values['VariableDeclarationType']
+            }
+          ]
+        }
+      }
+    });

SONARARMOR-817 Parser should serialize TSModuleDeclaration nodes #5226

SONARARMOR-817 Parser should serialize TSModuleDeclaration nodes #5226

Uh oh!

Conversation

roberto-orlandi-sonarsource commented Mar 20, 2025 • edited by jira bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roberto-orlandi-sonarsource commented Mar 24, 2025

Uh oh!

zglicz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roberto-orlandi-sonarsource commented Mar 24, 2025

Uh oh!

sonarqube-next bot commented Mar 25, 2025

Quality Gate failed

Uh oh!

Uh oh!

Uh oh!

roberto-orlandi-sonarsource commented Mar 20, 2025 •

edited by jira bot

Loading