Skip to content

Enable nullable annotations in basic types in Scripting #350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
slozier merged 1 commit into from
Apr 27, 2026
Merged

Enable nullable annotations in basic types in Scripting #350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/core/Microsoft.Scripting/ArgumentTypeException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;
using System.Runtime.Serialization;

Expand All @@ -12,11 +14,11 @@ public ArgumentTypeException()
: base() {
}

public ArgumentTypeException(string message)
public ArgumentTypeException(string? message)
: base(message) {
}

public ArgumentTypeException(string message, Exception innerException)
public ArgumentTypeException(string? message, Exception? innerException)
: base(message, innerException) {
}

Expand Down
2 changes: 2 additions & 0 deletions src/core/Microsoft.Scripting/AssemblyLoadedEventArgs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;
using System.Collections.Generic;
using System.Text;
Expand Down
2 changes: 2 additions & 0 deletions src/core/Microsoft.Scripting/CompilerOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;

namespace Microsoft.Scripting {
Expand Down
2 changes: 2 additions & 0 deletions src/core/Microsoft.Scripting/Hosting/MemberKind.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

namespace Microsoft.Scripting.Hosting {
/// <summary>
/// Specifies the type of member.
Expand Down
2 changes: 2 additions & 0 deletions src/core/Microsoft.Scripting/Hosting/ParameterFlags.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;

namespace Microsoft.Scripting.Hosting {
Expand Down
4 changes: 3 additions & 1 deletion src/core/Microsoft.Scripting/IndexSpan.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;

using Microsoft.Scripting.Utils;
Expand Down Expand Up @@ -33,7 +35,7 @@ public override int GetHashCode() {
return Length.GetHashCode() ^ Start.GetHashCode();
}

public override bool Equals(object obj) =>
public override bool Equals(object? obj) =>
obj is IndexSpan span && Equals(span);

public static bool operator ==(IndexSpan self, IndexSpan other) => self.Equals(other);
Expand Down
6 changes: 4 additions & 2 deletions src/core/Microsoft.Scripting/InvalidImplementationException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;
using System.Runtime.Serialization;

Expand All @@ -12,11 +14,11 @@ public InvalidImplementationException()
: base() {
}

public InvalidImplementationException(string message)
public InvalidImplementationException(string? message)
: base(message) {
}

public InvalidImplementationException(string message, Exception e)
public InvalidImplementationException(string? message, Exception? e)
: base(message, e) {
}

Expand Down
2 changes: 2 additions & 0 deletions src/core/Microsoft.Scripting/Microsoft.Scripting.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<EmitCompilerGeneratedFiles>True</EmitCompilerGeneratedFiles>
<MeziantouPolyfill_IncludedPolyfills>
T:System.Diagnostics.CodeAnalysis.MemberNotNullWhenAttribute;
T:System.Diagnostics.CodeAnalysis.NotNullAttribute;
P:System.Collections.ObjectModel.ReadOnlyCollection`1.Empty;
</MeziantouPolyfill_IncludedPolyfills>
</PropertyGroup>
Expand Down
20 changes: 11 additions & 9 deletions src/core/Microsoft.Scripting/PlatformAdaptationLayer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;
using System.Collections;
using System.Collections.Generic;
Expand Down Expand Up @@ -195,19 +197,19 @@ public virtual string CombinePaths(string path1, string path2) {
return Path.Combine(path1, path2);
}

public virtual string GetFileName(string path) {
public virtual string? GetFileName(string? path) {
return Path.GetFileName(path);
}

public virtual string GetDirectoryName(string path) {
public virtual string? GetDirectoryName(string? path) {
return Path.GetDirectoryName(path);
}

public virtual string GetExtension(string path) {
public virtual string? GetExtension(string? path) {
return Path.GetExtension(path);
}

public virtual string GetFileNameWithoutExtension(string path) {
public virtual string? GetFileNameWithoutExtension(string? path) {
return Path.GetFileNameWithoutExtension(path);
}

Expand All @@ -222,8 +224,8 @@ public virtual bool IsAbsolutePath(string path) {
if (IsSingleRootFileSystem) {
return Path.IsPathRooted(path);
}
var root = Path.GetPathRoot(path);
return root.EndsWith(@":\", StringComparison.Ordinal) || root.EndsWith(@":/", StringComparison.Ordinal);
string? root = Path.GetPathRoot(path);
return root is not null && (root.EndsWith(@":\", StringComparison.Ordinal) || root.EndsWith(@":/", StringComparison.Ordinal));
#else
throw new NotImplementedException();
#endif
Expand Down Expand Up @@ -274,7 +276,7 @@ public virtual void MoveFileSystemEntry(string sourcePath, string destinationPat

#region Environmental Variables

public virtual string GetEnvironmentVariable(string key) {
public virtual string? GetEnvironmentVariable(string key) {
#if FEATURE_PROCESS
return Environment.GetEnvironmentVariable(key);
#else
Expand All @@ -283,7 +285,7 @@ public virtual string GetEnvironmentVariable(string key) {
}

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2201:DoNotRaiseReservedExceptionTypes")]
public virtual void SetEnvironmentVariable(string key, string value) {
public virtual void SetEnvironmentVariable(string key, string? value) {
#if FEATURE_PROCESS
if (value is not null && value.Length == 0) {
SetEmptyEnvironmentVariable(key);
Expand Down Expand Up @@ -316,7 +318,7 @@ public virtual Dictionary<string, string> GetEnvironmentVariables() {

foreach (DictionaryEntry entry in Environment.GetEnvironmentVariables())
{
result.Add((string)entry.Key, (string)entry.Value);
result.Add((string)entry.Key, (entry.Value is string value) ? value : string.Empty);
Copy link
Copy Markdown
Contributor

@slozier slozier Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking at this since it would technically be a change in the public API, but I think it's fine. The whole PAL concept is such a mess, whether we use it or not seems arbitrary (at least in IronPython). Looking at the null vs empty usage in IronPython I think we probably have a few bugs...

Copy link
Copy Markdown
Member Author

@BCSharp BCSharp Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had looked into this, before I decided on the publicly visible nullabiltiy of the method's signature, and it has nothing to do with the messiness of the PAL usage. I am going to write down my findings here, so that we have some record for our future selves and others that may be wondering what happened and why. For an executive summary, aka bottom line aka tl;dr, look to the bottom of this post. If you keep reading, grab a coffee first…

This is PAL, which purpose it to provide the platform functionality in a platform-independent way. Maybe in the past (Silverlight, Unity) it had some important value, but now it practically routes everything through .NET (which is the true PAL now), possibly changing the signatures, like in the case of GetEnvironmentVariables(). It changes the return type from an IDictionary to a more specific and convenient to use Dictionary<string, string> (I would make it an interface, but it is another story; it's public now). The conversion converts from key/value types object/object? to string/string, so there is downcasting involved, as well as nullability change of the value type. So although interface IDictionary can technically, by the virtue of its typing, hold references to any object, reading the documentation of GetEnvironmentVariables() we learn that all keys and values are strings. So the downcasting is safe. As for nullability, the keys in IDictionary must not be null, so the new method signature is correct on that point. The question is the nullability of value. In practice, on all currently supported OSes by the DLR (Windows, macOS, Linux), the OS calls will never return a null value from the environment, by virtue of processing of the environment block. The worst case is an empty string, when a variable exists but is not set to anything. This is still not null. So the signature is correct.

However, the method is virtual, so one can claim it is "breaking" the API. My position is that it is not breaking the API, because we are going from nullability-agnostic to nullability-enabled. The API never made any nullability claims of the signature. As long as a subclass remains nullability-agnostic, it keeps working, even if it returns dictionaries with null values, which, by the way, is wrong, but this rule was not enforced. If the subclass becomes nullability-aware, well, then it has to conform to the nullability-aware PAL API.

The next question is: is it even possible for some sort of GetEnvironmentVariables() in some environment to have null values? It appears that yes, possibly, when reading from a filesystem provider for Windows registry (e.g. Environment.GetEnvironmentVariables(EnvironmentVariableTarget.User) and keys of type REG_NONE). It uses RegistryKey.GetValue(name, ""), where the second argument is the default value in case the key is declared but the value is missing. Note that it is an empty string, and not null; this is important because it shows the intent: values are never null.

The next step in the process is to convert that received registry value to a string, and the code uses Object.ToString(), which has an unfortunate signature of returning string?. My personal opinion is that this is a mistake, and there was a heated discussion online between the .NET developers themselves, plus public opinion, about making the correct choice, when the nullable attributes were introduced. I am not going to summarize here how it went (wrong), but the status quo is that the return value is annotated as nullable (for the sake of API compatibilty, since it is virtual, just as our GetEnvironmentVariables, so the same arguments and counter-arguments), and at the same time the documentation recommends (requires) ToString to never return null. Which is a missed opportunity for the sake of API stability, because that was exactly the point of the presence/absence of ?. So in the end, we have a function call that may return null, but the docs say it should not, though we never get proper compiler support to enforce this rule. In practice, for the existing types of the registry, this will never happen (Microsoft adheres to its own rules here), but null coalescing to an empty string in PAL enforces this in a non-exception-throwing manner, in some weird or future scenarios.

Bottom Line

  1. The intention from PAL is to (among other things) provide a convenient interface change from weakly typed IDictionary to strongly typed IDictionary<string,string>.
  2. This dictionary should not contain null values, but if a subclass does it anyway, it is still OK (null-forgiving operator, nullability-agnostic). The nullability annotation on string does not introduce a different type.

}

return result;
Expand Down
2 changes: 2 additions & 0 deletions src/core/Microsoft.Scripting/Properties/AssemblyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;
using System.Reflection;
using System.Runtime.CompilerServices;
Expand Down
16 changes: 9 additions & 7 deletions src/core/Microsoft.Scripting/Runtime/DynamicStackFrame.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;
using System.Reflection;

Expand All @@ -11,30 +13,30 @@ namespace Microsoft.Scripting.Runtime {
/// </summary>
[Serializable]
public class DynamicStackFrame {
private readonly string _funcName;
private readonly string _filename;
private readonly string? _funcName;
private readonly string? _filename;
private readonly int _lineNo;
private readonly MethodBase _method;
private readonly MethodBase? _method;

public DynamicStackFrame(MethodBase method, string funcName, string filename, int line) {
public DynamicStackFrame(MethodBase? method, string? funcName, string? filename, int line) {
_funcName = funcName;
_filename = filename;
_lineNo = line;
_method = method;
}

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate")]
public MethodBase GetMethod() {
public MethodBase? GetMethod() {
return _method;
}

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate")]
public string GetMethodName() {
public string? GetMethodName() {
return _funcName;
}

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate")]
public string GetFileName() {
public string? GetFileName() {
return _filename;
}

Expand Down
2 changes: 2 additions & 0 deletions src/core/Microsoft.Scripting/Runtime/NotNullAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;

namespace Microsoft.Scripting.Runtime {
Expand Down
2 changes: 2 additions & 0 deletions src/core/Microsoft.Scripting/Runtime/ParamDictionaryAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;

namespace Microsoft.Scripting {
Expand Down
2 changes: 2 additions & 0 deletions src/core/Microsoft.Scripting/Runtime/StreamContentProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;
using System.IO;

Expand Down
2 changes: 2 additions & 0 deletions src/core/Microsoft.Scripting/Runtime/TokenTriggers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;

namespace Microsoft.Scripting {
Expand Down
2 changes: 2 additions & 0 deletions src/core/Microsoft.Scripting/ScriptCodeParseResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

namespace Microsoft.Scripting {
public enum ScriptCodeParseResult {
/// <summary>
Expand Down
2 changes: 2 additions & 0 deletions src/core/Microsoft.Scripting/Severity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

namespace Microsoft.Scripting {
public enum Severity {
Ignore,
Expand Down
2 changes: 2 additions & 0 deletions src/core/Microsoft.Scripting/SourceCodeKind.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System.ComponentModel;

namespace Microsoft.Scripting {
Expand Down
4 changes: 3 additions & 1 deletion src/core/Microsoft.Scripting/SourceLocation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;
using System.Globalization;

Expand Down Expand Up @@ -156,7 +158,7 @@ public static int Compare(SourceLocation left, SourceLocation right) {
public bool Equals(SourceLocation other) =>
other.Index == Index && other.Line == Line && other.Column == Column;

public override bool Equals(object obj) =>
public override bool Equals(object? obj) =>
obj is SourceLocation other && Equals(other);

public override int GetHashCode() {
Expand Down
2 changes: 2 additions & 0 deletions src/core/Microsoft.Scripting/TokenCategory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

namespace Microsoft.Scripting {

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1027:MarkEnumsWithFlags")]
Expand Down
13 changes: 10 additions & 3 deletions src/core/Microsoft.Scripting/Utils/ArrayUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;
using System.Collections.Generic;
using System.Diagnostics;
Expand All @@ -21,7 +23,12 @@ public FunctorComparer(Comparison<T> comparison) {
_comparison = comparison;
}

public int Compare(T x, T y) {
public int Compare(T? x, T? y) {
if (x is null) {
return (y is null) ? 0 : -1;
} else if (y is null) {
return 1;
}
return _comparison(x, y);
}
}
Expand Down Expand Up @@ -68,7 +75,7 @@ public static T[] MakeArray<T>(ICollection<T> list) {
return res;
}

public static T[] MakeArray<T>(ICollection<T> elements, int reservedSlotsBefore, int reservedSlotsAfter) {
public static T[] MakeArray<T>(ICollection<T>? elements, int reservedSlotsBefore, int reservedSlotsAfter) {
if (reservedSlotsAfter < 0) throw new ArgumentOutOfRangeException(nameof(reservedSlotsAfter));
if (reservedSlotsBefore < 0) throw new ArgumentOutOfRangeException(nameof(reservedSlotsBefore));

Expand Down Expand Up @@ -192,7 +199,7 @@ public static T[] AppendRange<T>(T[] array, IList<T> items, int additionalItemCo
}

public static void SwapLastTwo<T>(T[] array) {
Debug.Assert(array is not null && array.Length >= 2);
Debug.Assert(array.Length >= 2);

T temp = array[array.Length - 1];
array[array.Length - 1] = array[array.Length - 2];
Expand Down
Loading