search

nspcc-dev / neo-go

Go Node and SDK for the Neo blockchain
MIT License
123 stars 79 forks source link

neotest: exclude RET statements from coverage #3617

Closed AnnaShaleva closed 1 month ago

AnnaShaleva commented 1 month ago

The sequence point bounds of all RET statements include the whole method body, for example: https://github.com/nspcc-dev/neo-go/blob/86ed214e8a53859fd85482370be20eb613c61419/pkg/compiler/codegen.go#L542-L543

Such behaviour breaks the resulting coverage report. A part of #3559.

AnnaShaleva commented 1 month ago

@Slava0135 you may take a look if you'd like to.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.

Project coverage is 85.13%. Comparing base (86ed214) to head (de4b133). Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
pkg/neotest/coverage.go 0.00% 15 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3617 +/- ## ========================================== - Coverage 85.15% 85.13% -0.03% ========================================== Files 333 333 Lines 39022 39036 +14 ========================================== + Hits 33231 33234 +3 - Misses 4221 4233 +12 + Partials 1570 1569 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Slava0135 commented 1 month ago

If overlaps can only ever happen with these RET ops then we don't need #3575

Slava0135 commented 1 month ago

(though maybe take the tests from there, should have been a separate PR)

AnnaShaleva commented 1 month ago

If overlaps can only ever happen with these RET ops

No, there are multiple reasons for range overlapping, another one is described in https://github.com/nspcc-dev/neo-go/issues/3559#issuecomment-2414148935:

string(id[:len(estimateKeyPrefix)]) != estimateKeyPrefix {

And even this is not the last place where instructions may overlap.

roman-khimov commented 1 month ago

Can you make some simple example of C# contract compiled with debug info? How does it treat these RETs that it also inevitably has? Likely it does something similar and in that sense it's the coverage tool job to process data appropriately. And in that sense the fix is OK.

AnnaShaleva commented 1 month ago

Can you make some simple example of C# contract compiled with debug info?

Four hours of attempts to build compiler, to make the released binary work and to compile a simple contract, I'm done with it. No working instructions, no clear documentation, no fresh NuGet modules. And they generate .debug.json file ZIPped into .nefdbgnfo.

The results are:

  1. First of all, overlapping sequence point records are present for statements like this: if (IsOwner() == false), which is quite normal. For the given example the resulting records are:

            "68[0]39:30-39:35",
        "70[0]39:17-39:35",

    Hence, #3559 is a relevant issue even if we don't take into account overlapping RET statements.

  2. Regarding "return" statements: in C# debug info there's no such large overlapping sequence points in the end of functions. For both void and non-void functions all RET opcodes have their own range which always correspond to the closing bracket of the function. For void functions this approach is understandable, because there's no return keyword in the end of function. But for non-void functions it's an interesting case because return keyword corresponds to JMP instruction which jumps to the RET opcode, and this RET opcode has a sequence point pointing to }. For example, the following method: image Has the following sequence points generated:

      {
     "id" : "Neo.SmartContract.Template.sharpContractTemplate.GetOwner()",
     "name" : "Neo.SmartContract.Template.sharpContractTemplate,GetOwner",
     "params" : [],
     "range" : "0-20",
     "return" : "Hash160",
     "sequence-points" : [
        "0[0]26:41-26:63",
        "3[0]26:41-26:63",
        "5[0]26:29-26:64",
        "7[0]26:20-26:64",
        "8[0]26:20-26:64",
        "9[0]26:20-26:64",
        "11[0]26:20-26:64",
        "12[0]26:20-26:64",
        "13[0]26:20-26:64",
        "15[0]26:20-26:64",
        "17[0]26:20-26:64",
        "18[0]26:13-26:65",
        "20[0]27:9-27:10"
     ],
     "variables" : []
  },

    And the following instructions range:

    INDEX    OPCODE       PARAMETER
0        PUSHDATA1    ff    <<
3        CONVERT      Buffer (30)
5        CALL         21 (16/10)
7        DUP          
8        ISNULL       
9        JMPIF        18 (9/09)
11       DUP          
12       SIZE         
13       PUSHINT8     20 (14)
15       JMPEQ        18 (3/03)
17       THROW        
18       JMP          20 (2/02)
20       RET

    Pay attention to instructions 18 and 20 and their sequence points, it's the described approach.

All in all, these "large" sequence points generated by Go compiler for some RET statements is a bug, will create an issue for that. But it's not easy to fix because we use two different approaches to sequence point generation, from what I saw, Go compiler doesn't bind any instructions to }.

  1. C# compiler is better that ours in terms of variety of generated debug information, thanks to Harry. It collects more sequence points for a wide range of statements and outputs more detailed debug info.
AnnaShaleva commented 1 month ago

Based on the comment above I'm closing this PR, because it's not a solution.

AnnaShaleva commented 1 month ago

Closed in favour of https://github.com/nspcc-dev/neo-go/issues/3622.