Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lua tests and fix for excessive binary_expression calculations #16

Merged
merged 5 commits into from
Apr 19, 2024

Conversation

themkat
Copy link
Contributor

@themkat themkat commented Apr 18, 2024

General

Test cases for Lua are mostly inspired by JavaScript. My idea is that it would be easier to work with if we kept some consistency with test names and what we are testing.

binary_expression error

I guess the original idea for the binary_expression rule was to test the logical expressions and their nesting? If not, please let me know 🙂

Almost everything in Lua is a binary_expression (overstatement, but still...). Let us take some quick examples of how many places they actually occur and see the calculations in the MAIN codebase. This is to show why the code fix was done.

Simple.lua

Notice that each little multiplication, equality check and string concatenation is a binary_expression. If we see the debug prints for the Simple.lua file, it looks like this:
image

We can easily verify that this is from the binary_expression operations with the output from tree-sitter:

chunk:
  function_declaration:
    identifier:
    parameters:
      identifier:
    block:
      return_statement:
        expression_list:
          binary_expression:
            identifier:
            identifier:
  function_call:
    identifier:
    arguments:
      binary_expression:
        string:
        function_call:
          identifier:
          arguments:
            number:
  for_statement:
    for_numeric_clause:
      identifier:
      number:
      number:
    block:
      if_statement:
        binary_expression:
          number:
          identifier:

The calculation is correct here, but that is due to no real nesting of binary_expressions.

If we try write a quad function instead of square, we see the issue quite clearly:

image

As you can see, we now get a score of 1 for the first multiplication. From my understanding of cognitive complexity, only logical expressions should increase the score. I see nothing in any documentation (e.g, the Sonar paper) that arithmetic should increase it.

Recursion.lua

To really drive the point home, let us look at the recursion example.
image

Here we see something similar. (one of the +1s is correct due to recursive call!). Because the multiplication n * factorial(n - 1) is a binary_expression that has a binary_expression (i.e, n - 1) in a child of child nodes, it will get a score. That is not (at least to my knowledge) something that should happen. If it should happen, then it is wrong for ALL other languages...

We could probably continue down this road, and check each test. I don't think that is necessary. Feel free to do so if you want 🙂

binary_expression fix

if it was not clear, this PR fixes the above consistencies. This is by changing the binary_expression checks to only give any score for logical operators that has a logical operator operation as a child. If not, the calculations above will happen resulting in the situation we see in the recursion example.

@jcs090218
Copy link
Member

Make sense to me! Thank you! 🚀

@jcs090218 jcs090218 merged commit 01faa3c into emacs-vs:master Apr 19, 2024
11 of 12 checks passed
@themkat themkat deleted the lua_tests branch April 19, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants