Add a code fix provider for invalid DuckType null checks #7448
Conversation
|
Build failures are due to the detected errors, some I already have PRs for but will aim to address them before merging this |
bouwkast
force-pushed
the
steven/r-and-d-august-2025
branch
from
852a4af to
2538a59
Compare
| category: "CodeQuality", | ||
| defaultSeverity: DiagnosticSeverity.Error, | ||
| isEnabledByDefault: true, | ||
| description: "The IDuckType is almost always never null, check the Instance for null to ensure we have access to the ducked object."); |
There was a problem hiding this comment.
"almost always never" 😅
| description: "The IDuckType is almost always never null, check the Instance for null to ensure we have access to the ducked object."); | |
| description: "The IDuckType is almost always non-null, check the Instance for null to ensure we have access to the duck typed object."); |
There was a problem hiding this comment.
I spent an embarrassingly amount of time coming up with how to phrase this 🪦
There was a problem hiding this comment.
Applied in 3d3d344
Forgot that the description is not the "Description" the messageFormat is the "Description" and the description is just optional 😖
| namespace Datadog.Trace.Tools.Analyzers.DuckTypeAnalyzer | ||
| { | ||
| /// <summary> | ||
| /// Checks fo r null checks against IDuckType instances. |
There was a problem hiding this comment.
| /// Checks fo r null checks against IDuckType instances. | |
| /// Checks for null checks against IDuckType instances. |
| // it is an implicit cast / box to object (unlike the `is null` pattern) | ||
| // so we need to undo that before we can check if it is a IDuckType |
There was a problem hiding this comment.
This is very interesting, were you aware of this @tonyredondo? If so, we should probably change the analyzer to make you never use == either and always use is😄
There was a problem hiding this comment.
One thing I'm not sure of is whether this is just a Roslyn thing
| return false; | ||
| } | ||
|
|
||
| // NOTE: IDuckType? == IDuckType which surprised me |
There was a problem hiding this comment.
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 : IDuckTypeThere was a problem hiding this comment.
Ohhh very interesting thanks!
| { | ||
| void TestMethod(IDuckType? duckType) | ||
| { | ||
| if (duckType.Instance {{operand}} null) |
There was a problem hiding this comment.
Given the incoming type is nullable, it's boxed, so I think this actually needs to be:
if (duckType?.Instance {{operand}} null)
There was a problem hiding this comment.
Yeah that's correct but I don't know how to do that 😆
I can poke around a bit more to see how to make it do that
There was a problem hiding this comment.
Hmm yes, this causes Visual Studio to not allow the the code fix to be applied, so I'll work on fixing it.
| // NOTE: The source fixedSource currently only replaces the condition, it does not remove the unnecessary nullability in the constraint. | ||
| // NOTE: Leaving that as an exercise for the reader |
There was a problem hiding this comment.
Technically the nullability isn't unnecessary (I think), so that's good 😄
There was a problem hiding this comment.
Oh interesting okay, so I really should get that supported
Summary of changes
In almost every case checking if an
IDuckTypeis/isn'tnullis invalid as they are virtually always going to exist. Instead we need to check whether theIDuckType.Instanceis/isn'tnull. This is easy to accidentally do, so this PR adds an analyzer and a code fix to find and throw an error for any occurance of checkingIDuckTypefornulland provides a code fix to change to.InstanceReason for change
Help enforce us to not accidentally check the IDuckType for
nulland instead check theInstancefornullImplementation details
Based it off of our other analyzers somewhat.
I've tried to comment the code as much as I could, I mainly built the thing by stepping through the tests in debug and seeing what the various SyntaxNodes were and what to do.
I've excluded the
IActivitybecause we do check it fornullto determine which runtime elements we have available for it via some pattern matching.Test coverage
ispatterns:duckType != null/duckType is not null(object)duckType == null->duckType.Instance == nullTested it out on my VS instance and seems to work good 👍
Other details
There was some ChatGPT usage here around the parenthesis / casting and namespace exclusions as that really tripped me up.