Skip to content

C#: Add flow summary for 'Task.ConfigureAwait()'#4543

Merged
tamasvajk merged 3 commits intogithub:mainfrom
tamasvajk:feature/configureawait
Oct 28, 2020
Merged

C#: Add flow summary for 'Task.ConfigureAwait()'#4543
tamasvajk merged 3 commits intogithub:mainfrom
tamasvajk:feature/configureawait

Conversation

@tamasvajk
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the C# label Oct 23, 2020
Copy link
Contributor Author

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

Let's suppose the AccessPath::cons is working. How would I verify that the added flow summaries are working for the following two cases?

var i = new Task<int>(() => 42).ConfigureAwait(false).GetAwaiter().GetResult();
var j = await new Task<int>(() => 42).ConfigureAwait(false);

This question is actually not specific to .ConfigureAwait(). Which checks rely on flow summaries on .GetAwaiter().GetResult()?

Comment on lines +1852 to +1864
m = this.getConfigureAwaitMethod() and
source = TCallableFlowSourceQualifier() and
sourceAp = AccessPath::empty() and
sink = TCallableFlowSinkReturn() and
sinkAp =
AccessPath::cons(any(FieldContent fc |
fc.getField() =
any(SystemRuntimeCompilerServicesConfiguredTaskAwaitableTStruct t)
.getUnderlyingAwaiterField()
),
AccessPath::field(any(SystemRuntimeCompilerServicesConfiguredTaskAwaitableTConfiguredTaskAwaiterStruct s
).getUnderlyingTaskField()))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hvitved This is missing from the test expected file.
When I try to debug this on a codeql database create DB with the following:

private predicate test(
  SystemRuntimeCompilerServicesConfiguredTaskAwaitableTConfiguredTaskAwaiterStruct a, Field f
) {
  a.getAField() = f
}

I only get two fields: _dummy and _dummyPrimitive, and therefore I get no fields in .getUnderlyingTaskField(). It looks to me that Roslyn reports these field names.

  • What is the difference between the test and a codeql database create extraction?
  • How do I extract the correct field names with codeql database create?
  • Why doesn't this AccessPath::cons work?

@hvitved hvitved dismissed a stale review via 145a01c October 26, 2020 19:53
@tamasvajk tamasvajk force-pushed the feature/configureawait branch from 145a01c to 03a3676 Compare October 27, 2020 09:24
@tamasvajk tamasvajk marked this pull request as ready for review October 27, 2020 09:24
@tamasvajk tamasvajk requested a review from a team as a code owner October 27, 2020 09:24
@tamasvajk
Copy link
Contributor Author

@hvitved The only change I did since your fix is that I've squashed the first two commits.

}
}

/** An unbound generic struct in the `System.Runtime.CompilerServices` namespace. */
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is not correct. I think this class is so special that there is no need to have it, instead inline the logic where it is used below.

@tamasvajk tamasvajk merged commit 7c3964a into github:main Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants