C# 12: Primary constructor inititalizers. #15625

michaelnebel · 2024-02-15T12:57:26Z

It turns out that the extraction of primary constructors didn't extract initialisers correctly. This is fixed in this PR.

That is, for

public class C1(object o1) { }
public class C2(object o2) : C1(o2) { }

We need to extract

The implicit inititaliser (calling the object constructor) for the primary constructor on C1.
The explicit initialiser (calling the C1 constructor) for the primary constructor on C2.

This is needed for

Getting data-flow to work properly for simple classes with primary constructors (otherwise we won't get a control flow node corresponding to the basic block containing the class declaration). Adding the implicit initialiser means that we at least have a first statement, which is calling the object constructor.
First step for getting data flow to work for classes with primary constructors that uses inheritance (the explicit initialiser needs to be extracted otherwise we can't detect flow from the primary constructor of C2 into the C1 base class constructor).

It also turns out that we also need to extract at least an empty (synthetic) body for primary constructors to avoid lots of control flow issues.

…ailing dataflow test.

…ise we get missing post update nodes to to reverseRead in the data flow consistency queries.

michaelnebel · 2024-02-19T14:32:20Z

Sadly DCA doesn't show any new results.

tamasvajk

I've added some minor comments/question.

tamasvajk · 2024-02-20T08:25:26Z

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

@@ -10,8 +12,16 @@ namespace Semmle.Extraction.CSharp.Entities
 {
    internal class Constructor : Method
    {
+        private readonly Lazy<List<SyntaxNode>> DeclaringReferenceSyntax;


What do we gain with Lazy<>? I think we're using these nodes in the ReportingLocation. Doesn't this mean that we need the DeclaringReferenceSyntax for all constructors?

Minor: fields should start with lower case letters.

Good point, I will just initialize it directly in the constructor.

tamasvajk · 2024-02-20T08:27:41Z

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

+            if (IsPrimary)
+            {
+                // Create a synthetic empty body for primary constructors.
+                Statements.SyntheticEmptyBlock.Create(Context, this, 0, Location);


As discussed on Slack, this might also be useful for default constructors.

Yes, that is a good point!
I would prefer to make furthermore improvements on follow up PRs. Is that ok?

Yep, a separate PR sounds good.

tamasvajk · 2024-02-20T08:30:14Z

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

+            if (Block is null && ExpressionBody is null && !IsPrimary ||
+                !IsSourceDeclaration)
+            {
                return;
+            }


As discussed on Slack, this might also apply to default constructors too.

class Base {} class Derived : Base {}

Derived has a default constructor, which implicitly is calling base().

same comment as above (and thank you for putting it here!)

tamasvajk · 2024-02-20T08:46:28Z

csharp/ql/test/library-tests/constructors/PrintAst.expected

@@ -25,6 +25,7 @@ constructors.cs:
 #   23|           -1: [TypeMention] object
 #   23|         1: [Parameter] s
 #   23|           -1: [TypeMention] string
+#   23|       4: [BlockStmt] {...}


For generated expressions, we're using expr_compiler_generated. Should we mark SyntheticEmptyBlocks as compiler generated too?

We can add it to the compiler_generated table?

Hmm, that will require database changes.

We could create stmt_compiler_generated. Should we do that?

I think it would be fine to do this in a separate PR, so that you could merge this one.

Is there any benefit of having expr_compiler_generated, compiler_generated, and stmt_compiler_generated? It looks like any solution would need a DB schema change. I think my preference would be merging all of this into compiler_generated.

That would be a nice simplification in any case. I will add that to my list of follow up items!

tamasvajk

LGTM

csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll

michaelnebel · 2024-02-22T08:49:03Z

I realized that I had forgotten to start the DCA experiment with flags to rebuild the extractor.
The above experiment is based on the merge commit and the parent from main.
It shows some new results, but only in quality queries, mostly in useless-type-test, but they seem unrelated to the changes (but they look sound) and they are all in generated code.

github-actions bot added the C# label Feb 15, 2024

michaelnebel force-pushed the csharp/primaryconstructorinitializer branch 8 times, most recently from b6737bb to a149bde Compare February 19, 2024 10:29

michaelnebel added the no-change-note-required This PR does not need a change note label Feb 19, 2024

michaelnebel marked this pull request as ready for review February 19, 2024 11:41

michaelnebel requested a review from a team as a code owner February 19, 2024 11:41

michaelnebel requested a review from hvitved February 19, 2024 12:01

michaelnebel added 7 commits February 19, 2024 13:04

C#: Add some test cases for primary constructor inititalizers and a f…

6b38a81

…ailing dataflow test.

C#: Invert logic in ExtractInitializer.

c613851

C#: Extract explicit and implicit primary constructor initializers.

7a80205

C#: Updated expected test output.

dcde659

C#: Synthesize an empty body for primary constructors.

28d5c11

C#: Add postupdate nodes for all instance parameter accesses - otherw…

d836871

…ise we get missing post update nodes to to reverseRead in the data flow consistency queries.

C#: Update expected test output.

feda6bc

tamasvajk reviewed Feb 20, 2024

View reviewed changes

C#: Code quality improvements.

f246272

michaelnebel force-pushed the csharp/primaryconstructorinitializer branch from a149bde to f246272 Compare February 20, 2024 10:48

michaelnebel requested a review from tamasvajk February 20, 2024 11:46

tamasvajk approved these changes Feb 20, 2024

View reviewed changes

michaelnebel merged commit ed3dba8 into github:main Feb 20, 2024

michaelnebel deleted the csharp/primaryconstructorinitializer branch February 20, 2024 14:12

hvitved reviewed Feb 20, 2024

View reviewed changes

csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll Show resolved Hide resolved

michaelnebel restored the csharp/primaryconstructorinitializer branch February 21, 2024 09:18

michaelnebel deleted the csharp/primaryconstructorinitializer branch February 21, 2024 09:19

C# 12: Primary constructor inititalizers. #15625

C# 12: Primary constructor inititalizers. #15625

Uh oh!

Conversation

michaelnebel commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelnebel commented Feb 19, 2024

Uh oh!

tamasvajk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tamasvajk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelnebel commented Feb 22, 2024

Uh oh!

Uh oh!

michaelnebel commented Feb 15, 2024 •

edited

Loading