Skip to content
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

Jump to bottom

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

xtqqczze
Copy link
Contributor

Follow-up to #14088, #14181.

@ghost ghost assigned daxian-dbw Nov 23, 2020
xtqqczze
xtqqczze commented Nov 23, 2020
View reviewed changes
src/System.Management.Automation/engine/parser/ast.cs
@@ -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

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

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

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

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

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

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

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

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.

@ghost ghost added the Review - Needed The PR is being reviewed label Nov 30, 2020
@ghost
Copy link

ghost commented Nov 30, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@xtqqczze
Copy link
Contributor Author

@powercode Can you review?

powercode
powercode reviewed Dec 1, 2020
View reviewed changes
src/System.Management.Automation/engine/parser/ast.cs
@@ -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
Collaborator

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.

src/System.Management.Automation/engine/parser/ast.cs
@@ -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
Collaborator

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

src/System.Management.Automation/engine/parser/ast.cs
@@ -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

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

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

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.

@SteveL-MSFT SteveL-MSFT added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Dec 15, 2020
@xtqqczze xtqqczze closed this Jun 25, 2021
@xtqqczze xtqqczze reopened this Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@powercode powercode powercode left review comments

@iSazonov iSazonov iSazonov left review comments

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@daxian-dbw daxian-dbw

Labels
CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log Review - Needed The PR is being reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@xtqqczze @powercode @iSazonov @daxian-dbw @SteveL-MSFT