neotest: exclude RET statements from coverage #3617

AnnaShaleva · 2024-10-16T15:44:17Z

The sequence point bounds of all RET statements include the whole method body, for example:

Lines 542 to 543 in 86ed214

    
           c.saveSequencePoint(decl.Body) 
        
           emit.Opcodes(c.prog.BinWriter, opcode.RET)

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

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. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva · 2024-10-16T15:46:24Z

pkg/neotest/coverage.go

+				// which break the resulting coverage report.
+				_, isRET := scriptRawCoverage.methodEndOffsets[uint16(point.Opcode)]
+				if isRET {
+					continue


That was my best attempt. I also tried to keep return statements in the coverage but "shorten" their bounds to a single "end" line, but it's not displayed properly and sometimes even wrong when there's no explicit "return" statement in the code. Hence, I decided to remove these sequence points from coverage.

Although I'm still not sure if it's a good solution because: it does not exclude those RET statements that are not in the end of the method. Although such RET statements have proper bounds. So may be it's even bug in the compiler and RET statements shouldn't actually have such method-body-width bounds.

And if it's a compiler bug:

neo-go/pkg/compiler/codegen.go

Lines 539 to 543 in 86ed214

// This can be the case with void and named-return functions.

if !isInit && !isDeploy && !lastStmtIsReturn(decl.Body) {

c.processDefers()

c.saveSequencePoint(decl.Body)

emit.Opcodes(c.prog.BinWriter, opcode.RET)

Then it's not clear what are the correct bounds for this "RET" opcode, because there's actually no return statement in the contract code itself.

Ideally, if we keep everything as is on the compiler level, then on the neotest level we need some way to distinguish some "real" return statements that are presented in the code in the end of every method from "RET" statements that belong to void and named-return functions. Currently we can't distinguish them based on the given debug info format, hence this PR removes all RET statements that belong to the end of the function.

And I probably also need to check that no other statements except "RET" may be excluded by this code.

And I probably also need to check that no other statements except "RET" may be excluded by this code.

OK, it’s not required, because our compiler always emit RET in the end of functions, and there's no function that ends with another instruction.

AnnaShaleva · 2024-10-16T15:47:04Z

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

codecov · 2024-10-16T15:53:00Z

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 ⚠️

Additional details and impacted files

@@            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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Slava0135 · 2024-10-16T15:58:26Z

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

Slava0135 · 2024-10-16T16:00:04Z

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

AnnaShaleva · 2024-10-16T16:00:39Z

If overlaps can only ever happen with these RET ops

No, there are multiple reasons for range overlapping, another one is described in #3559 (comment):

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

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

roman-khimov · 2024-10-17T10:04:43Z

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 · 2024-10-17T16:15:34Z

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 contract: sharpContractTemplate.cs.txt
Manifest: sharpContractTemplate.manifest.json
NEF: sharpContractTemplate.nef.txt
Debug info for this contract: sharpContractTemplate.debug.json
Parsed NEF: ops.txt

The results are:

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.

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:

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 }.

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 · 2024-10-17T16:16:23Z

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

AnnaShaleva · 2024-10-17T16:33:25Z

Closed in favour of #3622.

AnnaShaleva requested a review from roman-khimov as a code owner October 16, 2024 15:44

AnnaShaleva added the neotest Neotest framework and coverage tool label Oct 16, 2024

AnnaShaleva added this to the v0.106.4 milestone Oct 16, 2024

AnnaShaleva commented Oct 16, 2024

View reviewed changes

AnnaShaleva closed this Oct 17, 2024

AnnaShaleva mentioned this pull request Oct 17, 2024

Sequence points for some of RET statements are improperly generated #3622

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

neotest: exclude RET statements from coverage #3617

neotest: exclude RET statements from coverage #3617

AnnaShaleva commented Oct 16, 2024

AnnaShaleva Oct 16, 2024

AnnaShaleva Oct 16, 2024

AnnaShaleva Oct 16, 2024 •

edited

Loading

AnnaShaleva Oct 16, 2024 •

edited

Loading

AnnaShaleva Oct 16, 2024

AnnaShaleva Oct 16, 2024 •

edited

Loading

AnnaShaleva commented Oct 16, 2024

codecov bot commented Oct 16, 2024 •

edited

Loading

Slava0135 commented Oct 16, 2024

Slava0135 commented Oct 16, 2024

AnnaShaleva commented Oct 16, 2024

roman-khimov commented Oct 17, 2024

AnnaShaleva commented Oct 17, 2024 •

edited

Loading

AnnaShaleva commented Oct 17, 2024 •

edited

Loading

AnnaShaleva commented Oct 17, 2024

	c.saveSequencePoint(decl.Body)
	emit.Opcodes(c.prog.BinWriter, opcode.RET)

	// This can be the case with void and named-return functions.
	if !isInit && !isDeploy && !lastStmtIsReturn(decl.Body) {
	c.processDefers()
	c.saveSequencePoint(decl.Body)
	emit.Opcodes(c.prog.BinWriter, opcode.RET)

neotest: exclude RET statements from coverage #3617

neotest: exclude RET statements from coverage #3617

Conversation

AnnaShaleva commented Oct 16, 2024

AnnaShaleva Oct 16, 2024

Choose a reason for hiding this comment

AnnaShaleva Oct 16, 2024

Choose a reason for hiding this comment

AnnaShaleva Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

AnnaShaleva Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

AnnaShaleva Oct 16, 2024

Choose a reason for hiding this comment

AnnaShaleva Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

AnnaShaleva commented Oct 16, 2024

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

Slava0135 commented Oct 16, 2024

Slava0135 commented Oct 16, 2024

AnnaShaleva commented Oct 16, 2024

roman-khimov commented Oct 17, 2024

AnnaShaleva commented Oct 17, 2024 • edited Loading

AnnaShaleva commented Oct 17, 2024 • edited Loading

AnnaShaleva commented Oct 17, 2024

AnnaShaleva Oct 16, 2024 •

edited

Loading

AnnaShaleva Oct 16, 2024 •

edited

Loading

AnnaShaleva Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

AnnaShaleva commented Oct 17, 2024 •

edited

Loading

AnnaShaleva commented Oct 17, 2024 •

edited

Loading