Add a code fix provider for invalid DuckType null checks #7448

bouwkast · 2025-08-28T20:59:38Z

Summary of changes

This introduces a new Roslyn analyzer to flag instances where we are directly checking if an IDuckType is or isn't null.
In most cases this is incorrect and the correct equality check would be to check if the IDuckType.Instance is or isn't null.

The analyzer flags all instances of the former as a new rule called DDDUCK001 🦆 .

In addition to the analyzer, I've added a code fix provider that rewrites the flagged expression to use ?.Instance instead. This allows us to correct the issue and still check for null on the base IDuckType just to be safe.

It is important to note though that not every instance of this detection is 100% accurate - there are and will be cases where checking if the IDuckType is null is the correct thing to do.

However, using the conditional accessor should still be safe, but for transparency I've added some current cases to a pramga and to tracer/GlobalSuppressions.cs.

To ensure that we do not potentially disrupt developer workflows drastically with this the analyzer is flagged as a Warning, it will be bumped to an Error at a later point.

Reason for change

Help enforce us to not accidentally check the IDuckType for null and instead check the Instance for null.

Using a null conditional (?.) also means that we are still checking if the IDuckType is null on the off chance that it is.

Implementation details

The analyzer is based off of our other analyzers (somewhat) and largely was based on stepping through the code in the tests in debug mode (works well in VS).

It will match on the following expressions: ==, !=, is, is not.
This is only when the left and right hand sides implement an IDuckType and the other side is the null keyword.

I went through each of the current violations of the analyzer and either applied the code fix in VS automatically or ignored them on a case-by-case basis.

Test coverage

Tests have been added for the following usage patterns (code fix and analyzer included)
- binary and is patterns: duckType != null / duckType is not null
- generic constraints: where T : IDuckType
- (object)duckType == null -> duckType?.Instance == null

Other details

There was some ChatGPT usage here around the parenthesis / casting and namespace exclusions as that really tripped me up.

bouwkast · 2025-08-28T21:43:00Z

Build failures are due to the detected errors, some I already have PRs for but will aim to address them before merging this

andrewlock

Awesome!

tracer/src/Datadog.Trace.Tools.Analyzers/DuckTypeAnalyzer/DuckDiagnostics.cs

tracer/src/Datadog.Trace.Tools.Analyzers/DuckTypeAnalyzer/DuckTypeNullCheckAnalyzer.cs

andrewlock · 2025-08-29T13:16:56Z

tracer/src/Datadog.Trace.Tools.Analyzers/DuckTypeAnalyzer/DuckTypeNullCheckAnalyzer.cs

+                return false;
+            }
+
+            // NOTE: IDuckType? == IDuckType which surprised me


I think that's because IDuckType is effectively a reference type by default, so the ? is an annotation on that rather than making the struct nullable.

That's why this is bad (it has to box to an object):

public void MyMethod(IDuckType duck);

And this is good (no boxing)

public void MyMethod<T>(T duck); where T : IDuckType

Ohhh very interesting thanks!

...test/Datadog.Trace.Tools.Analyzers.Tests/DuckTypeAnalyzers/DuckTypeNullCheckAnalyzerTests.cs

Whenever you open up a code file where the error was the error would disappear. This was because the syntax tree changes when you open it and Roslyn appears to be swapping the type to "object" and boxing it. I noticed this earlier but thought I resolved it. Ended up going for a much simpler approach that seems consistent.

Sometimes the null check is okay

dd-trace-dotnet-ci-bot · 2025-09-24T21:14:24Z

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:

Welch test with statistical test for significance of 5%
Only results indicating a difference greater than 5% and 5 ms are considered.

Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard.

Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph).

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7448) - mean (72ms)  : 71, 73
     .   : milestone, 72,
    master - mean (72ms)  : 70, 73
     .   : milestone, 72,

    section Baseline
    This PR (7448) - mean (68ms)  : 66, 70
     .   : milestone, 68,
    master - mean (68ms)  : 67, 70
     .   : milestone, 68,

    section CallTarget+Inlining+NGEN
    This PR (7448) - mean (1,050ms)  : 1004, 1097
     .   : milestone, 1050,
    master - mean (1,051ms)  : 995, 1106
     .   : milestone, 1051,

gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7448) - mean (107ms)  : 105, 109
     .   : milestone, 107,
    master - mean (107ms)  : 105, 108
     .   : milestone, 107,

    section Baseline
    This PR (7448) - mean (107ms)  : 104, 109
     .   : milestone, 107,
    master - mean (106ms)  : 104, 109
     .   : milestone, 106,

    section CallTarget+Inlining+NGEN
    This PR (7448) - mean (759ms)  : 727, 790
     .   : milestone, 759,
    master - mean (745ms)  : 728, 763
     .   : milestone, 745,

gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7448) - mean (104ms)  : 101, 107
     .   : milestone, 104,
    master - mean (101ms)  : 99, 102
     .   : milestone, 101,

    section Baseline
    This PR (7448) - mean (104ms)  : 100, 108
     .   : milestone, 104,
    master - mean (100ms)  : 98, 102
     .   : milestone, 100,

    section CallTarget+Inlining+NGEN
    This PR (7448) - mean (847ms)  : crit, 771, 923
     .   : crit, milestone, 847,
    master - mean (771ms)  : 716, 826
     .   : milestone, 771,

gantt
    title Execution time (ms) FakeDbCommand (.NET 8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7448) - mean (111ms)  : crit, 108, 113
     .   : crit, milestone, 111,
    master - mean (93ms)  : 92, 94
     .   : milestone, 93,

    section Baseline
    This PR (7448) - mean (108ms)  : 105, 111
     .   : milestone, 108,
    master - mean (92ms)  : 90, 94
     .   : milestone, 92,

    section CallTarget+Inlining+NGEN
    This PR (7448) - mean (737ms)  : crit, 709, 765
     .   : crit, milestone, 737,
    master - mean (663ms)  : 651, 675
     .   : milestone, 663,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7448) - mean (215ms)  : 204, 225
     .   : milestone, 215,
    master - mean (207ms)  : 197, 218
     .   : milestone, 207,

    section Baseline
    This PR (7448) - mean (203ms)  : 194, 212
     .   : milestone, 203,
    master - mean (204ms)  : 192, 216
     .   : milestone, 204,

    section CallTarget+Inlining+NGEN
    This PR (7448) - mean (1,232ms)  : 1149, 1315
     .   : milestone, 1232,
    master - mean (1,215ms)  : 1146, 1284
     .   : milestone, 1215,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7448) - mean (310ms)  : 290, 329
     .   : milestone, 310,
    master - mean (296ms)  : 280, 311
     .   : milestone, 296,

    section Baseline
    This PR (7448) - mean (302ms)  : 287, 318
     .   : milestone, 302,
    master - mean (292ms)  : 276, 309
     .   : milestone, 292,

    section CallTarget+Inlining+NGEN
    This PR (7448) - mean (990ms)  : 942, 1039
     .   : milestone, 990,
    master - mean (983ms)  : 936, 1030
     .   : milestone, 983,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7448) - mean (304ms)  : 287, 321
     .   : milestone, 304,
    master - mean (297ms)  : 284, 310
     .   : milestone, 297,

    section Baseline
    This PR (7448) - mean (302ms)  : 286, 317
     .   : milestone, 302,
    master - mean (298ms)  : 282, 314
     .   : milestone, 298,

    section CallTarget+Inlining+NGEN
    This PR (7448) - mean (1,029ms)  : 971, 1088
     .   : milestone, 1029,
    master - mean (1,041ms)  : 977, 1105
     .   : milestone, 1041,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7448) - mean (289ms)  : 273, 304
     .   : milestone, 289,
    master - mean (287ms)  : 272, 302
     .   : milestone, 287,

    section Baseline
    This PR (7448) - mean (290ms)  : 274, 305
     .   : milestone, 290,
    master - mean (289ms)  : 274, 305
     .   : milestone, 289,

    section CallTarget+Inlining+NGEN
    This PR (7448) - mean (934ms)  : 819, 1048
     .   : milestone, 934,
    master - mean (917ms)  : 829, 1005
     .   : milestone, 917,

This will help ease onto the analyzer / code fix and not accidentally cause more developer friction.

bouwkast · 2025-09-30T20:01:46Z

.../Datadog.Trace.ClrProfiler.Managed.Tests/AutoInstrumentation/AWS/Shared/SpanPointersTests.cs

        var stringAttr = new Mock<IDynamoDbAttributeValue>();
        stringAttr.Setup(a => a.S).Returns("testvalue");
        var keys = new Mock<IDynamoDbKeysObject>();
+        keys.Setup(k => k.Instance).Returns(new object()); // not important of what it returns as long as it isn't null


Done due to the new (correct) check if the keys.Instance is null

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AWS/Shared/SpanPointers.cs

bouwkast · 2025-09-30T20:03:36Z

@codex review

chatgpt-codex-connector · 2025-09-30T20:06:16Z

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

An unknown error occurred

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting

@codex fix this CI failure
@codex address that feedback

bouwkast · 2025-09-30T20:18:56Z

@codex review

chatgpt-codex-connector · 2025-09-30T20:21:05Z

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

An unknown error occurred

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting

@codex fix this CI failure
@codex address that feedback

bouwkast · 2025-09-30T22:10:36Z

@codex review

chatgpt-codex-connector · 2025-09-30T22:13:16Z

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

An unknown error occurred

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting

@codex fix this CI failure
@codex address that feedback

bouwkast requested a review from a team as a code owner August 28, 2025 20:59

bouwkast added type:reliability area:duck-typing labels Aug 28, 2025

bouwkast force-pushed the steven/r-and-d-august-2025 branch from 852a4af to 2538a59 Compare August 28, 2025 21:44

andrewlock approved these changes Aug 29, 2025

View reviewed changes

bouwkast added 13 commits September 24, 2025 10:20

Add a code fix provider for invalid DuckType null checks

ffba22a

The description is optional

db7d7cd

Change "almost always never null" to "almost always non-null"

c557a85

Correctly code fix ?.

c80e9d6

Always code fix to a conditional accessor

7920c2b

Fix null IDuckType checks in Kafka

5e48352

Fix null IDuckType checks in S3

53b7b21

Fix null IDuckType checks in AWS StepFunctions

903274b

Fix null IDuckType checks in AWS SpanPointers

23a0f25

Fix null IDuckType checks in Serilog

fe13001

Fix null IDuckType checks in NLog

84b62a1

Ignore some false instances

146c511

Sometimes the null check is okay

bouwkast force-pushed the steven/r-and-d-august-2025 branch from a57af5c to 146c511 Compare September 24, 2025 20:28

bouwkast requested review from a team as code owners September 24, 2025 20:28

bouwkast requested a review from link04 September 24, 2025 20:28

This comment has been minimized.

Sign in to view

bouwkast added 2 commits September 29, 2025 16:45

Fix SpanPointersTests using a mocked DuckType

3aeb86e

Downgrade fom error to warning

5a28e0b

This will help ease onto the analyzer / code fix and not accidentally cause more developer friction.

bouwkast mentioned this pull request Sep 30, 2025

Fix all instances of DDDUCK001 (invalid null check for IDuckType) #7454

Closed

bouwkast commented Sep 30, 2025

View reviewed changes

bouwkast merged commit 028cfa1 into master Sep 30, 2025
153 checks passed

bouwkast deleted the steven/r-and-d-august-2025 branch September 30, 2025 22:13

github-actions bot added this to the vNext-v3 milestone Sep 30, 2025

This was referenced Sep 30, 2025

Protect against NullReferenceExceptions in AWS S3 Integration #7365

Closed

Protect against more NullReferenceExceptions in Kafka #6834

Closed

Uh oh!

Add a code fix provider for invalid DuckType null checks #7448

Add a code fix provider for invalid DuckType null checks #7448

Uh oh!

Conversation

bouwkast commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

Uh oh!

bouwkast commented Aug 28, 2025

Uh oh!

andrewlock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewlock Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

bouwkast Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

dd-trace-dotnet-ci-bot bot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Execution-Time Benchmarks Report ⏱️

Uh oh!

bouwkast Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

bouwkast commented Sep 30, 2025

Uh oh!

chatgpt-codex-connector bot commented Sep 30, 2025

Uh oh!

bouwkast commented Sep 30, 2025

Uh oh!

chatgpt-codex-connector bot commented Sep 30, 2025

Uh oh!

bouwkast commented Sep 30, 2025

Uh oh!

chatgpt-codex-connector bot commented Sep 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bouwkast commented Aug 28, 2025 •

edited

Loading

bouwkast Aug 29, 2025 •

edited

Loading

dd-trace-dotnet-ci-bot bot commented Sep 24, 2025 •

edited

Loading