Add type inference for Select-Object command #7171

powercode · 2018-06-25T19:51:39Z

PR Summary

Add type inference for Select-Object command

Fixes #7170

PR Checklist

[PR has a meaningful title]
[Summarized changes]
This change add type inference for Select-Object in the cases when no projection is done.
I.E. when neither -Property or -ExcludeProperty is specified.
[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]
User-facing changes
- Not Applicable
- 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.

iSazonov · 2018-06-26T11:06:08Z

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

+
            return expression != null &&
-                   SafeExprEvaluator.TrySafeEval(expression, ExecutionContext, out value) &&
+                   SafeExprEvaluator.TrySafeEval(expression, ExecutionContext, out var value) &&


The PR contains so many style changes that makes it unreasonably large.
Let's leave only significant changes - it will greatly facilitate and speed up the code viewing. Please move style changes in another PR.

BrucePay

Over all it looks good. I'm still a bit curious about the change from IEnumerable -> List. Also, i think the comments that talk about 'yield' need to be updated.

BrucePay · 2018-06-26T17:33:53Z

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

-                var list = value as IList;
-                if (list != null && list.Count > 0)
+                if (value is IList list && list.Count > 0)
                {


Extra line?

BrucePay · 2018-06-26T17:35:55Z

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

            var psMethod = member as PSMethod;
-            var methodCacheEntry = psMethod?.adapterData as DotNetAdapter.MethodCacheEntry;
-            return methodCacheEntry != null && methodCacheEntry.methodInformationStructures[0].method.IsConstructor;
+            return psMethod?.adapterData is DotNetAdapter.MethodCacheEntry methodCacheEntry && methodCacheEntry.methodInformationStructures[0].method.IsConstructor;


There is so much going on in this statement that's hard to read. The original code was much easier to follow.

Agree! A bit to fast there :)

BrucePay · 2018-06-26T17:45:02Z

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


-        private IEnumerable<PSTypeName> InferTypesFrom(CommandAst commandAst)
+        private void InferTypesFrom(CommandAst commandAst, List<PSTypeName> inferredTypes)
        {


You should validate that this input is not null.

BrucePay · 2018-06-26T17:56:44Z

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

-                }
-                yield break; // yield break;
+                // Get-CimInstance/New-CimInstance - yields a CimInstance with ETS type based on its arguments for -Namespace and -ClassName parameters
+                InferTypesFromCimCommand(pseudoBinding, inferredTypes);


Since this is no longer and IEnumerable, saying "yield" is a bit weird. Perhaps

Get-CimInstance/New-CimInstance - return a CimInstance with ETS type based on its arguments for -Namespace and -ClassName parameters

BrucePay · 2018-06-26T18:01:07Z

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

+            if (pseudoBinding.BoundArguments.TryGetValue("Property", out _)
+               || pseudoBinding.BoundArguments.TryGetValue("ExcludeProperty", out _))
+            {
+                return;


If the argument to -Property is a set of literal strings (most common case), then you could infer the "type" of the output object from them.

Yes. I discussed this a bit with Jason.
We don't currently have a good representation for this, but I would like to have that.
Same thing when you create a PSCustomObject from a hashtable.

But maybe something like "PSObject#Prop1/Prop2/Prop3"

It wouldn't say anything about the types of the properties, but that could maybe be encoded as well.

BrucePay · 2018-06-26T18:07:43Z

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


-        private IEnumerable<PSTypeName> InferTypesFrom(CommandAst commandAst)
+        private void InferTypesFrom(CommandAst commandAst, List<PSTypeName> inferredTypes)
        {


Why the change from IEnumerable to List?

I don't understand too why we add the allocations.

Mostly debugability. It was a nightmare with all the nested state machines.

But since you have an allocation and virtual dispatch on every generated statemachine class that is generated when we use yield, my guess is that we actually have fewer allocations now.

And in general, I think the code got simpler. Several instances of loops that just yielded the loop object that just disappeared.

If you want to do the exercise, try debugging a tab completion before and after the change.

Yes, debugging can be a headache here.

As for allocations my understanding is that it depends on how we access the object - if we use IEnumerable interface we allocate in both cases, if we cast to a collection we don't allocate I believe.

But there is no underlying collection to cast to. All the methods here that yield only has the generated state machine classes.

powercode · 2018-06-27T17:44:29Z

I don't think the Travis build failure has anything to do with this PR.

iSazonov · 2018-06-28T05:25:18Z

@powercode Please look CodeFactor issues.

iSazonov · 2018-06-29T04:34:11Z

@powercode As for CodeFactor issues.
Many thanks that you fix ALL issues!!!

Really our policy is flexible:

we don't ask fix all issues - only ones in new and changed code
if you is very busy you can ask maintainers to skip CodeFactor issues - we'll fix them ourselves later.
if you do many changes it is better fix style issues in separate PR or add first separate commit with style changes.

Thanks again for your contribution!

iSazonov · 2018-06-29T04:36:37Z

Restart CI Travis Linux.

anmenaga · 2018-06-29T18:11:56Z

@TravisEz13 this looks ready; pls take a look when you can.

powercode added 2 commits June 25, 2018 21:28

Adding type inference for Select-Object

9ef2974

Select-Object type inference tests

ed49d5e

powercode requested review from BrucePay and daxian-dbw as code owners June 25, 2018 19:51

powercode force-pushed the selectinfer branch from 445c58c to 8ad7106 Compare June 25, 2018 21:55

powercode added 2 commits June 26, 2018 00:22

Refactoring TypeInference for CommandAst to use explicit list of infe…

6aef1c3

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

Adding StyleCop.Cache to .gitignore

99aeb51

powercode force-pushed the selectinfer branch from 8ad7106 to 99aeb51 Compare June 25, 2018 22:23

Removing most Linq usage from TypeInferenceVisitor.

e4d1afd

iSazonov reviewed Jun 26, 2018

View reviewed changes

BrucePay suggested changes Jun 26, 2018

View reviewed changes

Addressing review comments

b457837

BrucePay approved these changes Jun 27, 2018

View reviewed changes

Addressing Code Factor issues

0b140d0

iSazonov assigned TravisEz13 Jun 29, 2018

TravisEz13 merged commit 3c079b9 into PowerShell:master Jul 2, 2018

Add type inference for Select-Object command #7171

Add type inference for Select-Object command #7171

Uh oh!

Conversation

powercode commented Jun 25, 2018

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrucePay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

powercode commented Jun 27, 2018

Uh oh!

iSazonov commented Jun 28, 2018

Uh oh!

iSazonov commented Jun 29, 2018

Uh oh!

iSazonov commented Jun 29, 2018

Uh oh!

anmenaga commented Jun 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants