-
Notifications
You must be signed in to change notification settings - Fork 8k
Add type inference for functions without OutputType attribute and anonymous functions #21127
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 functions without OutputType attribute and anonymous functions #21127
Conversation
|
I've got 2 test failures that I'm not sure how to handle. -Edit: Fixed my own test by moving the function definition outside the member invocation because the ExportVisitor doesn't visit child nodes of member invocations. |
|
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
|
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
|
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
|
@MartinGC94 Please resolve merge conflicts and rebase. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
As for failed test. I see OutputType attribute in Get-Process hasn't parameter sets. So I guess with the change we take all types from the attribute instead of doing type inference based on real objects. |
|
I figured out a way: I can just hide it behind |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| # Invoke-Expression is used to "hide" Get-Process from the type inference. | ||
| # If the typeinference code is updated to handle Invoke-Expression, this test will need to find some other way to set $p | ||
| # so that the type inference can't figure it out without evaluating the variable value | ||
| $p = Invoke-Expression -Command 'Get-Process' |
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.
Can we use the pattern
PowerShell/test/powershell/engine/Api/TypeInference.Tests.ps1
Lines 1569 to 1575 in 8733c17
| Describe "AstTypeInference tests" -Tags CI { | |
| It "Infers type from integer with passed in powershell instance" { | |
| $powerShell = [PowerShell]::Create([RunspaceMode]::CurrentRunspace) | |
| $res = [AstTypeInference]::InferTypeOf( { 1 }.Ast, $powerShell) | |
| $res.Count | Should -Be 1 | |
| $res.Name | Should -Be 'System.Int32' | |
| } |
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.
No because the type inference based on the variable runtime value is a last resort: https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/parser/TypeInferenceVisitor.cs#L2485
The regular type inference will still see the variable being assigned by the command Get-Process and hence, use the output type defined there.
I think the only way to make the test fully robust is to make a test specific Get-Process2 cmdlet without output types defined so that the type inference has no way of finding out the value without the SafeEval.
|
@MartinGC94 Please resolve merge conflicts. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
📣 Hey @MartinGC94, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
PR Summary
This adds type inference for functions where the
OutputTypeattribute hasn't been filled out and for anonymous functions.This allows tab completion to work like you would expect in situations like:
PR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).