C#: Replace initializer splitting with an ObjectInitMethod. #20922

aschackmull · 2025-11-26T14:25:44Z

Field initializers need to be executed by the constructor. In case of multiple constructors that call each other, only those constructors that call base constructors should execute field initializers in order to run the initializers exactly once for each constructed object. Currently this is done using splitting to copy all the initializers to the CFGs of all the relevant constructors. This PR changes this to happen in a single method, called "object initializer", which is then called from all the relevant constructors.

The extractor is modified to emit object initializer methods and calls to them from all relevant constructors (i.e. those that call base).
The CFG is changed to put the field initializers in the object initializer so they're only in one place. This gets rid of initializer-splitting.
A small bug is fixed: field initializers should run before the base call - not after.
The AST is kept mostly as-is. This means that field initializers are still top-level in the enclosing class, but from both a CFG and data flow perspective the new object initializer method is added as enclosing callable.

michaelnebel

Uh, very neat!
Just some quick initial comments. It appears that there are some tests failing.

michaelnebel · 2025-11-27T14:06:27Z

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

+{
+    internal sealed class ObjectInitMethod : CachedEntity, IMethodEntity
+    {
+        Type ContainingType { get; }


michaelnebel · 2025-11-27T14:06:39Z

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

+            this.ContainingType = containingType;
+        }
+
+        public string Name => "<object initializer>";


Can be made a readonly field.

michaelnebel · 2025-11-27T14:30:39Z

Do you also intend to model some synthetic assignments in the new synthetic method?

aschackmull · 2025-11-28T08:41:19Z

Do you also intend to model some synthetic assignments in the new synthetic method?

The assignments are already extracted as top-level expressions in the enclosing class. We could move them into this new method - that's what Java does, but Java also allow one to write a chunk of code that goes directly into the object initializer, so the situation there is slightly different. However, I opted not to do that since it would be a much more invasive change and require us to change any current QL code that relies on identifying field initializers as they would have moved and would look different. Though, if we were building from scratch then I would have put the assignments inside the method, but as it stands I don't think it's worth it to make the change.

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

Copilot

Pull request overview

This PR replaces the initializer splitting mechanism with a dedicated object initializer method to improve how field initializers are executed by constructors. Instead of copying field initializers to all relevant constructors' CFGs, they now reside in a single <object initializer> method that is called from constructors that invoke base constructors. This ensures initializers run exactly once per object construction.

Key changes:

Introduces <object initializer> methods in the extractor for classes with field initializers
Modifies CFG to place field initializers in the object initializer method
Fixes bug where field initializers were running after base calls instead of before

Reviewed changes

Copilot reviewed 94 out of 95 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
csharp/ql/test/library-tests/obinit/ObInit.ql	Adds query predicates to test object initializer methods and their calls
csharp/ql/test/library-tests/obinit/obinit.cs	New test file with classes demonstrating field initializer behavior across constructor chains
csharp/ql/test/library-tests/obinit/ObInit.expected	Expected test results showing object initializer method creation and CFG structure
csharp/ql/test/library-tests/obinit/Flow.ql	Adds data flow test to verify field initializer values flow correctly
csharp/ql/test/library-tests/obinit/Flow.expected	Expected data flow results showing field values propagating from source to sink
Multiple test .expected files	Updates across test suite reflecting addition of `<object initializer>` methods in AST ordering and data flow

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

… bodies.

michaelnebel

It looks like DCA identified some "new" missing call targets.

michaelnebel · 2025-12-03T09:05:54Z

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

+
+        public override void WriteId(EscapingTextWriter trapFile)
+        {
+            trapFile.WriteSubId(ContainingType);


Does this work as expected for generics (I suspect it does due to how WriteId looks like for constructors)? Just wondering whether you actively considered that case.

I believe extracted Callable bodies are 1-1 with their corresponding lines of code, ie. we extract only one Constructor body regardless of generics. And since ObjectInitMethod extraction is triggered by inserting a call in those constructor bodies we will also only extract one ObjectInitMethod. That is at least my understanding. And I think I've located the line that ensures this: In Method.cs the call to ExtractInitializers is guarded by IsSourceDeclaration.

michaelnebel · 2025-12-03T09:13:55Z

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

+            this.ContainingType = containingType;
+        }
+
+        public static readonly string Name = "<object initializer>";


hvitved

Overall LGTM, thanks for fixing this long standing tech debt.

hvitved · 2025-12-04T09:55:33Z

csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll

+   */
+  class InitializedInstanceMember extends Member {
+    InitializedInstanceMember() {
+      exists(AssignExpr ae |


I would make ae a field and then just return that in getInitializer.

hvitved · 2025-12-04T09:56:47Z

csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll

+  }
+
+  /**
+   * Gets the last member initializer expression for non-static constructor `c`


QL doc is outdated.

hvitved · 2025-12-04T10:03:21Z

csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll

      not c instanceof GotoCompletion
      or
-      last(InitializerSplitting::lastConstructorInitializer(scope, _), last, c) and
+      last(callable.(Constructor).getInitializer(), last, c) and


Shouldn't there also be a case for getObjectInitializerCall?

I think that would resolve the deadEnd inconsistency at csharp/ql/test/library-tests/standalone/errorrecovery/CONSISTENCY/CfgConsistency.expected.

Sure, I guess we can add that as a safety mechanism for broken code. I didn't add it originally since such a case can only occur when the doesn't compile. In correct C# code there will always be a callable.(Constructor).getInitializer() after a call to an ObjectInitMethod.

We actually need two additional cases to completely cover the case when the Constructor.getInitializer() is missing - both here for when there's no body, and as a step when there is. I've added both with suitable comments.

hvitved · 2025-12-04T10:12:24Z

csharp/ql/test/library-tests/obinit/Flow.ql

@@ -0,0 +1,22 @@
+import csharp
+
+module FlowConfig implements DataFlow::ConfigSig {


I think this test should use inline expectations, see e.g. FieldFlow.ql.

aschackmull · 2025-12-04T12:25:10Z

It looks like DCA identified some "new" missing call targets.

Is that in any way related to this PR or is it somehow spurious? I don't see how my changes would cause that, but I may be missing something?

michaelnebel · 2025-12-04T12:38:49Z

It looks like DCA identified some "new" missing call targets.

Is that in any way related to this PR or is it somehow spurious? I don't see how my changes would cause that, but I may be missing something?

Looked at bit closer: You are right, they are most likely spurious. The affected projects also have fluctuations in other DCA stats (for instance extractor messages). So probably fine to ignore.

michaelnebel

Very nice work @aschackmull ! Thank you!

github-actions bot added the C# label Nov 26, 2025

aschackmull force-pushed the csharp/object-initializer branch 2 times, most recently from 49eed3a to 0ef7bee Compare November 27, 2025 13:21

michaelnebel reviewed Nov 27, 2025

View reviewed changes

aschackmull force-pushed the csharp/object-initializer branch from bed13c3 to f7ff3f5 Compare November 28, 2025 13:12

github-advanced-security bot found potential problems Nov 28, 2025

View reviewed changes

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

aschackmull force-pushed the csharp/object-initializer branch 2 times, most recently from 3ee3160 to 70440f3 Compare December 2, 2025 12:18

aschackmull marked this pull request as ready for review December 2, 2025 12:41

aschackmull requested a review from a team as a code owner December 2, 2025 12:41

Copilot AI review requested due to automatic review settings December 2, 2025 12:41

Copilot AI reviewed Dec 2, 2025

View reviewed changes

aschackmull added 10 commits December 2, 2025 13:49

C#: Add object initializer test.

a7066ec

C#: Add extractor support for object initializer methods.

9414cfb

C#: Replace initializer splitting with ObjectInitMethod.

24a575a

C#: Move and rename module.

85121e8

C#: Fixup test

02e5f45

C#: Accept PrintAst index shift.

541dce4

C#: Accept expected changes.

7e4e872

C#: Tweaks from review comments.

ba7b517

C#: Fix enclosing DataFlowCallable of ObjectInitMethods with multiple…

2eb2a50

… bodies.

C#: Accept CFG dead ends for compilation errors.

67a2bce

aschackmull force-pushed the csharp/object-initializer branch from 70440f3 to 1f0a3b7 Compare December 2, 2025 13:01

C#: Accept integration test change

5d63b6e

aschackmull force-pushed the csharp/object-initializer branch from 1f0a3b7 to 5d63b6e Compare December 2, 2025 13:03

michaelnebel reviewed Dec 3, 2025

View reviewed changes

hvitved requested changes Dec 4, 2025

View reviewed changes

C#: Address review comments.

e4ee7c9

michaelnebel previously approved these changes Dec 4, 2025

View reviewed changes

C#: Fix lambda flow.

28e9420

aschackmull dismissed michaelnebel’s stale review via 28e9420 December 5, 2025 09:58

hvitved approved these changes Dec 5, 2025

View reviewed changes

		@@ -0,0 +1,22 @@
		import csharp

		module FlowConfig implements DataFlow::ConfigSig {

C#: Replace initializer splitting with an ObjectInitMethod. #20922

Are you sure you want to change the base?

C#: Replace initializer splitting with an ObjectInitMethod. #20922

Conversation

aschackmull commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelnebel 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!

michaelnebel commented Nov 27, 2025

Uh oh!

aschackmull commented Nov 28, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

michaelnebel 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!

hvitved 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!

aschackmull commented Dec 4, 2025

Uh oh!

michaelnebel commented Dec 4, 2025

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aschackmull commented Nov 26, 2025 •

edited

Loading