Refactor null checks to use 'is null' and 'is not null' syntax by BCSharp · Pull Request #298 · IronLanguages/dlr

BCSharp · 2026-03-01T04:29:46Z

The main benefit of using pattern matching rather than== null and != null is that it bypasses the equality operator, which can be redefined by the type. Theoretically, a redefined equality operator might be less efficient at best, and incorrect at worst.

As far as I can see though, in the DLR there are no cases that a type redefines the equality operator, so there is no performance benefit in using pattern matching (it's a different story in IronPython). However, using pattern matching ensures correctness and improves readability, so that the reader need not check whether there is an overload for those operators.

The changes in this PR are numerous, but the vast majority of them are simple substitutions. On a few occasions, the expression was rewritten in a different way, e.g. using null propagation, null coalescence, switch expression, etc.

slozier

Thanks! Seems fine to me. I was looking to see if there was some code style options we could enable but didn't find anything.

The only questionable changes are the one to comments where they're calling Expression.Equal. If you think changing those comments to is makes sense then I'm fine with it.

slozier · 2026-03-01T14:06:32Z

Src/Microsoft.Dynamic/Ast/MethodCallExpression.cs

-            Debug.Assert(arguments != null);
-            Debug.Assert(arguments != null);
+            Debug.Assert(arguments is not null);
+            Debug.Assert(arguments is not null);


Not an issue with your PR but it's checking arguments twice? Guess it should be parameters...

slozier · 2026-03-01T14:24:12Z

Src/Microsoft.Dynamic/Actions/DynamicSiteHelper.cs

-            if (mb.DeclaringType != null && 
-                mb.DeclaringType.Namespace != null &&
-                mb.DeclaringType.Namespace.StartsWith("Microsoft.Scripting", StringComparison.Ordinal)) {
+            if (mb.DeclaringType?.Namespace?.StartsWith("Microsoft.Scripting", StringComparison.Ordinal) == true) {


Wonder what C# does here? Nullable equality? Just curious...

Wonder what C# does here? Nullable equality? Just curious...

Yes, Nullable equality. But I think I'm going to replace the comparison operator with the null coalescence operator; it generates the same IL code but perhaps is more idiomatic C#.

slozier · 2026-03-02T02:10:13Z

Src/Microsoft.Dynamic/Ast/BinaryExpression.cs

        /// <summary>
        /// Null coalescing expression
-        /// {result} ::= ((tmp = {_left}) == null) ? {right} : tmp
+        /// {result} ::= ((tmp = {_left}) is null) ? {right} : tmp


It is applicable to the expression as well?

slozier · 2026-03-02T02:13:42Z

Src/Microsoft.Dynamic/ComInterop/DispatchArgBuilder.cs

            parameter = Marshal(parameter);

-            // parameter == null ? IntPtr.Zero : Marshal.GetIDispatchForObject(parameter);
+            // parameter is null ? IntPtr.Zero : Marshal.GetIDispatchForObject(parameter);


Comment should still be == I think?

slozier · 2026-03-02T02:15:00Z

Src/Microsoft.Dynamic/ComInterop/UnknownArgBuilder.cs

            parameter = Marshal(parameter);

-            // parameter == null ? IntPtr.Zero : Marshal.GetIUnknownForObject(parameter);
+            // parameter is null ? IntPtr.Zero : Marshal.GetIUnknownForObject(parameter);


Comment should still be ==?

slozier · 2026-03-02T02:29:43Z

Src/Microsoft.Dynamic/Ast/GeneratorRewriter.cs

                    var catchBody = new RethrowRewriter { Exception = deferredVar }.Visit(c.Body);

-                    // if (deferredVar != null) {
+                    // if (deferredVar is not null) {


slozier · 2026-03-02T02:29:57Z

Src/Microsoft.Dynamic/Ast/GeneratorRewriter.cs

                //  if (_finallyReturnVar) goto finallyReturn;
                //   ...
-                //   if (saved != null) throw saved;
+                //   if (saved is not null) throw saved;


slozier · 2026-03-02T02:30:26Z

Src/Microsoft.Dynamic/Ast/LightExceptionRewriter.cs

            //
            // ehLabel:
-            //  if ((e = GetLightException(_lastValue) as Exception)) != null) {
+            //  if ((e = GetLightException(_lastValue) as Exception)) is not null) {


slozier · 2026-03-02T02:33:31Z

Src/Microsoft.Dynamic/Interpreter/LightLambda.Generated.cs

        internal const int MaxParameters = 16;
        internal TRet Run0<TRet>() {
-            if (_compiled != null || TryGetCompiled()) {
+            if (_compiled is not null || TryGetCompiled()) {


I guess there's Python code that will have to be updated for this? Unless the script it no longer being run...

Yes, I have updates to 4 code generation scripts on the IronPython side, to be checked in together with the submodule ref update.

slozier · 2026-03-02T02:43:58Z

Src/Microsoft.Dynamic/Runtime/BinderOps.cs

-            listArgs.CopyTo(res, initialArgs.Length);
-            return res;
+            return additionalArgs switch {
+                IList listArgs => [..initialArgs, ..listArgs],


Definitely nicer syntax. Any idea if it does the optimal thing?

Interesting question. I'd say it's comparable to the old code, sometimes better, sometimes worse, depending on the actual type of additionalArgs. This is not really surprising since the compiler uses static analysis to determine the most performant way to create the collection declared with a collection expression.1 Also, newer versions of Roslyn may come with new code optimizations and tricks so the actual performance may improve over time.

Notable differences:

Array.Copy(initialArgs, res, initialArgs.Length);

is replaced by

ReadOnlySpan<object> readOnlySpan = new ReadOnlySpan<object>(initialArgs); readOnlySpan.CopyTo(new Span<object>(res).Slice(0, readOnlySpan.Length));

…probably the same/similar as Array.Copy.

listArgs.CopyTo(res, initialArgs.Length);

is replaced by an inlined loop copying element-wise; probably the same as ICollection.CopyTo but without the extra call.

This makes me think that the code can be further improved by simply using ICollection as the first type case, rather than IList. This will fast-track a broader group of types used for additionalArgs, something that the original code didn't do.

In the case when additionalArgs is not IList but is IEnumerable, the new form creates an intermediate list, copies initialArgs to it (AddRange), iterates additionalArgs and copies element-wise, then copies the list with ToArray.

This is worse than than the original code, because initialArgs are being copied twice: first to the intermediate list, then to the final array.

The old code behaviour can be restored by:

IEnumerable<object> ie => [..initialArgs, ..new List<object>(ie)],

which is does the copying using spans, but the type pattern is more restrictive (IEnumerable<object> extends IEnumerable).

So not to change the semantics, I'd need to add this switch case between the existing ones. However, if we are talking about performance, a case of object[] arr as the first case would provide even better performance than anything else.

That said, we are talking about argument lists, which are never very long anyway, so performance differences are likely negligible. This method is not called anywhere in the DLR or IronPython, so it is hard to guess a typical use. Therefore I am inclined to leave it as is (except for the change of IList to ICollection — a low-hanging fruit).

BCSharp · 2026-03-03T01:32:14Z

The only questionable changes are the one to comments where they're calling Expression.Equal. If you think changing those comments to is makes sense then I'm fine with it.

I have no preference; in some cases I have left ==/!= in comments, in some cases I changed it, based on a gut feeling. The fact that you think it may be questionable tells me that you have a stronger preference than I do. Since I will be making another commit, I'm going to revert the changes in the comments you have pointed out.

Refactor null checks to use 'is null' and 'is not null' syntax

9cc9abd

slozier reviewed Mar 2, 2026

View reviewed changes

Update after review

59ee964

Conversation

BCSharp commented Mar 1, 2026

Uh oh!

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

BCSharp commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants