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.

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

Null member access operators ?. and ?[] #10960

Merged

Conversation

adityapatwardhan
Copy link
Member

@adityapatwardhan adityapatwardhan commented Nov 1, 2019

PR Summary

PR for ?. and ?[] operators.
Braces need around variable name. Like ${x}?.Name and ${x}?.Get()
Implementation on RFC - PowerShell/PowerShell-RFC#223

PR Context

The null conditional member access operators allow to check the item for being null before a property is accessed or an index is used or a method is invoked. This helps reduced the null checking logic from scripts.

PR Checklist

@adityapatwardhan adityapatwardhan changed the title WIP: Null member access operators ?. and ?[] Null member access operators ?. and ?[] Nov 12, 2019
@adityapatwardhan adityapatwardhan marked this pull request as ready for review November 12, 2019 00:48
@adityapatwardhan
Copy link
Member Author

@PoshChan please retry Windows

@PoshChan
Copy link
Collaborator

@adityapatwardhan, successfully started retry of PowerShell-CI-Windows

src/System.Management.Automation/engine/parser/ast.cs Outdated Show resolved Hide resolved
src/System.Management.Automation/engine/parser/ast.cs Outdated Show resolved Hide resolved
src/System.Management.Automation/engine/parser/ast.cs Outdated Show resolved Hide resolved
@@ -7989,7 +8053,7 @@ IAssignableValue ISupportsAssignment.GetAssignableValue()
return new InvokeMemberAssignableValue { InvokeMemberExpressionAst = this };
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Remove trailing spaces here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

Choose a reason for hiding this comment

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

Not fixed yet.

src/System.Management.Automation/engine/parser/Compiler.cs Outdated Show resolved Hide resolved
src/System.Management.Automation/engine/parser/Compiler.cs Outdated Show resolved Hide resolved
@@ -7351,11 +7351,11 @@ private ExpressionAst CheckPostPrimaryExpressionOperators(Token token, Expressio
// To support fluent style programming, allow newlines after the member access operator.
SkipNewlines();

if (token.Kind == TokenKind.Dot || token.Kind == TokenKind.ColonColon)
if (token.Kind == TokenKind.Dot || token.Kind == TokenKind.ColonColon || token.Kind == TokenKind.QuestionDot)
Copy link
Member

Choose a reason for hiding this comment

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

In C#, there is no point to support ?. for member access to static members, but in PowerShell, you can do $a = [string]; $a::Equals(...), which makes it debatable whether we want to support it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Though it is technically possible to implement something like: $a = [string]; ${a}?::Equals(...), i doubt there is any usefulness to this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's technically possible. I vote for postponing it until we have a ask. @SteveL-MSFT any thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Though it is technically possible to implement something like: $a = [string]; ${a}?::Equals(...), i doubt there is any usefulness to this.

Where I think it would be most useful is situations where you want to invoke a static method only if an assembly is already loaded. e.g.

('MyCustomType' -as [type])?::Initialize()

Copy link
Collaborator

@vexx32 vexx32 Nov 14, 2019

Choose a reason for hiding this comment

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

That certainly beats my current method! (and has better utility, probably, as well)

switch ($null) {
    ('typename' -as [type]) { . Type.ps1 }
    ('type2name' -as [type]) { . Type2.ps1 }
}

Copy link
Member

Choose a reason for hiding this comment

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

Since it's additive we can postpone

src/System.Management.Automation/engine/parser/token.cs Outdated Show resolved Hide resolved
@adityapatwardhan
Copy link
Member Author

@daxian-dbw - Please re-review.

src/System.Management.Automation/engine/parser/ast.cs Outdated Show resolved Hide resolved
src/System.Management.Automation/engine/parser/ast.cs Outdated Show resolved Hide resolved
src/System.Management.Automation/engine/parser/ast.cs Outdated Show resolved Hide resolved
src/System.Management.Automation/engine/parser/ast.cs Outdated Show resolved Hide resolved
@@ -7989,7 +8053,7 @@ IAssignableValue ISupportsAssignment.GetAssignableValue()
return new InvokeMemberAssignableValue { InvokeMemberExpressionAst = this };
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Not fixed yet.

src/System.Management.Automation/engine/parser/Compiler.cs Outdated Show resolved Hide resolved
@daxian-dbw
Copy link
Member

@rjmholt and @JamesWTruher Can you please review this PR as well? Thanks!

@@ -404,6 +404,7 @@ internal List<CompletionResult> GetResultHelper(CompletionContext completionCont

Copy link
Collaborator

Choose a reason for hiding this comment

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

@adityapatwardhan, your last commit had 1 failures in PowerShell-CI-windows
Enter-PSHostProcess tests.By Process Id.Can enter using NamedPipeConnectionInfo

Exception calling "Invoke" with "0" argument(s): "The runspace state is not valid for this operation."
at <ScriptBlock>, D:\a\1\s\test\powershell\Modules\Microsoft.PowerShell.Core\Enter-PSHostProcess.Tests.ps1: line 136
136:                 $ps.AddScript('$pid').Invoke() | Should -Be $pwshId

@@ -404,6 +404,7 @@ internal List<CompletionResult> GetResultHelper(CompletionContext completionCont

Copy link
Collaborator

Choose a reason for hiding this comment

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

@adityapatwardhan, your last commit had 1 failures in PowerShell-CI-windows
NullConditionalMemberAccess.?. operator tests.Use ?. on a dynamic method name that does not exist

Expected an exception, with FullyQualifiedErrorId 'Argument' to be thrown, but the FullyQualifiedErrorId was 'MethodNotFound'. from D:\a\1\s\test\powershell\Language\Operators\NullConditional.Tests.ps1:343 char:15
    +             { (Get-Date '11/11/2019')?.$methodName() } | Should -Thro ?
    +               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
at <ScriptBlock>, D:\a\1\s\test\powershell\Language\Operators\NullConditional.Tests.ps1: line 343
343:             { (Get-Date '11/11/2019')?.$methodName() } | Should -Throw -ErrorId 'Argument'

@@ -404,6 +404,7 @@ internal List<CompletionResult> GetResultHelper(CompletionContext completionCont

Copy link
Collaborator

Choose a reason for hiding this comment

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

@adityapatwardhan, your last commit had 1 failures in PowerShell-CI-linux
NullConditionalMemberAccess.?. operator tests.Use ?. on a dynamic method name that does not exist

Expected an exception, with FullyQualifiedErrorId 'Argument' to be thrown, but the FullyQualifiedErrorId was 'MethodNotFound'. from /home/vsts/work/1/s/test/powershell/Language/Operators/NullConditional.Tests.ps1:343 char:15
    +             { (Get-Date '11/11/2019')?.$methodName() } | Should -Thro ?
    +               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Language/Operators/NullConditional.Tests.ps1: line 343
343:             { (Get-Date '11/11/2019')?.$methodName() } | Should -Throw -ErrorId 'Argument'

@@ -404,6 +404,7 @@ internal List<CompletionResult> GetResultHelper(CompletionContext completionCont

Copy link
Collaborator

Choose a reason for hiding this comment

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

@adityapatwardhan, your last commit had 1 failures in PowerShell-CI-macos
NullConditionalMemberAccess.?. operator tests.Use ?. on a dynamic method name that does not exist

Expected an exception, with FullyQualifiedErrorId 'Argument' to be thrown, but the FullyQualifiedErrorId was 'MethodNotFound'. from /Users/runner/runners/2.160.0/work/1/s/test/powershell/Language/Operators/NullConditional.Tests.ps1:343 char:15
    +             { (Get-Date '11/11/2019')?.$methodName() } | Should -Thro ?
    +               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
at <ScriptBlock>, /Users/runner/runners/2.160.0/work/1/s/test/powershell/Language/Operators/NullConditional.Tests.ps1: line 343
343:             { (Get-Date '11/11/2019')?.$methodName() } | Should -Throw -ErrorId 'Argument'

@@ -404,6 +404,7 @@ internal List<CompletionResult> GetResultHelper(CompletionContext completionCont

Copy link
Collaborator

Choose a reason for hiding this comment

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

@adityapatwardhan, your last commit had 20 failures in PowerShell-CI-static-analysis
(These are 5 of the failures)

Verify Markdown Links.Verify links in /home/vsts/work/1/s/README.md.https://docs.microsoft.com/powershell/scripting/setup/installing-powershell-core-on-windows?view=powershell-6 should work

retry of URL failed with error: Response status code does not indicate success: 404 (Not Found).
at <ScriptBlock>, /home/vsts/work/1/s/test/common/markdown/markdown-link.tests.ps1: line 117
117:                                     throw "retry of URL failed with error: $($_.Exception.Message)"

Verify Markdown Links.Verify links in /home/vsts/work/1/s/README.md.https://docs.microsoft.com/powershell/scripting/setup/installing-powershell-core-on-linux?view=powershell-6#ubuntu-1804 should work

retry of URL failed with error: Response status code does not indicate success: 404 (Not Found).
at <ScriptBlock>, /home/vsts/work/1/s/test/common/markdown/markdown-link.tests.ps1: line 117
117:                                     throw "retry of URL failed with error: $($_.Exception.Message)"

Verify Markdown Links.Verify links in /home/vsts/work/1/s/README.md.https://docs.microsoft.com/powershell/scripting/setup/installing-powershell-core-on-linux?view=powershell-6#ubuntu-1604 should work

retry of URL failed with error: Response status code does not indicate success: 404 (Not Found).
at <ScriptBlock>, /home/vsts/work/1/s/test/common/markdown/markdown-link.tests.ps1: line 117
117:                                     throw "retry of URL failed with error: $($_.Exception.Message)"

Verify Markdown Links.Verify links in /home/vsts/work/1/s/README.md.https://docs.microsoft.com/powershell/scripting/setup/installing-powershell-core-on-linux?view=powershell-6#debian-9 should work

retry of URL failed with error: Response status code does not indicate success: 404 (Not Found).
at <ScriptBlock>, /home/vsts/work/1/s/test/common/markdown/markdown-link.tests.ps1: line 117
117:                                     throw "retry of URL failed with error: $($_.Exception.Message)"

Verify Markdown Links.Verify links in /home/vsts/work/1/s/README.md.https://docs.microsoft.com/powershell/scripting/setup/installing-powershell-core-on-linux?view=powershell-6#centos-7 should work

retry of URL failed with error: Response status code does not indicate success: 404 (Not Found).
at <ScriptBlock>, /home/vsts/work/1/s/test/common/markdown/markdown-link.tests.ps1: line 117
117:                                     throw "retry of URL failed with error: $($_.Exception.Message)"

Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

One open issue: [System.DBNull]::Value?.GetTypeCode() and [NullString]::Value?.GetType() will return nothing, which might raise confusion. We need to discuss and decide if it's fine to have this semantics for the null-conditional operator.
Talked with @adityapatwardhan offline, and we will discuss and address this in the RC release.
/cc @JamesWTruher @SteveL-MSFT @rjmholt

@adityapatwardhan
Copy link
Member Author

Filed issue: #11084

@adityapatwardhan
Copy link
Member Author

@PoshChan please retry static

@PoshChan
Copy link
Collaborator

@adityapatwardhan, successfully started retry of PowerShell-CI-static-analysis

@adityapatwardhan
Copy link
Member Author

@daxian-dbw Ready to merge?

@daxian-dbw daxian-dbw merged commit 2579c00 into PowerShell:master Nov 16, 2019
@SteveL-MSFT SteveL-MSFT added this to the 7.0.0-preview.6 milestone Nov 18, 2019
@SteveL-MSFT SteveL-MSFT added the CL-Experimental Indicates that a PR should be marked as an Experimental Feature in the Change Log label Nov 18, 2019
@ghost
Copy link

ghost commented Nov 21, 2019

🎉v7.0.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

@adityapatwardhan adityapatwardhan deleted the NullMemberAccessOperators branch June 8, 2020 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-Experimental Indicates that a PR should be marked as an Experimental Feature in the Change Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants