GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add Type Inference for $_ / $PSItem in catch{ } blocks by vexx32 · Pull Request #8020 · PowerShell/PowerShell

Conversation

@vexx32

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) commented Oct 13, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
  • User-facing changes
    • Not Applicable
    • OR
    • User-facing Documentation needed
      • Issue filed - Issue link:
  • Testing - New and feature
    • Not Applicable or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
      • Add [feature] if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SeeminglyScience, iSazonov, and powercode reacted with hooray emoji
All reactions
  • 🎉 3 reactions
@vexx32
Rain Sallow (/u/ta11ow) (vexx32) requested review from Bruce Payette (BrucePay) and Dongbo Wang (daxian-dbw) as code owners October 13, 2018 18:49
@iSazonov

Ilya (iSazonov) commented Oct 15, 2018

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov Ilya (iSazonov) self-assigned this Oct 15, 2018
@iSazonov
Ilya (iSazonov) requested a review from Jason Shirk (lzybkr) October 15, 2018 05:36
lzybkr
Jason Shirk (lzybkr) approved these changes Oct 15, 2018
View reviewed changes

@lzybkr Jason Shirk (lzybkr) left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Oct 15, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

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.

Jason Shirk (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Oct 15, 2018

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Please add test to demo that both cases work.
vexx32 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov
Ilya (iSazonov) requested review from Steve Lee (SteveL-MSFT) and Aditya Patwardhan (adityapatwardhan) October 15, 2018 15:33
@vexx32
Rain Sallow (/u/ta11ow) (vexx32) force-pushed the TypeInference/Catch_PSItem branch from 9c1c9c3 to 586aae3 Compare October 15, 2018 16:49
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Oct 15, 2018

Copy link
Copy Markdown
Collaborator Author

Jason Shirk (@lzybkr) Ilya (@iSazonov)

Added tests.

With Patrick Meinecke (@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! 😄
SeeminglyScience and lzybkr reacted with heart emoji
All reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vexx32
Rain Sallow (/u/ta11ow) (vexx32) force-pushed the TypeInference/Catch_PSItem branch from 0e24c59 to dd61d52 Compare October 15, 2018 17:08
@lzybkr

Jason Shirk (lzybkr) commented Oct 15, 2018

Copy link
Copy Markdown
Contributor

Clever but simple, perfect. 👍👍👍
SeeminglyScience and vexx32 reacted with hooray emoji SeeminglyScience and vexx32 reacted with heart emoji
All reactions
  • 🎉 2 reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov
Ilya (iSazonov) reviewed Oct 16, 2018
View reviewed changes
Comment thread test/powershell/engine/Api/TypeInference.Tests.ps1 Outdated

@iSazonov Ilya (iSazonov) Oct 16, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

$res | Should -HaveCount 1

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vexx32 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@iSazonov Ilya (iSazonov) Oct 16, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Could you please address the comment?
Line 941 too.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Oct 16, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/engine/Api/TypeInference.Tests.ps1 Outdated

@iSazonov Ilya (iSazonov) Oct 16, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please add more catch-es.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Oct 16, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
iSazonov
Ilya (iSazonov) reviewed Oct 16, 2018
View reviewed changes
Comment thread test/powershell/engine/Api/TypeInference.Tests.ps1 Outdated

@iSazonov Ilya (iSazonov) Oct 16, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I meant:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Oct 16, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Rain Sallow (/u/ta11ow) (vexx32) added 8 commits October 16, 2018 09:49
@vexx32
Fix tab completions
a9d4fa4
@vexx32
Add tests for type inference of $_ in catch blocks
0a4f93e
@vexx32
Add test for System.Exception completion from un-typed catch block
61af646 
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
@vexx32
Reformat constructor for StyleCop
ea2ebe2 
Update generic type argument
Update tests.
(thanks @SeeminglyScience!)
@vexx32
Add testcases to verify that type inference works with common exceptions
c35a78c
@vexx32
Reformat slightly for less confusing syntax
2fa0169
@vexx32
Add further tests just to be absolutely sure
1f0db69
@vexx32
Fix tests
b9eaea2 
Reformat test strings
Remove [type] specifier from params (not needed)
@vexx32
Rain Sallow (/u/ta11ow) (vexx32) force-pushed the TypeInference/Catch_PSItem branch from d09fb29 to b9eaea2 Compare October 16, 2018 13:50
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Oct 16, 2018

Copy link
Copy Markdown
Collaborator Author

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. 😄
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vexx32
Fix tests to use -HaveCount
589d471
iSazonov
Ilya (iSazonov) approved these changes Oct 16, 2018
View reviewed changes
@iSazonov
Ilya (iSazonov) merged commit 33f2f0f into PowerShell:master Oct 18, 2018
@vexx32
Rain Sallow (/u/ta11ow) (vexx32) deleted the TypeInference/Catch_PSItem branch January 22, 2019 13:20
@iSazonov Ilya (iSazonov) added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Feb 1, 2019
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@vexx32 @iSazonov
Add Type Inference for $_ / $PSItem in catch{ } blocks (PowerShell#8020)
2d38719
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov approved these changes
@BrucePay Bruce Payette (BrucePay) Awaiting requested review from BrucePay
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw daxian-dbw is a code owner
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Awaiting requested review from adityapatwardhan
@SteveL-MSFT Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
+1 more reviewer
@lzybkr Jason Shirk (lzybkr) lzybkr approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@iSazonov Ilya (iSazonov)

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Autocompletion gives no results for $PSItem / $_ in Catch blocks

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@vexx32 @iSazonov @lzybkr
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.