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

Add Type Inference for $_ / $PSItem in catch{ } blocks #8020

Merged
merged 9 commits into from Oct 18, 2018

Conversation

@vexx32
Copy link
Contributor

commented Oct 13, 2018

PR Summary

Fix #7828

  • Adds type inference results to $_ / $PSItem when it is used to refer to the ErrorRecord in a catch block.
  • Adds test for the type inference.
  • Includes type inference for typed catch [ExceptionType] { $_ } blocks such that $_ is inferred as a dummy ErrorRecord<TException> which is identical to ErrorRecord but with a TException-typed Exception member.
    • In other words, using typed catch blocks correctly autocompletes the members for $_.Exception.<tab>

Example:

PS> try {throw 'ball'} catch {$_.<tab>
# completes to:
PS> try {throw 'ball'} catch {$_.CategoryInfo

(Compare current behaviour, where tab completion is missing due to a lack of type inference on the $_ / $PSItem variable.)

PR Checklist

@vexx32 vexx32 requested review from BrucePay and daxian-dbw as code owners Oct 13, 2018

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Oct 15, 2018

Perhaps we need a test that try {throw 'ball'} catch {$d=Get-Date; $_.CategoryInfo works.

@iSazonov iSazonov self-assigned this Oct 15, 2018

@iSazonov iSazonov requested a review from lzybkr Oct 15, 2018

@lzybkr

lzybkr approved these changes Oct 15, 2018

Copy link
Member

left a comment

It would be cool to also add special handling for:

try {}
catch [SomeSpecificException] { $_.Exception.<TAB> }

I realize this could be a bit messy though and I'm not sure it's commonly needed, so maybe it's not worth it.

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

Can you expand upon that? As-is, the currently available logic handles this as you'd expect, completing the members for a generic exception object.

Are there some exceptions with additional members we would want to try to detect from the exception type handed to the catch block itself? I'm not aware of any, but I'm sure there are probably some or could have some definitely in the case of custom exception types as well.

@lzybkr I don't think this would go quite in the same place on this code (would it?), so if you could point out where it would need to be added, I'll give it a whirl.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Oct 15, 2018

@vexx32 Please add test to demo that both cases work.

@vexx32 vexx32 force-pushed the vexx32:TypeInference/Catch_PSItem branch Oct 15, 2018

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

@lzybkr @iSazonov

Added tests.

With @SeeminglyScience's help, also added in some type inference for typed catch blocks. He came up with the idea of a dummy generic ErrorRecord<T> class that could be used just for type inference in order to get the members of the typed exceptions.

Pretty happy with it! 😄

@vexx32 vexx32 force-pushed the vexx32:TypeInference/Catch_PSItem branch Oct 15, 2018

@lzybkr

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

Clever but simple, perfect. 👍👍👍

test/powershell/engine/Api/TypeInference.Tests.ps1 Outdated
$variableAst = { try {} catch { $_ } }.Ast.Find({ param($a) $a -is [System.Management.Automation.Language.VariableExpressionAst] }, $true)
$res = [AstTypeInference]::InferTypeOf($variableAst)

$res.Count | Should -Be 1

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 16, 2018

Collaborator

$res | Should -HaveCount 1

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 16, 2018

Collaborator

Could you please address the comment?
Line 941 too.

This comment has been minimized.

Copy link
@vexx32

vexx32 Oct 16, 2018

Author Contributor

All done. There are a few tests for other things that should probably use this as well, but I think that might be better left to another PR specifically for test revisions.

test/powershell/engine/Api/TypeInference.Tests.ps1 Outdated
}

It 'Infers type of $_.Expression in typed catch block' {
$memberAst = { try {} catch [System.IO.FileNotFoundException] { $_.Exception } }.Ast.Find(

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 16, 2018

Collaborator

Please add more catch-es.

This comment has been minimized.

Copy link
@vexx32

vexx32 Oct 16, 2018

Author Contributor

Reformatted this for additional Test Cases with a variety of exception types.

Would you like me to add additional tests verifying that constructs like catch [type1], [type2] {} and subsequent catch blocks all work as expected?

Upon having this thought, I just went ahead and did it; figured it better be as robust as we can make it!

test/powershell/engine/Api/TypeInference.Tests.ps1 Outdated
It 'Infers type of $_.Expression in [<Type>] typed catch block' -TestCases $catchClauseTypes {
param([Type] $Type)

$memberAst = [scriptblock]::Create("try {} catch [$Type] { $_.Exception }").Ast.Find(

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 16, 2018

Collaborator

I meant:

"try {} catch [System.OverflowException] catch [System.TimeoutException] { $_.Exception }"

This comment has been minimized.

Copy link
@vexx32

vexx32 Oct 16, 2018

Author Contributor

See additional below tests. I suppose I only test catch [type] {$_.Exception} catch {$_.Exception} but if you want me to go through a couple stacks I'm game. :)

vexx32 added some commits Oct 13, 2018

Add test for System.Exception completion from un-typed catch block
Add dummy generic errorrecord class
for type inference

Add logic for inferring errorrecord/exception type

Add tests for typed catch block inference

Tidy Exception<E> class
Reformat constructor for StyleCop
Update generic type argument
Update tests.
(thanks @SeeminglyScience!)
Fix tests
Reformat test strings
Remove [type] specifier from params (not needed)

@vexx32 vexx32 force-pushed the vexx32:TypeInference/Catch_PSItem branch to b9eaea2 Oct 16, 2018

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

Appveyor caught something funky, don't think it was related. Rebased to restart it.

Other than that... everything looks like it works as it ought. 😄

@iSazonov iSazonov merged commit 33f2f0f into PowerShell:master Oct 18, 2018

8 checks passed

CodeFactor 1 issue fixed. 1 issue found.
Details
PowerShell-CI-linux #PR-8020-20181016.06 succeeded
Details
PowerShell-CI-macos #PR-8020-20181016.06 succeeded
Details
PowerShell-CI-spelling #PR-8020-20181016.06 succeeded
Details
PowerShell-CI-windows #PR-8020-20181016.06 succeeded
Details
WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details

@vexx32 vexx32 deleted the vexx32:TypeInference/Catch_PSItem branch Jan 22, 2019

@iSazonov iSazonov added the CL-Engine label Feb 1, 2019

adityapatwardhan pushed a commit to adityapatwardhan/PowerShell that referenced this pull request Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.