C#: Roslyn-based stub generation #14095

hvitved · 2023-08-30T09:02:05Z

This PR introduced a Roslyn-based stub generator, based on the existing (but much much slower) QL based stub generator. The make_stubs_nuget.py script is updated to use the new generator, and this PR updates some of the existing stubs with newly generated stubs.

Commit-by-commit review is suggested.

csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubVisitor.cs

+        if (symbol.IsAbstract)
+        {
+            if (
+                // exclude interface declarations
+                (symbol is not INamedTypeSymbol type || type.TypeKind != TypeKind.Interface) &&
+                // exclude non-static interface members
+                (symbol.ContainingType is not INamedTypeSymbol containingType || containingType.TypeKind != TypeKind.Interface || symbol.IsStatic))
+            {
+                stubWriter.Write("abstract ");
+            }
+        }


csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubVisitor.cs

+        if (symbol.IsSealed)
+        {
+            if (!(symbol is INamedTypeSymbol type && (type.TypeKind == TypeKind.Enum || type.TypeKind == TypeKind.Delegate || type.TypeKind == TypeKind.Struct)))
+                stubWriter.Write("sealed ");
+        }


csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubGenerator.cs

csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubVisitor.cs

+
+    private void StubModifiers(ISymbol symbol, bool skipAccessibility = false)
+    {
+        if (symbol.ContainingType is ITypeSymbol containing && containing.TypeKind == TypeKind.Interface)


csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubVisitor.cs

+            type is INamedTypeSymbol named && (named.IsTupleType || named.TypeArguments.Any(ContainsTupleType)) ||
+            type is IArrayTypeSymbol array && ContainsTupleType(array.ElementType) ||
+            type is IPointerTypeSymbol pointer && ContainsTupleType(pointer.PointedAtType);


csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubVisitor.cs

+           SymbolEqualityComparer.Default.Equals(t1, t2) ||
+            (
+                t1 is INamedTypeSymbol named1 &&
+                t2 is INamedTypeSymbol named2 &&
+                EqualsModuloTupleElementNames(named1.ConstructedFrom, named2.ConstructedFrom) &&
+                named1.TypeArguments.Length == named2.TypeArguments.Length &&
+                named1.TypeArguments.Zip(named2.TypeArguments).All(p => EqualsModuloTupleElementNames(p.First, p.Second))
+            ) ||
+            (
+                t1 is IArrayTypeSymbol array1 &&
+                t2 is IArrayTypeSymbol array2 &&
+                EqualsModuloTupleElementNames(array1.ElementType, array2.ElementType)
+            ) ||
+            (
+                t1 is IPointerTypeSymbol pointer1 &&
+                t2 is IPointerTypeSymbol pointer2 &&
+                EqualsModuloTupleElementNames(pointer1.PointedAtType, pointer2.PointedAtType)
+            );


tamasvajk

Looks good to me. I've added some comments/questions.

One general question: we're not explicitly ordering the printed members. Do we know if the roslyn provided order is stable? If we used alphabetical ordering, then we would probably match what the previous stub generator did, and then verifying the correctness of the newly generated stubs would be easy.

csharp/extractor/Semmle.Extraction.CSharp.DependencyStubGenerator/Program.cs

csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubGenerator.cs

csharp/extractor/Semmle.Util/MemoizedFunc.cs

csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubVisitor.cs

…il` project

michaelnebel

Approximately half way through. Will continue later today.
This looks really really cool @hvitved . Really great (and impressive) work!

Besides the inlined questions and comments:

Maybe run DCA since we change existing production code.
Is it possible to make some unit tests somehow of the StubVisitor?

csharp/extractor/Semmle.Extraction.CSharp/Entities/UserOperator.cs

csharp/extractor/Semmle.Util/MemoizedFunc.cs

csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubGenerator.cs

csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubVisitor.cs

hvitved · 2023-09-21T10:59:03Z

Looks good to me. I've added some comments/questions.

One general question: we're not explicitly ordering the printed members. Do we know if the roslyn provided order is stable? If we used alphabetical ordering, then we would probably match what the previous stub generator did, and then verifying the correctness of the newly generated stubs would be easy.

I have addressed your comments, and the members are now sorted alphabetically. I simply updated the original stub regeneration commits with the newly generated stubs, instead of adding extra commits.

hvitved · 2023-09-21T11:34:56Z

Approximately half way through. Will continue later today.
This looks really really cool @hvitved . Really great (and impressive) work!

Thanks :-)

I have (force) pushed changes to address your comments as well.

Besides the inlined questions and comments:

Maybe run DCA since we change existing production code.
Will do
Is it possible to make some unit tests somehow of the StubVisitor?
Possible, yes, but I think I would rather add an integration test that runs a QL test using regenerated stubs.

tamasvajk

I have addressed your comments, and the members are now sorted alphabetically. I simply updated the original stub regeneration commits with the newly generated stubs, instead of adding extra commits.

Thanks. It looks like the original stub generator did a bit more than ordering alphabetically. ( I think it might have generated the full signatures in memory, and then order them before writing.) I think it would be difficult to repro this based on Roslyn, which is a shame, it would have been great to compare the original and new stubs. (BTW, we might not have a stable order for overloads either.)

I've added some comments where I found differences between the old and new stubs.

...ubs/_frameworks/Microsoft.AspNetCore.App/Microsoft.AspNetCore.Authentication.Abstractions.cs

...urces/stubs/_frameworks/Microsoft.AspNetCore.App/Microsoft.AspNetCore.Authentication.Core.cs

.../stubs/_frameworks/Microsoft.AspNetCore.App/Microsoft.AspNetCore.Components.Authorization.cs

...esources/stubs/_frameworks/Microsoft.AspNetCore.App/Microsoft.AspNetCore.Components.Forms.cs

csharp/ql/test/resources/stubs/Antlr3.Runtime/3.5.1/Antlr3.Runtime.cs

csharp/ql/test/resources/stubs/_frameworks/Microsoft.NETCore.App/System.AppContext.cs

.../ql/test/resources/stubs/_frameworks/Microsoft.NETCore.App/System.Runtime.InteropServices.cs

hvitved · 2023-09-21T11:53:02Z

Thanks. It looks like the original stub generator did a bit more than ordering alphabetically. ( I think it might have generated the full signatures in memory, and then order them before writing.) I think it would be difficult to repro this based on Roslyn, which is a shame, it would have been great to compare the original and new stubs. (BTW, we might not have a stable order for overloads either.)

Indeed, that is what the QL based stub generator does, and we very much don't want to first compute things in memory and then write to disk later.

csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubVisitor.cs

csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubGenerator.cs

tamasvajk

LGTM.
How much faster is this solution than the previous QL based one?

michaelnebel · 2023-09-22T08:23:10Z

csharp/ql/test/resources/stubs/Antlr3.Runtime/3.5.1/Antlr3.Runtime.cs

-            public ANTLRStringStream(System.Char[] data, int numberOfActualCharsInArray) => throw null;
-            protected ANTLRStringStream() => throw null;
-            public virtual int CharPositionInLine { get => throw null; set => throw null; }
+            public virtual int CharPositionInLine { get => throw null; set { } }


set doesn't throw null?

No reason to, it is just a matter of making the stubs as simple as possible in terms of generated code.

michaelnebel · 2023-09-22T08:34:08Z

LGTM. How much faster is this solution than the previous QL based one?

Yeah, I am also really curious!

hvitved · 2023-09-22T08:35:42Z

How much faster is this solution than the previous QL based one?

It takes just a few seconds to generate stubs for the full standard library, so definitely much faster, but I don't have actual numbers from before.

michaelnebel

Great work Tom!
LGTM 👍

Non-blocking question:
Are you planning on making it unit testable or introduce some integration like tests for? We do have tests for the queries used by the old stub generator.

hvitved · 2023-09-22T09:01:57Z

Generating stubs for Swashbuckle.AspNetCore.Swagger (which includes NetCore and AspNetCore) takes 50 seconds for the QL based stub generator, while it takes 5 seconds for the new stub generator.

hvitved · 2023-09-22T09:02:38Z

Are you planning on making it unit testable or introduce some integration like tests for? We do have tests for the queries used by the old stub generator.

Unit tests, no. But I was thinking about adding an integration test.

github-actions bot added C# Ruby labels Aug 30, 2023

github-advanced-security bot found potential problems Aug 30, 2023

View reviewed changes

hvitved force-pushed the csharp/stub-generator branch 2 times, most recently from db4d1c3 to 73075e3 Compare August 31, 2023 08:56

github-advanced-security bot found potential problems Aug 31, 2023

View reviewed changes

hvitved force-pushed the csharp/stub-generator branch from 73075e3 to aff8207 Compare September 5, 2023 11:40

github-actions bot removed the Ruby label Sep 5, 2023

hvitved force-pushed the csharp/stub-generator branch 2 times, most recently from e806ae9 to f8ace6a Compare September 6, 2023 07:32

hvitved mentioned this pull request Sep 6, 2023

C#: Clear TRAP stack when calling PopulateGenerics #14149

Merged

hvitved force-pushed the csharp/stub-generator branch from f8ace6a to c06b515 Compare September 6, 2023 12:21

github-actions bot added the QL-for-QL label Sep 6, 2023

hvitved force-pushed the csharp/stub-generator branch from c06b515 to 2beb829 Compare September 6, 2023 12:23

github-actions bot removed the QL-for-QL label Sep 6, 2023

hvitved force-pushed the csharp/stub-generator branch 2 times, most recently from 6db4dc5 to 591f055 Compare September 8, 2023 08:50

hvitved force-pushed the csharp/stub-generator branch 3 times, most recently from 3381f33 to b3ac833 Compare September 20, 2023 09:26

hvitved added the no-change-note-required This PR does not need a change note label Sep 20, 2023

hvitved marked this pull request as ready for review September 20, 2023 10:16

hvitved requested a review from a team as a code owner September 20, 2023 10:16

tamasvajk reviewed Sep 21, 2023

View reviewed changes

michaelnebel self-requested a review September 21, 2023 07:46

hvitved added 4 commits September 21, 2023 11:33

C#: Move NestPaths to Semmle.Util

2429a53

C#: Move some methods into newly created `Semmle.Extraction.CSharp.Ut…

6021d00

…il` project

C#: Roslyn based stub generation

e021fb4

C#: Use new stub generator in make_stubs_nuget.py

8b2c233

michaelnebel reviewed Sep 21, 2023

View reviewed changes

hvitved added 2 commits September 21, 2023 12:56

C#: Regenerate Microsoft.AspNetCore.App stubs

e944b90

C#: Regenerate NHibernate stubs

2343e5e

hvitved force-pushed the csharp/stub-generator branch from b3ac833 to e14898c Compare September 21, 2023 10:57

Address review comments

04c4e73

hvitved force-pushed the csharp/stub-generator branch from e14898c to 04c4e73 Compare September 21, 2023 11:33

tamasvajk reviewed Sep 21, 2023

View reviewed changes

michaelnebel reviewed Sep 21, 2023

View reviewed changes

csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubVisitor.cs Outdated Show resolved Hide resolved

michaelnebel reviewed Sep 21, 2023

View reviewed changes

csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubVisitor.cs Show resolved Hide resolved

michaelnebel reviewed Sep 21, 2023

View reviewed changes

csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubVisitor.cs Outdated Show resolved Hide resolved

tamasvajk reviewed Sep 21, 2023

View reviewed changes

csharp/extractor/Semmle.Extraction.CSharp.StubGenerator/StubVisitor.cs Outdated Show resolved Hide resolved

hvitved force-pushed the csharp/stub-generator branch from 6b11ee0 to 208616e Compare September 21, 2023 13:14

hvitved added 2 commits September 21, 2023 15:35

Address more review comments

4805e2a

Regenerate stubs

f07d02b

hvitved force-pushed the csharp/stub-generator branch from 208616e to f07d02b Compare September 21, 2023 13:36

tamasvajk reviewed Sep 21, 2023

View reviewed changes

hvitved force-pushed the csharp/stub-generator branch from d8a806a to 0108aef Compare September 22, 2023 07:13

C#: Refactor and regenerate stubs

831baa8

hvitved force-pushed the csharp/stub-generator branch from 0108aef to 831baa8 Compare September 22, 2023 07:15

tamasvajk approved these changes Sep 22, 2023

View reviewed changes

michaelnebel reviewed Sep 22, 2023

View reviewed changes

michaelnebel approved these changes Sep 22, 2023

View reviewed changes

hvitved merged commit 9638a6c into github:main Sep 22, 2023
10 checks passed

hvitved deleted the csharp/stub-generator branch September 22, 2023 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C#: Roslyn-based stub generation #14095

C#: Roslyn-based stub generation #14095

hvitved commented Aug 30, 2023 •

edited

Loading

tamasvajk left a comment

michaelnebel left a comment

hvitved commented Sep 21, 2023

hvitved commented Sep 21, 2023 •

edited

Loading

tamasvajk left a comment

hvitved commented Sep 21, 2023

tamasvajk left a comment

michaelnebel Sep 22, 2023

hvitved Sep 22, 2023

michaelnebel commented Sep 22, 2023

hvitved commented Sep 22, 2023

michaelnebel left a comment

hvitved commented Sep 22, 2023

hvitved commented Sep 22, 2023

C#: Roslyn-based stub generation #14095

C#: Roslyn-based stub generation #14095

Conversation

hvitved commented Aug 30, 2023 • edited Loading

tamasvajk left a comment

Choose a reason for hiding this comment

michaelnebel left a comment

Choose a reason for hiding this comment

hvitved commented Sep 21, 2023

hvitved commented Sep 21, 2023 • edited Loading

tamasvajk left a comment

Choose a reason for hiding this comment

hvitved commented Sep 21, 2023

tamasvajk left a comment

Choose a reason for hiding this comment

michaelnebel Sep 22, 2023

Choose a reason for hiding this comment

hvitved Sep 22, 2023

Choose a reason for hiding this comment

michaelnebel commented Sep 22, 2023

hvitved commented Sep 22, 2023

michaelnebel left a comment

Choose a reason for hiding this comment

hvitved commented Sep 22, 2023

hvitved commented Sep 22, 2023

hvitved commented Aug 30, 2023 •

edited

Loading

hvitved commented Sep 21, 2023 •

edited

Loading