C#: CSV-based flow summaries #6003

hvitved · 2021-06-03T12:03:23Z

~~https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/1138/~~
https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/1145/

tamasvajk

Looks good to me.

I'll add the Coverage.ql, and then modify the CSV coverage report workflow to handle C# too.

csharp/ql/test/library-tests/dataflow/external-models/steps.ql

csharp/ql/src/semmle/code/csharp/Type.qll

tamasvajk · 2021-06-17T08:48:33Z

csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ql

+        "My.Qltest;D;false;StepPropertySetter;(System.Object);;Argument[0];Property[My.Qltest.D.Property] of Argument[-1];value",
+        "My.Qltest;D;false;StepElementGetter;();;Element of Argument[-1];ReturnValue;value",
+        "My.Qltest;D;false;StepElementSetter;(System.Object);;Argument[0];Element of Argument[-1];value",
+        "My.Qltest;D;false;Apply;(System.Func<My.Qltest.D.Apply.<0>,My.Qltest.D.Apply.<1>>,My.Qltest.D.Apply.<1>);;Argument[1];Parameter[0] of Argument[0];value",


This would look nicer with S and T instead of My.Qltest.D.Apply.<0>.

csharp/ql/src/semmle/code/csharp/Generics.qll

tamasvajk · 2021-06-17T09:07:02Z

csharp/ql/test/library-tests/unification/Unification.expected

+| Unification.cs:9:10:9:11 | T3 | Unification.cs:38:11:38:22 | Nested<System.Int32>+NestedA<String> |
+| Unification.cs:9:10:9:11 | T3 | Unification.cs:38:11:38:22 | Nested<System.Int32>+NestedA<T11> |
+| Unification.cs:9:10:9:11 | T3 | Unification.cs:38:11:38:22 | Nested<System.String>+NestedA<Int32> |
+| Unification.cs:9:10:9:11 | T3 | Unification.cs:38:11:38:22 | Nested<System.String>+NestedA<T11> |
+| Unification.cs:9:10:9:11 | T3 | Unification.cs:39:11:39:17 | Nested<>+NestedB |
+| Unification.cs:9:10:9:11 | T3 | Unification.cs:39:11:39:17 | Nested<System.Int32>+NestedB |
+| Unification.cs:9:10:9:11 | T3 | Unification.cs:39:11:39:17 | Nested<System.String>+NestedB |
+| Unification.cs:9:10:9:11 | T3 | Unification.cs:41:22:41:33 | Nested<>+NestedB+NestedC<T12> |
+| Unification.cs:9:10:9:11 | T3 | Unification.cs:41:22:41:33 | Nested<System.Int32>+NestedB+NestedC<Boolean> |
+| Unification.cs:9:10:9:11 | T3 | Unification.cs:41:22:41:33 | Nested<System.Int32>+NestedB+NestedC<T12> |
+| Unification.cs:9:10:9:11 | T3 | Unification.cs:41:22:41:33 | Nested<System.String>+NestedB+NestedC<Decimal> |


Not all type names show up here with fully qualified names. It looks like nested constructed generics are not handled properly.

tamasvajk · 2021-06-17T09:09:32Z

csharp/ql/test/library-tests/overrides/Overrides22.expected

-| overrides.H<>.M<S>(TA, S) | overrides.I2<TA>.M<S>(TA, S) | implements |
-| overrides.Outer<>.A10.M<T>(Inner) | overrides.Outer<>.I6.M<T>(Inner) | implements |
+| overrides.G.M<S>(string, S) | overrides.I2<System.String>.M<S>(string, S) | implements |
+| overrides.H<>.M<S>(TA, S) | overrides.I2<overrides.H<>.<0>>.M<S>(TA, S) | implements |


I'm surprised to see TA being changed to overrides.H<>.<0>, but S not being changed. Also, shouldn't all instances of TA be replaced by overrides.H<>.<0>?

tamasvajk · 2021-06-17T09:57:26Z

Wouldn't it make sense to store the fully qualified names in the DB (if there's an easy built in way of getting it from Roslyn)?

I did a quick search, and couldn't find any APIs for getting it, but SymbolDisplayFormat seems promising, and at the same time it already shows that even Roslyn uses <> notation when displaying "qualified names" of constructed generics types. So the current QL implementation is quite close to theirs, the only two differences seem to be the type parameters and unbound generics ( `4 instead of <,,,>). Here's an instance of SymbolDisplayFormat that we could use if we decided to move this logic to the extractor.

Here are some examples of type names in Roslyn tests:

tamasvajk · 2021-06-17T13:48:55Z

Wouldn't it make sense to store the fully qualified names in the DB (if there's an easy built in way of getting it from Roslyn)?

I did a quick search, and couldn't find any APIs for getting it, but SymbolDisplayFormat ...

We discussed this in person: Our conclusion was to not use the fully qualified name returned at runtime, and also not match exactly how Roslyn represent qualified names. We're actually quite close to the one implemented by Roslyn, here are some samples of the representation used by us:

nested types: NS1.C1+C2
constructed generics: NS1.C1<System.Int32,System.Int32>
unbound generics: NS1.C1<,>
open generics: NS1.C1<T1,T2>.

tamasvajk

LGTM

github-actions bot added C# Java labels Jun 3, 2021

hvitved force-pushed the csharp/external-summaries branch 2 times, most recently from f4f9d96 to af8adee Compare June 4, 2021 14:07

hvitved marked this pull request as ready for review June 4, 2021 14:26

hvitved requested a review from a team as a code owner June 4, 2021 14:26

hvitved assigned tamasvajk Jun 4, 2021

hvitved added the no-change-note-required This PR does not need a change note label Jun 4, 2021

tamasvajk previously approved these changes Jun 7, 2021

View reviewed changes

tamasvajk reviewed Jun 7, 2021

View reviewed changes

csharp/ql/test/library-tests/dataflow/external-models/steps.ql Outdated Show resolved Hide resolved

hvitved added 2 commits June 16, 2021 14:09

C#: CSV-based flow summaries

def3d6b

C#: Always use fully qualified names in CSV data-flow summaries

8866e6c

hvitved marked this pull request as draft June 16, 2021 12:13

hvitved dismissed tamasvajk’s stale review via 992acaa June 16, 2021 12:17

hvitved force-pushed the csharp/external-summaries branch from af8adee to 992acaa Compare June 16, 2021 12:17

github-actions bot added the documentation label Jun 16, 2021

hvitved force-pushed the csharp/external-summaries branch from 992acaa to 4b5612a Compare June 16, 2021 13:39

hvitved added 2 commits June 16, 2021 19:36

C#: Changes to Type::{getQualifier,hasQualifiedName}

0af44a7

C#: Add tests for complex CSV flow summaries

3f6beaf

hvitved force-pushed the csharp/external-summaries branch from 4b5612a to 3f6beaf Compare June 16, 2021 17:36

tamasvajk reviewed Jun 17, 2021

View reviewed changes

C#: Adjust getQualifiedName for type parameters

eca11f1

hvitved added 2 commits June 17, 2021 20:47

C#: Cache NamedElement::hasQualifiedName/2

d5163ca

C#: Remove bad magic

66e4940

hvitved marked this pull request as ready for review June 20, 2021 15:46

tamasvajk approved these changes Jun 22, 2021

View reviewed changes

hvitved merged commit 38a38fd into github:main Jun 22, 2021

hvitved deleted the csharp/external-summaries branch June 22, 2021 10:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

C#: CSV-based flow summaries #6003

C#: CSV-based flow summaries #6003

Uh oh!

hvitved commented Jun 3, 2021 •

edited

Loading

Uh oh!

tamasvajk left a comment

Uh oh!

Uh oh!

Uh oh!

tamasvajk Jun 17, 2021

Uh oh!

Uh oh!

tamasvajk Jun 17, 2021

Uh oh!

tamasvajk Jun 17, 2021

Uh oh!

tamasvajk commented Jun 17, 2021 •

edited

Loading

Uh oh!

tamasvajk commented Jun 17, 2021

Uh oh!

tamasvajk left a comment

Uh oh!

Uh oh!

C#: CSV-based flow summaries #6003

C#: CSV-based flow summaries #6003

Uh oh!

Conversation

hvitved commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tamasvajk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tamasvajk Jun 17, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tamasvajk Jun 17, 2021

Choose a reason for hiding this comment

Uh oh!

tamasvajk Jun 17, 2021

Choose a reason for hiding this comment

Uh oh!

tamasvajk commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tamasvajk commented Jun 17, 2021

Uh oh!

tamasvajk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hvitved commented Jun 3, 2021 •

edited

Loading

tamasvajk commented Jun 17, 2021 •

edited

Loading