Skip to content

Enable nullable: System.Management.Automation.Language.TypeName #14232

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

Closed
xtqqczze wants to merge 1 commit into from
Closed

Enable nullable: System.Management.Automation.Language.TypeName #14232

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
66 changes: 35 additions & 31 deletions src/System.Management.Automation/engine/parser/ast.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ namespace System.Management.Automation.Language
using SwitchClause = Tuple<ExpressionAst, StatementBlockAst>;
using System.Runtime.CompilerServices;
using System.Reflection.Emit;
using System.Diagnostics;

internal interface ISupportsAssignment
{
Expand Down Expand Up @@ -8175,12 +8176,13 @@ internal interface ISupportsTypeCaching
/// <summary>
/// A simple type that is not an array or does not have generic arguments.
/// </summary>
#nullable enable
public sealed class TypeName : ITypeName, ISupportsTypeCaching
{
internal readonly string _name;
internal Type _type;
internal Type? _type;
internal readonly IScriptExtent _extent;
internal TypeDefinitionAst _typeDefinitionAst;
internal TypeDefinitionAst? _typeDefinitionAst;

/// <summary>
/// Construct a simple typename.
Expand Down Expand Up @@ -8252,7 +8254,7 @@ public TypeName(IScriptExtent extent, string name, string assembly)
/// <summary>
/// The name of the assembly, if specified, otherwise null.
/// </summary>
public string AssemblyName { get; internal set; }
public string? AssemblyName { get; internal set; }

/// <summary>
/// Always returns false, array typenames are instances of <see cref="ArrayTypeName"/>.
Expand All @@ -8273,7 +8275,7 @@ internal bool HasDefaultCtor()
{
if (_typeDefinitionAst == null)
{
Type reflectionType = GetReflectionType();
Type? reflectionType = GetReflectionType();
if (reflectionType == null)
{
// we are pessimistic about default ctor presence.
Expand Down Expand Up @@ -8312,7 +8314,7 @@ internal bool HasDefaultCtor()
/// The <see cref="Type"/> if possible, null otherwise. Null may be returned for valid typenames if the assembly
/// containing the type has not been loaded.
/// </returns>
public Type GetReflectionType()
public Type? GetReflectionType()
{
if (_type == null)
{
Expand Down Expand Up @@ -8347,7 +8349,7 @@ public Type GetReflectionType()
/// The <see cref="Type"/> if possible, null otherwise. Null may be returned for valid typenames if the assembly
/// containing the type has not been loaded.
/// </returns>
public Type GetReflectionAttributeType()
public Type? GetReflectionAttributeType()
{
var result = GetReflectionType();
if (result == null || !typeof(Attribute).IsAssignableFrom(result))
Expand Down Expand Up @@ -8379,7 +8381,7 @@ public override string ToString()
}

/// <summary/>
public override bool Equals(object obj)
public override bool Equals(object? obj)
{
if (!(obj is TypeName other))
return false;
Expand Down Expand Up @@ -8418,7 +8420,8 @@ public override int GetHashCode()
/// </remarks>
internal bool IsType(Type type)
{
string fullTypeName = type.FullName;
string? fullTypeName = type.FullName;
Debug.Assert(fullTypeName is not null);
Copy link
Contributor Author

@xtqqczze xtqqczze Nov 23, 2020

Choose a reason for hiding this comment

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

Maybe we should check for null?

if (fullTypeName is null)
    return false;

Copy link
Contributor Author

@xtqqczze xtqqczze Nov 30, 2020

Choose a reason for hiding this comment

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

The Debug.Assert is necessary here to tell the compiler fullTypeName is not null.

In my opinion there should an actual null check here, but in a separate PR,

Copy link
Collaborator

@powercode powercode Dec 1, 2020

Choose a reason for hiding this comment

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

I don't think it can ever be null. Fullname is the qualified name of the type, including the namespace. I don't see how that could ever be null.

The fully qualified name of the type, including its namespace but not its assembly; or null if the current instance represents a generic type parameter, an array type, pointer type, or byref type based on a type parameter, or a generic type that is not a generic type definition but contains unresolved type parameters.

Copy link
Collaborator

@powercode powercode Dec 1, 2020

Choose a reason for hiding this comment

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

Looking at the summary of the type, it says

A simple type that is not an array or does not have generic arguments.

So basically, we are meeting the requirement that it has a typename.
I think you can bang it, maybe with a comment.

i.e.

string fullTypeName = type.FullName!;

Copy link
Contributor Author

@xtqqczze xtqqczze Dec 1, 2020

Choose a reason for hiding this comment

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

string fullTypeName = type.FullName!;

What about the possible NullReferenceException in the next line?

Copy link
Collaborator

@powercode powercode Dec 1, 2020

Choose a reason for hiding this comment

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

type always has a fullname in the cases this class handles.

Copy link
Contributor Author

@xtqqczze xtqqczze Dec 1, 2020

Choose a reason for hiding this comment

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

Currently, the only reference to the TypeName.IsType method is from FunctionMemberAst.IsReturnTypeVoid. But the method is internal, it is accessible to any file in System.Management.Automation, so I think we should try to avoid the NullReferenceException.

Copy link
Collaborator

@iSazonov iSazonov Jun 25, 2021

Choose a reason for hiding this comment

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

Formally fullTypeName is nullable. So I am ok with adding the Debug.Assert() here. We should avoid changing a code at annotation time.

if (fullTypeName.Equals(Name, StringComparison.OrdinalIgnoreCase))
return true;
int lastDotIndex = fullTypeName.LastIndexOf('.');
Expand All @@ -8430,7 +8433,7 @@ internal bool IsType(Type type)
return false;
}

Type ISupportsTypeCaching.CachedType
Type? ISupportsTypeCaching.CachedType
{
get { return _type; }

Expand All @@ -8443,8 +8446,8 @@ Type ISupportsTypeCaching.CachedType
/// </summary>
public sealed class GenericTypeName : ITypeName, ISupportsTypeCaching
{
private string _cachedFullName;
private Type _cachedType;
private string? _cachedFullName;
private Type? _cachedType;

/// <summary>
/// Construct a generic type name.
Expand Down Expand Up @@ -8556,7 +8559,7 @@ public string Name
/// <summary>
/// The name of the assembly, if specified, otherwise null.
/// </summary>
public string AssemblyName { get { return TypeName.AssemblyName; } }
public string? AssemblyName { get { return TypeName.AssemblyName; } }

/// <summary>
/// Always returns false because this class does not represent arrays.
Expand Down Expand Up @@ -8586,11 +8589,11 @@ public string Name
/// <summary>
/// Returns the <see cref="System.Type"/> that this typename represents, if such a type exists, null otherwise.
/// </summary>
public Type GetReflectionType()
public Type? GetReflectionType()
{
if (_cachedType == null)
{
Type generic = GetGenericType(TypeName.GetReflectionType());
Type? generic = GetGenericType(TypeName.GetReflectionType());

if (generic != null && generic.ContainsGenericParameters)
{
Expand Down Expand Up @@ -8639,7 +8642,7 @@ public Type GetReflectionType()
/// </summary>
/// <param name="generic"></param>
/// <returns></returns>
internal Type GetGenericType(Type generic)
internal Type? GetGenericType(Type? generic)
{
if (generic == null || !generic.ContainsGenericParameters)
{
Expand All @@ -8662,12 +8665,12 @@ internal Type GetGenericType(Type generic)
/// The <see cref="Type"/> if possible, null otherwise. Null may be returned for valid typenames if the assembly
/// containing the type has not been loaded.
/// </returns>
public Type GetReflectionAttributeType()
public Type? GetReflectionAttributeType()
{
Type type = GetReflectionType();
Type? type = GetReflectionType();
if (type == null)
{
Type generic = TypeName.GetReflectionAttributeType();
Type? generic = TypeName.GetReflectionAttributeType();
if (generic == null || !generic.ContainsGenericParameters)
{
if (!TypeName.FullName.Contains('`'))
Expand Down Expand Up @@ -8697,7 +8700,7 @@ public override string ToString()
}

/// <summary/>
public override bool Equals(object obj)
public override bool Equals(object? obj)
{
if (!(obj is GenericTypeName other))
return false;
Expand Down Expand Up @@ -8731,7 +8734,7 @@ public override int GetHashCode()
return hash;
}

Type ISupportsTypeCaching.CachedType
Type? ISupportsTypeCaching.CachedType
{
get { return _cachedType; }

Expand All @@ -8744,8 +8747,8 @@ Type ISupportsTypeCaching.CachedType
/// </summary>
public sealed class ArrayTypeName : ITypeName, ISupportsTypeCaching
{
private string _cachedFullName;
private Type _cachedType;
private string? _cachedFullName;
private Type? _cachedType;

/// <summary>
/// Construct an ArrayTypeName.
Expand Down Expand Up @@ -8836,7 +8839,7 @@ public string Name
/// <summary>
/// The name of the assembly, if specified, otherwise null.
/// </summary>
public string AssemblyName { get { return ElementType.AssemblyName; } }
public string? AssemblyName { get { return ElementType.AssemblyName; } }

/// <summary>
/// Returns true always as this class represents arrays.
Expand Down Expand Up @@ -8866,14 +8869,14 @@ public string Name
/// <summary>
/// Returns the <see cref="System.Type"/> that this typename represents, if such a type exists, null otherwise.
/// </summary>
public Type GetReflectionType()
public Type? GetReflectionType()
{
try
{
RuntimeHelpers.EnsureSufficientExecutionStack();
if (_cachedType == null)
{
Type elementType = ElementType.GetReflectionType();
Type? elementType = ElementType.GetReflectionType();
if (elementType != null)
{
Type type = Rank == 1 ? elementType.MakeArrayType() : elementType.MakeArrayType(Rank);
Expand Down Expand Up @@ -8908,7 +8911,7 @@ public Type GetReflectionType()
/// <summary>
/// Always return null, arrays can never be an attribute.
/// </summary>
public Type GetReflectionAttributeType()
public Type? GetReflectionAttributeType()
{
return null;
}
Expand All @@ -8922,7 +8925,7 @@ public override string ToString()
}

/// <summary/>
public override bool Equals(object obj)
public override bool Equals(object? obj)
{
if (!(obj is ArrayTypeName other))
return false;
Expand All @@ -8936,7 +8939,7 @@ public override int GetHashCode()
return Utils.CombineHashCodes(ElementType.GetHashCode(), Rank.GetHashCode());
}

Type ISupportsTypeCaching.CachedType
Type? ISupportsTypeCaching.CachedType
{
get { return _cachedType; }

Expand Down Expand Up @@ -8981,7 +8984,7 @@ public ReflectionTypeName(Type type)
/// <summary>
/// The name of the assembly.
/// </summary>
public string AssemblyName { get { return _type.Assembly.FullName; } }
public string? AssemblyName { get { return _type.Assembly.FullName; } }
Copy link
Collaborator

@powercode powercode Dec 1, 2020

Choose a reason for hiding this comment

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

I wonder if this will ever be null. I know that it's annotated as such on Assembly, but I haven't been able to produce an instance of it.

@daxian-dbw, do you know?

Copy link
Contributor Author

@xtqqczze xtqqczze Dec 1, 2020

Choose a reason for hiding this comment

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

AssemblyName is public property in public class System.Management.Automation.Language.ReflectionTypeName, I don't think can should assume it can never return null.

Copy link
Collaborator

@iSazonov iSazonov Jun 25, 2021

Choose a reason for hiding this comment

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

Since we use .Net API without wrapping we should follow .Net.
I suggest to keep this as is and open new tracking issue in PowerShell repo and new issue in .Net Runtime repo.
Also I guess we could suddenly get null if single file packaging is used.


/// <summary>
/// Returns true if the type is an array, false otherwise.
Expand Down Expand Up @@ -9023,7 +9026,7 @@ public override string ToString()
}

/// <summary/>
public override bool Equals(object obj)
public override bool Equals(object? obj)
{
if (!(obj is ReflectionTypeName other))
return false;
Expand All @@ -9036,7 +9039,7 @@ public override int GetHashCode()
return _type.GetHashCode();
}

Type ISupportsTypeCaching.CachedType
Type? ISupportsTypeCaching.CachedType
{
get { return _type; }

Expand Down Expand Up @@ -9101,6 +9104,7 @@ internal override AstVisitAction InternalVisit(AstVisitor visitor)

#endregion Visitors
}
#nullable restore

/// <summary>
/// The ast representing a variable reference, either normal references, e.g. <c>$true</c>, or splatted references
Expand Down