Skip branches in generated `MoveNext()` for singleton iterators #813

bert2 · 2020-04-13T11:38:48Z

This fixes #810 by skipping the two branches inside MoveNext() that will be generated when the iterator only yields once.

For instance, the iterator...

public class SingletonIterator
{
    public IEnumerable<string> Fetch()
    {
        yield return "one";
    }
}

...will result in a generated MoveNext() method that has no switch instruction, but a brfalse.s and a beq.s instruction instead:

<Fetch>d__0.MoveNext:
IL_0000:  ldarg.0     
IL_0001:  ldfld       UserQuery+SingletonIterator+<Fetch>d__0.<>1__state
IL_0006:  stloc.0     
IL_0007:  ldloc.0     
IL_0008:  brfalse.s   IL_0012
IL_000A:  br.s        IL_000C
IL_000C:  ldloc.0     
IL_000D:  ldc.i4.1    
IL_000E:  beq.s       IL_0014
IL_0010:  br.s        IL_0016
IL_0012:  br.s        IL_0018
IL_0014:  br.s        IL_0034
IL_0016:  ldc.i4.0    
IL_0017:  ret         
IL_0018:  ldarg.0     
IL_0019:  ldc.i4.m1   
IL_001A:  stfld       UserQuery+SingletonIterator+<Fetch>d__0.<>1__state
IL_001F:  nop         
IL_0020:  ldarg.0     
IL_0021:  ldstr       "one"
IL_0026:  stfld       UserQuery+SingletonIterator+<Fetch>d__0.<>2__current
IL_002B:  ldarg.0     
IL_002C:  ldc.i4.1    
IL_002D:  stfld       UserQuery+SingletonIterator+<Fetch>d__0.<>1__state
IL_0032:  ldc.i4.1    
IL_0033:  ret         
IL_0034:  ldarg.0     
IL_0035:  ldc.i4.m1   
IL_0036:  stfld       UserQuery+SingletonIterator+<Fetch>d__0.<>1__state
IL_003B:  ldc.i4.0    
IL_003C:  ret

MarcoRossignoli · 2020-04-15T07:40:17Z

You should add also a complete real test inside folder https://github.com/tonerdo/coverlet/tree/master/test/coverlet.core.tests/Coverage:

Add a file named CoverageTest.Yield.cs
Add a file named Instrumentation.Yield.cs and add a simple class like your failing sample like https://github.com/tonerdo/coverlet/blob/master/test/coverlet.core.tests/Samples/Instrumentation.SelectionStatements.cs#L5
Inside CoverageTest.Yield.cs prepare and run real instrumentation test like https://github.com/tonerdo/coverlet/blob/master/test/coverlet.core.tests/Coverage/CoverageTests.SelectionStatements.cs#L14 read the comment as guide, copy same structure.

If you need to debug your test you have to simply change code like (CoverageTests.SelectionStatements.cs file sample)

  FunctionExecutor.RunInProcess(async (string[] pathSerialize) =>
                {
                    // Run load and call a delegate passing class as dynamic to simplify method call
                    CoveragePrepareResult coveragePrepareResult = await TestInstrumentationHelper.Run<SelectionStatements>(instance =>
                    {
                        // We call method to trigger coverage hits
                        instance.If(true);

                        // For now we have only async Run helper
                        return Task.CompletedTask;
                    }, persistPrepareResultToFile: pathSerialize[0], disableRestoreModules: true);

                    // we return 0 if we return something different assert fail
                    return 0;
                }, new string[] { path });

These real tests run in separate process so you cannot debug as is, you need to change in 2 place RunInProcess instead of Run and add disableRestoreModules: true) to avoid module restore at the end of instrumentation, this avoid error related to locked file.
After debugging you can revert removing parameters and calling correct out of proc run method.
You can assert expected number of branches with this https://github.com/tonerdo/coverlet/blob/master/test/coverlet.core.tests/Coverage/InstrumenterHelper.Assertions.cs#L87 feel free to run only in Debug configuration.

Let me know if you're in trouble.

MarcoRossignoli · 2020-04-15T07:41:08Z

cc: @matteoerigozzi can you take a look too?

bert2 · 2020-04-15T08:33:55Z

Ok, I'll look into it.

I noticed one problem though: I'm unable to run the tests as described in CONTRIBUTING.md:

PS> dotnet --version
3.1.201
PS> dotnet restore; dotnet build --no-restore; dotnet pack
[...]
PS> dotnet test --no-build /p:CollectCoverage=true /p:Include=\"[coverlet.collector]*,[coverlet.core]*,[coverlet.msbuild.tasks]*\" /p:Exclude=\"[coverlet.core.tests.samples.netstandard]*,[coverlet.tests.xunit.extensions]*\"
MSBUILD : error MSB1006: Property is not valid.
Switch: [coverlet.core]*

For switch syntax, type "MSBuild -help"

The escaping of the list doesn't seem to work on my end. Is this guideline still accurate?

MarcoRossignoli · 2020-04-15T08:51:08Z

You're inside powershell you need to escape differently https://github.com/tonerdo/coverlet/blob/master/Documentation/MSBuildIntegration.md#note-for-powershell--vsts-users

bert2 · 2020-04-15T10:42:53Z

Well that was easier than expected. Do you want me to squash those commits?

MarcoRossignoli

Well that was easier than expected. Do you want me to squash those commits?

No I'll squash on merge.

test/coverlet.core.tests/Coverage/CoverageTest.Yield.cs

MarcoRossignoli · 2020-04-15T16:52:41Z

I'm bit busy these days...update seems ok but I need to go a bit deeper to confirm, I'll come back asap!Thank's!

MarcoRossignoli

Pattern recognition seems too much generic

src/coverlet.core/Symbols/CecilSymbolHelper.cs

…essing here)

…(just guessing here)" This reverts commit 23607b7.

MarcoRossignoli · 2020-04-18T17:32:06Z

/azp run

azure-pipelines · 2020-04-18T17:32:14Z

Azure Pipelines successfully started running 1 pipeline(s).

test/coverlet.core.tests/Coverage/InstrumenterHelper.Assertions.cs

MarcoRossignoli

Some more tests, seems ok.

test/coverlet.core.tests/Samples/Instrumentation.Yield.cs

src/coverlet.core/Symbols/CecilSymbolHelper.cs

bert2 · 2020-04-19T16:19:26Z

Some more tests, seems ok.

Yup, that's right. ~~Will also add some more unit tests~~ (I couldn't come up with any unit test cases not already covered by the integration tests).

test/coverlet.core.tests/Coverage/CoverageTest.Yield.cs

MarcoRossignoli

Some nits and a question on enumerable test.

MarcoRossignoli · 2020-05-10T15:53:50Z

/azp run

azure-pipelines · 2020-05-10T15:53:58Z

Azure Pipelines successfully started running 1 pipeline(s).

MarcoRossignoli

Great work!
I refactored a bit tests to be more clear.

MarcoRossignoli · 2020-05-10T15:58:49Z

@bert2 feel free to take any other issue, if you like coverage one this is the query https://github.com/coverlet-coverage/coverlet/issues?q=is%3Aissue+is%3Aopen+label%3Atenet-coverage maybe start from untriaged.
There aren't a lot of devs that likes lose himself in the rabbit hole(IL/Roslyn) 😅 , but there are al lot of stuff to learn on how runtime/code works.

MarcoRossignoli · 2020-05-10T16:01:10Z

CI ran but there were some issue between github and AzDo likely because we're in the middle of move btw this is the link of CI https://dev.azure.com/tonerdo/coverlet/_build/results?buildId=1023&view=results

bert2 · 2020-05-11T09:56:08Z

@MarcoRossignoli it definitely was a very interesting journey :) I will probably look into some of the coverage bugs, however I'm not sure when I'll be able to spend that much time for it again.

MarcoRossignoli · 2020-05-11T10:03:06Z

@MarcoRossignoli it definitely was a very interesting journey :) I will probably look into some of the coverage bugs, however I'm not sure when I'll be able to spend that much time for it again.

No props at all it's not mandatory it's oss!Thank's again for the PR and help!

bert2 · 2020-11-14T09:33:14Z

Hey @MarcoRossignoli, I wrote a CodeLens extension for Visual Studio that shows you the instructions of a method: https://github.com/bert2/microscope

Shameless self-promotion, I know, but it I think it could be quite useful when working on software like coverlet 😊

MarcoRossignoli · 2020-11-14T11:13:26Z

Thank you I'll give it a try for sure!

bert2 added 2 commits April 13, 2020 13:31

skip branches in generated MoveNext() for singleton iterators

6e1b51a

use All() instead of Any()

e0757c8

MarcoRossignoli added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) bug Something isn't working tenet-coverage Issue related to possible incorrect coverage labels Apr 15, 2020

bert2 added 2 commits April 15, 2020 12:18

add integration tests

ed10977

fix tests and assert number of branches

6d96cc0

MarcoRossignoli requested changes Apr 15, 2020

View reviewed changes

test/coverlet.core.tests/Coverage/CoverageTest.Yield.cs Outdated Show resolved Hide resolved

test/coverlet.core.tests/Coverage/CoverageTest.Yield.cs Outdated Show resolved Hide resolved

test/coverlet.core.tests/Coverage/CoverageTest.Yield.cs Outdated Show resolved Hide resolved

remove guideline comments and dont skip tests on MacOS

9b327b6

bert2 requested a review from MarcoRossignoli April 15, 2020 11:08

MarcoRossignoli requested changes Apr 17, 2020

View reviewed changes

src/coverlet.core/Symbols/CecilSymbolHelper.cs Outdated Show resolved Hide resolved

bert2 added 4 commits April 18, 2020 11:46

add failing test case for singleton generator with switch

669089d

use Random in switch to avoid it from being optimized away (just gu…

23607b7

…essing here)

Revert "use Random in switch to avoid it from being optimized away …

03620d9

…(just guessing here)" This reverts commit 23607b7.

also skip second branch if iterator contains a switch

98605ca

MarcoRossignoli reviewed Apr 18, 2020

View reviewed changes

test/coverlet.core.tests/Coverage/InstrumenterHelper.Assertions.cs Show resolved Hide resolved

MarcoRossignoli requested changes Apr 19, 2020

View reviewed changes

test/coverlet.core.tests/Samples/Instrumentation.Yield.cs Show resolved Hide resolved

src/coverlet.core/Symbols/CecilSymbolHelper.cs Show resolved Hide resolved

bert2 added 2 commits April 22, 2020 17:35

add comment explaining skipFirstBranch and skipSecondBranch

c6e8f21

add more integration test cases

4104e80

bert2 requested a review from MarcoRossignoli April 22, 2020 15:53

MarcoRossignoli reviewed May 2, 2020

View reviewed changes

test/coverlet.core.tests/Coverage/CoverageTest.Yield.cs Show resolved Hide resolved

MarcoRossignoli reviewed May 2, 2020

View reviewed changes

test/coverlet.core.tests/Coverage/CoverageTest.Yield.cs Outdated Show resolved Hide resolved

MarcoRossignoli reviewed May 2, 2020

View reviewed changes

test/coverlet.core.tests/Coverage/CoverageTest.Yield.cs Outdated Show resolved Hide resolved

MarcoRossignoli requested changes May 2, 2020

View reviewed changes

bert2 and others added 5 commits May 3, 2020 12:13

check coverage of switch(n)

d3c069f

check coverage of other lines in Yield.Enumerable()

65ea9f3

fix assertions for release builds

93d5202

remove coverage assertions for release builds

5b1471f

refactor test a bit

e576020

MarcoRossignoli removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label May 10, 2020

nit: style

cf5d269

MarcoRossignoli approved these changes May 10, 2020

View reviewed changes

MarcoRossignoli merged commit 3264ad3 into coverlet-coverage:master May 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip branches in generated `MoveNext()` for singleton iterators #813

Skip branches in generated `MoveNext()` for singleton iterators #813

bert2 commented Apr 13, 2020

MarcoRossignoli commented Apr 15, 2020 •

edited

MarcoRossignoli commented Apr 15, 2020 •

edited

bert2 commented Apr 15, 2020

MarcoRossignoli commented Apr 15, 2020 •

edited

bert2 commented Apr 15, 2020 •

edited

MarcoRossignoli left a comment

MarcoRossignoli commented Apr 15, 2020

MarcoRossignoli left a comment •

edited

MarcoRossignoli commented Apr 18, 2020

azure-pipelines bot commented Apr 18, 2020

MarcoRossignoli left a comment •

edited

bert2 commented Apr 19, 2020 •

edited

MarcoRossignoli left a comment •

edited

MarcoRossignoli commented May 10, 2020

azure-pipelines bot commented May 10, 2020

MarcoRossignoli left a comment

MarcoRossignoli commented May 10, 2020 •

edited

MarcoRossignoli commented May 10, 2020

bert2 commented May 11, 2020

MarcoRossignoli commented May 11, 2020 •

edited

bert2 commented Nov 14, 2020

MarcoRossignoli commented Nov 14, 2020

Skip branches in generated MoveNext() for singleton iterators #813

Skip branches in generated MoveNext() for singleton iterators #813

Conversation

bert2 commented Apr 13, 2020

MarcoRossignoli commented Apr 15, 2020 • edited

MarcoRossignoli commented Apr 15, 2020 • edited

bert2 commented Apr 15, 2020

MarcoRossignoli commented Apr 15, 2020 • edited

bert2 commented Apr 15, 2020 • edited

MarcoRossignoli left a comment

Choose a reason for hiding this comment

MarcoRossignoli commented Apr 15, 2020

MarcoRossignoli left a comment • edited

Choose a reason for hiding this comment

MarcoRossignoli commented Apr 18, 2020

azure-pipelines bot commented Apr 18, 2020

MarcoRossignoli left a comment • edited

Choose a reason for hiding this comment

bert2 commented Apr 19, 2020 • edited

MarcoRossignoli left a comment • edited

Choose a reason for hiding this comment

MarcoRossignoli commented May 10, 2020

azure-pipelines bot commented May 10, 2020

MarcoRossignoli left a comment

Choose a reason for hiding this comment

MarcoRossignoli commented May 10, 2020 • edited

MarcoRossignoli commented May 10, 2020

bert2 commented May 11, 2020

MarcoRossignoli commented May 11, 2020 • edited

bert2 commented Nov 14, 2020

MarcoRossignoli commented Nov 14, 2020

Skip branches in generated `MoveNext()` for singleton iterators #813

Skip branches in generated `MoveNext()` for singleton iterators #813

MarcoRossignoli commented Apr 15, 2020 •

edited

MarcoRossignoli commented Apr 15, 2020 •

edited

MarcoRossignoli commented Apr 15, 2020 •

edited

bert2 commented Apr 15, 2020 •

edited

MarcoRossignoli left a comment •

edited

MarcoRossignoli left a comment •

edited

bert2 commented Apr 19, 2020 •

edited

MarcoRossignoli left a comment •

edited

MarcoRossignoli commented May 10, 2020 •

edited

MarcoRossignoli commented May 11, 2020 •

edited