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

Correct coverage for await using (issue #914) #1111

Merged
merged 4 commits into from
Mar 8, 2021

Conversation

alexthornton1
Copy link
Contributor

This commit corrects coverage for the await using statement added
in C# 8, including the variant without an explicit scope (i.e.,
without curly braces and a body), by eliminating two kinds of
patterns that arise in the async state machine that weren't
already being detected and eliminated.

This commit corrects coverage for the `await using` statement added
in C# 8, including the variant without an explicit scope (i.e.,
without curly braces and a body), by eliminating two kinds of
patterns that arise in the async state machine that weren't
already being detected and eliminated.
@alexthornton1
Copy link
Contributor Author

alexthornton1 commented Mar 7, 2021

There are a couple of interesting things going on in this PR that I thought I should mention:

  1. I found myself wanting to ask a question like "Does this instruction load from argument 0?" or "Does this instruction store to the same local variable that the previous instruction loaded from?", but ran into some trouble with Cecil -- which I'm not very familiar with yet -- sometimes deciding the instruction would be something like ldc.i4.1 and sometimes deciding it would be ldc.i4 1 instead, inconsistent with what I'd see in disassembly. So I added a new class in coverlet.core called InstructionExtensions that smooths out those differences. If that's working around my misunderstanding of Cecil, I'd be glad to be set straight on that.
  2. I found that some of the same patterns I was looking for in async foreach occurred again in async using, so there's a refactoring opportunity available there, but I wonder if the refactoring opportunity is actually a larger one, because I see similar patterns being searched for elsewhere. For example, I needed to look for a pattern in SkipGeneratedBranchesForAwaitUsing.CheckForCleanup that seemed similar to SkipGeneratedBranchesForExceptionHandlers. But the existing code wouldn't match those patterns -- and that might partly be caused by the issue I found in my own code that seemed to require InstructionExtensions, where the code was searching for similar (but not the same) instructions and, thus, not matching.

@alexthornton1
Copy link
Contributor Author

Before:
Before

After:
After

Added a missing comment describing one of the patterns we're
searching for.
@alexthornton1
Copy link
Contributor Author

Okay, that's my last tweak until it's reviewed. :)

Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if we can avoid new helpers.
A part from that LGTM

src/coverlet.core/Extensions/InstructionExtensions.cs Outdated Show resolved Hide resolved
src/coverlet.core/Symbols/CecilSymbolHelper.cs Outdated Show resolved Hide resolved
@MarcoRossignoli MarcoRossignoli added the tenet-coverage Issue related to possible incorrect coverage label Mar 7, 2021
Two adjustments for things that arose during code review:

(1) Now using static local functions instead of non-static ones.

(2) Removed InstructionHelpers class and reverted to straightforward
    ways of checking for instruction types.
@alexthornton1
Copy link
Contributor Author

After commit 56d124d:
After_56d124d

Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MarcoRossignoli MarcoRossignoli merged commit 29e4fd4 into coverlet-coverage:master Mar 8, 2021
pbmiguel pushed a commit to pbmiguel/coverlet that referenced this pull request Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tenet-coverage Issue related to possible incorrect coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants