Type inference for Select-Object, Group-Object, PSObject and Hashtable by powercode · Pull Request #7231 · PowerShell/PowerShell

powercode · 2018-07-03T22:31:00Z

PR Summary

Adding the concept of a PSSyntheticTypeName, derived from PSTypeName,
that extends it with a list of synthetic members.

This allows us to express the inferred type of

[pscustomobject] @{
   A = 1
   B = "2"
}

as a "PSObject#A:B" and with information about the types of A and B.

This is also used to annotate the output of

Select-Object -Property
Select-Object -ExcludeProperty
Select-Object -ExpandProperty

Finally, it adds information about the types of the
Group and Value properties of the output of Group-Object

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
  - Issue filed - Issue link:
Testing - New and feature
- 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]

…rred types This reduces allocations and makes the code easier to debug.

powercode · 2018-07-04T09:02:41Z

I don't intend to fix the remaining CodeFactor issues.

Adding the concept of a PSSyntheticTypeName, derived from PSTypeName, that extends it with a list of synthetic members. This allows us to express the inferred type of [pscustomobject] @{ A = 1 B = "2" } as a "PSObject#A:B" and with information about the types of A and B. This is also used to annotate the output of Select-Object -Property Select-Object -ExcludeProperty Select-Object -ExpandProperty Finally, it adds information about the types of the Group and Value properties of the output of Group-Object

powercode · 2018-07-24T07:00:58Z

Ping

daxian-dbw · 2018-08-09T18:43:44Z

Sorry for delaying so long. Will review in a day or two.

TravisEz13 · 2018-08-20T19:20:45Z

@powercode, @daxian-dbw is on vacation

daxian-dbw

Sorry for being late on the code review. Left a few comments.

src/System.Management.Automation/engine/parser/TypeInferenceVisitor.cs

powercode · 2018-08-31T23:37:35Z

@daxian-dbw Thanks for your review! Very constructive as usual!

daxian-dbw

LGTM. Left two non-blocking comments.
@powercode Thanks for the great contribution, again!

src/System.Management.Automation/engine/parser/TypeInferenceVisitor.cs

daxian-dbw

Thanks for addressing the non-blocking comments!

BrucePay

There are a few little things that we can change later. Otherwise it looks OK to me.

BrucePay · 2018-07-09T18:37:36Z

src/System.Management.Automation/engine/CommandInfo.cs

+            }
+        }
+
+        private static bool IsPSTypeName(PSMemberNameAndType member) => string.Compare("PSTypeName", member.Name, StringComparison.CurrentCultureIgnoreCase) == 0;


Why not use string.Equals() here? It's faster than string.Compare().

BrucePay · 2018-07-09T18:46:13Z

src/System.Management.Automation/engine/parser/TypeInferenceVisitor.cs

+
+                foreach (var propertyName in properties)
+                {
+                    if (string.Compare(name, propertyName, StringComparison.CurrentCultureIgnoreCase) == 0)


Again - should use string.Equals()

BrucePay · 2018-09-13T17:28:38Z

src/System.Management.Automation/engine/CommandInfo.cs

+        /// </summary>
+        /// <param name="name">The name of the type.</param>
+        /// <param name="type">The real type.</param>
+        internal PSTypeName(string name, Type type)


This looks like it should be public but we can do that in a future PR.

BrucePay · 2018-09-13T17:35:42Z

src/System.Management.Automation/engine/parser/TypeInferenceVisitor.cs

-                return;
+                if (astParameterArgumentPair is AstPair astPair)
+                {
+                    object ToWildCardOrString(string value) => WildcardPattern.ContainsWildcardCharacters(value) ? (object)new WildcardPattern(value) : value;


Consider using PSPropertyExpression - it already handles wildcards in property names.

Maybe even use the ParameterProcessor? So we can handle hashtable arguments also?
I'll leave that to a future PR.

TravisEz13 · 2018-09-13T19:39:46Z

Add Committee review for new public interface

SteveL-MSFT · 2018-09-19T22:30:06Z

@PowerShell/powershell-committee reviewed the public constructor for PSTypeName and likes the proposal

Refactoring TypeInference for CommandAst to use explicit list of infe…

e3f639f

…rred types This reduces allocations and makes the code easier to debug.

powercode requested review from BrucePay and daxian-dbw as code owners July 3, 2018 22:31

powercode force-pushed the selectinfer branch 2 times, most recently from da09520 to b0f840e Compare July 3, 2018 23:42

iSazonov assigned TravisEz13 Jul 4, 2018

powercode force-pushed the selectinfer branch from b0f840e to 2a92db8 Compare July 4, 2018 20:22

daxian-dbw reviewed Aug 31, 2018

View reviewed changes

Addressing Dongbo's comments.

7fdd758

powercode force-pushed the selectinfer branch from 0ccc88e to 7fdd758 Compare August 31, 2018 23:37

daxian-dbw approved these changes Aug 31, 2018

View reviewed changes

src/System.Management.Automation/engine/parser/TypeInferenceVisitor.cs Outdated Show resolved Hide resolved

src/System.Management.Automation/engine/parser/TypeInferenceVisitor.cs Show resolved Hide resolved

Addressing Dongo's comments, take 2.

ea21469

daxian-dbw approved these changes Sep 4, 2018

View reviewed changes

BrucePay approved these changes Sep 13, 2018

View reviewed changes

Addressing the simple parts of Bruce's comments.

8f2eedf

TravisEz13 added Review - Committee The PR/Issue needs a review from the PowerShell Committee WG-Engine core PowerShell engine, interpreter, and runtime labels Sep 13, 2018

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Sep 19, 2018

TravisEz13 merged commit eb28633 into PowerShell:master Sep 19, 2018

iSazonov mentioned this pull request Oct 1, 2018

Tab completion don't work for $error #3373

Closed

SeeminglyScience mentioned this pull request Jun 29, 2019

WIP Move to runspace synchronizer from completions PowerShell/PowerShellEditorServices#980

Closed

Conversation

powercode commented Jul 3, 2018 • edited by TravisEz13 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

powercode commented Jul 4, 2018

Uh oh!

powercode commented Jul 24, 2018

Uh oh!

daxian-dbw commented Aug 9, 2018

Uh oh!

TravisEz13 commented Aug 20, 2018

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

powercode commented Aug 31, 2018

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

BrucePay left a comment

Choose a reason for hiding this comment

Uh oh!

BrucePay Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

BrucePay Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

BrucePay Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

BrucePay Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

powercode Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented Sep 13, 2018

Uh oh!

SteveL-MSFT commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

powercode commented Jul 3, 2018 •

edited by TravisEz13

Loading

SteveL-MSFT commented Sep 19, 2018 •

edited

Loading