Improve type inference and completions

[MartinGC94]

PR Summary

Fixes #15811
Fixes #9949
Fixes #11803
Fixes #15885
Fixes #17257

Improves the type inference in the following scenarios:

1. Index access on an Ilist like [System.Collections.Generic.IList[System.Text.StringBuilder]]$null)[0]
2. "as" operator like ($var -as [ICloneable]) (Note it only works with type literals, it still doesn't work with other expression types)
3. $_ inside method invocations that use a scriptblock, like the magic foreach/where methods: (0..10).ForEach{$_}

Improves tab completion in the following ways:

1. Allow methods to be shown in completion results for ForEach-Object -MemberName
2. Prevents completion on expressions that return void like ([void]("")).
3. Allow non-default Class constructors to show up when class completion is based on the AST
4. Update the tooltip to show multiple overloads for methods when completing class methods from the AST

Fixes a failed assertion due to a missing null check.

PR Context

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• 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: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A 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
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
  • OR
  • I have considered the user experience from a tooling perspective and opened an issue in the relevant tool repository. This may include:
    • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode
      (which runs in a different PS Host).
      • Issue filed:
    • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
      • Issue filed:
    • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
      • Issue filed:
    • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).
      • Issue filed:

[MartinGC94]

The Cannot convert the "ns_70262529219d4bdfa518523ef9b4fae9.E_sbyte" value of type "System.String" to type "System.Type". error seems like a random error. It doesn't happen on my system when I test it.
The Tab expansion in constrained language mode.Verifies that tab expansion cannot convert disallowed IntPtr type test failure is related to my changes but only because it's relying on the type inference not working properly for "as" statements.
I don't know how to fix that test so it still tests against disallowed conversions in constrained language mode so some assistance would be nice.

[iSazonov]

It doesn't happen on my system when I test it.

If it is persistent on CI you can temporary add test code (in tests or Engine itself) :-)

[MartinGC94]

Looks like I was right about that test but now a different unrelated test is failing...

[iSazonov]

Looks like I was right about that test but now a different unrelated test is failing...

But the test doesn't fail on other PRs or daily builds.

[MartinGC94]

These are the 3 runs I've had on this PR:

1. https://dev.azure.com/powershell/PowerShell/_build/results?buildId=95571&view=ms.vss-test-web.build-test-results-tab
2. https://dev.azure.com/powershell/PowerShell/_build/results?buildId=95594&view=ms.vss-test-web.build-test-results-tab
3. https://dev.azure.com/powershell/PowerShell/_build/results?buildId=95659&view=ms.vss-test-web.build-test-results-tab

First run had the Enum out of range error due to bad RNG luck. The next 2 runs replaced that error with a different error Clean up open Runspaces when exit powershell process.PowerShell process should not freeze at exit. I've seen that error show up in some of my other PRs so I know it's unstable but even if we assume it's my code, why wouldn't it fail the first time?

The tabexpansion error that is there in all 3 runs is perfectly legit but like I said earlier it only exists because the test is designed around type inference not working. We need a different test to test if the completion code is correctly blocked from doing conversions.

[iSazonov]

Yes, I see the same errors in other PRs.

[ghost]

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

[daxian-dbw]

Sorry for the long delay. Changes look good to me overall. Left a few comments.

[adityapatwardhan]

@MartinGC94 Can you have a look at the failing test.

[MartinGC94]

@adityapatwardhan I would like some assistance with it. As far as I can tell, the test is failing because it thinks a conversion happened, when in reality it's just the type inference that is now working properly. I don't know how to build a new test that tests that conversion doesn't happen in constrained language mode.

[ghost]

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

[daxian-dbw]

LGTM.

[MartinGC94]

@daxian-dbw I have no idea. I can't reproduce that on my system:

-Edit: I misread what you wrote. (TabExpansion2 '(1234 -as [IntPtr]).' 20).CompletionMatches gives 2 different results in the current version because first it tries to convert the value and get the intptr results and when that fails in constrained mode it falls back to the broken type inference that uses the type on the left (in your example, an int32) instead of the type on the right. My fix makes it properly use the type specified on the right side of the -as expression.

[daxian-dbw]

@MartinGC94 Got it, thanks!
In that case, I think we can just remove this test, especially given that (TabExpansion2 '([IntPtr]1234).').CompletionMatches returns the same results in both full language and the constrained language.

[MartinGC94]

I'm not sure. (TabExpansion2 '([IntPtr]1234).').CompletionMatches gives the same result in both modes but it takes 2 different paths to do so. In full language mode it converts the value to get the results, in the constrained language mode the conversion fails and it falls back to the type inference just like -as now does.
The point of the test seems to be to ensure that no changes are introduced that somehow allows this illegal conversion to happen and if that's not covered elsewhere then surely we need a replacement test?

[daxian-dbw]

My point about (TabExpansion2 '([IntPtr]1234).').CompletionMatches is that this returns the members of IntPtr even though IntPtr is not a supported type in the constrained language mode, so it would be OK for (1234 -as [IntPtr]).<tab> to return the same, as long as the execution would fail as expected.

I'm not sure if there is a test to cover the disallowing-conversion-to-intptr scenario. It would be better to add a test to cover just that.

[MartinGC94]

Could reflection be used for the test?

    using namespace System.Reflection
    using namespace System.Management.Automation.Language
    $Tokens = $null
    $Errors = $null
    $BinaryAst = ([Parser]::ParseInput('(1234 -as [System.IntPtr])', [ref]$Tokens, [ref]$Errors)).Find(
        {param($Ast); $Ast -is [ParenExpressionAst]},
        $false
    )
    $RealExecutionContext = $ExecutionContext.GetType().GetField('_context', [BindingFlags]::NonPublic -bor [BindingFlags]::Instance).GetValue($ExecutionContext)
    $Discard = $null
    $Method = $ExecutionContext.GetType().Assembly.GetType('System.Management.Automation.SafeExprEvaluator').GetMethod('TrySafeEval', [BindingFlags]::NonPublic -bor [BindingFlags]::Static)
    $Method.Invoke(
        $null,
        ($BinaryAst, $RealExecutionContext, [ref] $Discard)
    )

$Method.Invoke will throw the expected error in constrained language mode.

[daxian-dbw]

It "Verifies that tab expansion cannot convert disallowed IntPtr type" {
      try
      {
          $ExecutionContext.SessionState.LanguageMode = "ConstrainedLanguage"
          $result = 1234 -as [IntPtr]
          $null -eq $result | Should -BeTrue
      }
      finally
      {
          Invoke-LanguageModeTestingSupportCmdlet -EnableFullLanguageMode
      }
}

Can't we replace it with this test?

[MartinGC94]

Maybe. It depends on what should be tested. Based on the description for the SafeExprEvaluator class

SafeExprEvaluator description

This class is very similar to the restricted language checker, but it is meant to allow more things, yet still be considered "safe", at least in the sense that tab completion can rely on it to not do bad things. The primary use is for intellisense where you don't want to run arbitrary code, but you do want to know the values of various expressions so you can get the members.

I would assume the goal of the test was to be sure SafeExprEvaluator didn't somehow bypass the constrained language restrictions, otherwise it doesn't make any sense why they would go with that tabexpansion test over a more obvious test like 1234 -as [IntPtr].

[daxian-dbw]

Does SafeExprEvaluator get involved in the code paths of both TabExpansion2 '(1234 -as [IntPtr]).' 20 and TabExpansion2 '([IntPtr]1234).'?
You mentioned "in the constrained language mode the conversion fails", so I assume it's SafeExprEvaluator that decides the conversion is not safe but want to confirm with you.

Besides, you cannot invoke the method $Method.Invoke(...) in constrained language mode, can you?

[MartinGC94]

Does SafeExprEvaluator get involved in the code paths of both TabExpansion2 '(1234 -as [IntPtr]).' 20 and TabExpansion2 '([IntPtr]1234).'?

Yes, see this: https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs#L5802 and this: https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs#L7392
Member completion always tries the TrySafeEval method and if that fails for any reason it falls back to the type inference.

so I assume it's SafeExprEvaluator that decides the conversion is not safe but want to confirm with you.

No, as far as I can tell it's the compiler, TrySafeEval simply calls Compiler.GetExpressionValue which throws in constrained mode. So I guess you are right that there's no point in testing SafeExprEvaluator separately?

Besides, you cannot invoke the method $Method.Invoke(...) in constrained language mode, can you?

You sort of can. If you are in full language mode when you are defining $Method and then change to constrained mode you can call the Invoke method without any issues but if you are in constrained language mode from the start then you won't even get a chance to assign a value to $Method.

GitHub

PowerShell/CompletionCompleters.cs at master · PowerShell/PowerShell

PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

GitHub

PowerShell/CompletionCompleters.cs at master · PowerShell/PowerShell

PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

[daxian-dbw]

No, as far as I can tell it's the compiler, TrySafeEval simply calls Compiler.GetExpressionValue which throws in constrained mode. So I guess you are right that there's no point in testing SafeExprEvaluator separately?

Yeah, Compiler.GetExpressionValue will go through the same code path as you run 1234 -as [IntPtr] directly. So #16963 (comment) should exercise the same code.

[MartinGC94]

Alright, I've updated the test. Thanks for the assist @daxian-dbw

[daxian-dbw]

LGTM

[pull-request-quantifier-deprecated]

This PR has 149 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

---

Quantification details

Label      : Medium
Size       : +110 -39
Percentile : 49.8%

Total files changed: 6

Change summary by file extension:
.cs : +75 -35
.ps1 : +35 -4

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

• Fast and predictable releases to production:
  • Optimal size changes are more likely to be reviewed faster with fewer
    iterations.
  • Similarity in low PR complexity drives similar review times.
• Review quality is likely higher as complexity is lower:
  • Bugs are more likely to be detected.
  • Code inconsistencies are more likely to be detetcted.
• Knowledge sharing is improved within the participants:
  • Small portions can be assimilated better.
• Better engineering practices are exercised:
  • Solving big problems by dividing them in well contained, smaller problems.
  • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

• Use the PullRequestQuantifier to quantify your PR accurately
  • Create a context profile for your repo using the context generator
  • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
  • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
  • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
• Change your engineering behaviors
  • For PRs that fall outside of the desired spectrum, review the details and check if:
    • Your PR could be split in smaller, self-contained PRs instead
    • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

• One line was added: +1 -0
• One line was deleted: +0 -1
• One line was modified: +1 -1 (git diff doesn't know about modified, it will
  interpret that line like one addition plus one deletion)
• Change percentiles: Change characteristics (addition, deletion, modification)
  of this PR in relation to all other PRs within the repository.

---

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

[adityapatwardhan]

@MartinGC94 Thank you for your contribution!

[ghost]

🎉v7.3.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes