Adding support for Typeinference based on runtime variable values #2744

powercode · 2016-11-20T22:40:04Z

Adding TypeInferenceVisitor (transforming GetInferredType method of corresponding types)
Removing Ast.GetInferredType hierarchy of virtual methods
Adding calls to SafeExpressEval when TypeInference is used in tabexpansion to allow
use of runtime variables when inferring type.

msftclas · 2016-11-20T22:40:09Z

Hi @powercode, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

lzybkr · 2016-11-21T03:45:12Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs

Please avoid formatting changes in a PR with meaningful code changes - it makes reviewing the code differences harder.

powercode · 2016-11-21T06:28:12Z

@lzybkr I'm between a rock and a hard place here, having to explain the new push with either arrogance or incompetence :) It was the latter. I heard you the first time, but missed how I had Beyond compare configured to ignore whitespace.
Fixed now

vors · 2016-12-01T21:55:05Z

@powercode can you, please, rebase and resolve the conflicts?

vors · 2016-12-01T22:15:59Z

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

This replacement is reasonable from the commits history point of view, but in the final diff, I don't see why do we want to remove one class/file and replace it with another one with the similar duties. Maybe we should keep the same name, so the caller doesn't need to change?

It will also help, if PowerShell team will decide to bring some changes back to windows, for that see https://github.com/PowerShell/PowerShell/blob/master/docs/dev-process/map-json-files.md

vors · 2016-12-01T22:33:22Z

src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs

The signature suggests that it should be some sort of a copy-ctor, but it's not.

vors · 2016-12-01T22:38:13Z

src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs

vors · 2016-12-01T22:38:24Z

src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs

vors · 2016-12-01T22:39:26Z

src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs

Should we remove this line as well? Now it's assigned in two places: here and in the CreateCompletionContext

indeed we should :)

vors · 2016-12-01T22:46:58Z

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

Adding public API in System.Management.Automation require a lot of thoughts.
I don't immediately see valuable external use-cases for types in this file. If you agree, please make them internal.

I think I would like you to have this discussion with @lzybkr. I have to admit that I don't have a clear view regarding what should be made public and what shouldn't.
It is clearly easier to make internal api:s public later on, then to go the other way.

@lzybkr ?

Following other visitors (i.e.SemanticChecks, ) I think we should start with internal.

The goal here is to end up with a public type inference API. Script analyzer is one obvious tool that would benefit (in fact they have their own implementation, and a shared implementation would be better I think.)

vors · 2016-12-01T22:50:58Z

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

Please, prefer overloads over optional parameters for public APIs. The reason is how C# compiler generates the code for the caller: it requires re-compilation of the caller, if the default parameter changed.
It's not a problem for internal APIs, but it is for public.

This note is applicable, only if we agreed to keep this type public.

vors · 2016-12-01T23:27:46Z

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

This method copy-pasted from src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs, why?

Forgot I had done that. I was thinking about dependencies. I think it's ok for Completion to have a dependency on TypeInference, but not the other way. So I think I had the idea to move it to TypeInference, and call it from Completion, but never got around to it.

vors · 2016-12-01T23:30:26Z

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

Currently there is no CIM on Unix. Should we add conditional compilation?

vors · 2016-12-01T23:32:17Z

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

This method is too big, please break it down

vors · 2016-12-01T23:33:41Z

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

This method is too big

vors · 2016-12-01T23:34:27Z

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

This method is too big

vors · 2016-12-03T03:17:29Z

Waiting on addressing the code-review comments

vors · 2016-12-21T15:44:29Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs

This is internal method. Should we just remove it completely?

vors · 2016-12-21T15:45:37Z

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

New line at the end of file to make git happy

Now that we have code coverage data, it would be great to get some great coverage on this new file.

lzybkr · 2016-12-21T20:04:39Z

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

The goal here is to end up with a public type inference API. Script analyzer is one obvious tool that would benefit (in fact they have their own implementation, and a shared implementation would be better I think.)

lzybkr · 2016-12-21T20:15:19Z

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

We should avoid Enum.HasFlag, it's much, much more expensive than a simple bitwise test.

See the implementation here: https://github.com/dotnet/coreclr/blob/e67851210d1c03d730a3bc97a87e8a6713bbf772/src/vm/reflectioninvocation.cpp#L3699

lzybkr · 2016-12-21T20:47:19Z

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

Some comments on what this is all about would be nice, right? (It's probably my fault, so hopefully I remember correctly).

The OutputType on cmdlets like Get-ChildItem may depend on the path. The CmdletInfo returned based on just the command name will specify returning all possibilities, e.g. certificates, environment, registry, etc.

If you specified -Path, the list of OutputType can be refined, but we have to make a copy of the CmdletInfo object this way to get that refinement.

lzybkr · 2016-12-21T20:49:57Z

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

Now that we have code coverage data, it would be great to get some great coverage on this new file.

powercode · 2016-12-31T01:57:55Z

If I understand correctly, we still have to

determine a good name for PowerShellExecutionHelper
decide if the public API is good enough or what changes should be made to it.
anything else?

powercode · 2017-04-28T19:00:41Z

@daxian-dbw Good suggestions! Have a look and see if I've understood you correctly.

daxian-dbw · 2017-04-28T06:00:49Z

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

+    /// <summary>
+    /// Enum describing permissions to use runtime evaluation during type inference
+    /// </summary>
+    [Flags]


Is it necessary to declare the Flag attribute?

No, it should not

daxian-dbw · 2017-04-28T06:03:27Z

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

+        /// <returns></returns>
+        public static IList<PSTypeName> InferTypeOf(Ast ast, PowerShell powerShell)
+        {
+            return InferTypeOf(ast, TypeInferenceRuntimePermissions.None);


This is wrong. The passed in powershell argument is not used.

daxian-dbw · 2017-04-28T06:07:08Z

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

+        /// <param name="powerShell">the instance of powershell to user for expression evalutaion needed for type inference</param>
+        /// <param name="evalPersmissions">The runtime usage permissions allowed during type inference</param>
+        /// <returns></returns>
+        public static IList<PSTypeName> InferTypeOf(Ast ast, PowerShell powerShell,TypeInferenceRuntimePermissions evalPersmissions)


PowerShell powerShell,TypeInferenceRuntimePermissions

Need a space between powerShell,TypeInferenceRuntimePermissions

internal ExecutionContext ExecutionContext => _powerShell.Runspace.ExecutionContext;

A rare case probably: if the powershell instance is associated with a RusnpacePool, then _powerShell.Runspace will be null and the above expression will result in a NullReferenceException

Changing to _powerShell.Runspace?.ExecutionContext;
Callers will have to check.

Better add comments in the constructors to call it out, and use an assert to validate in debug build.

Do you expect the passed in powershell instance to associate with a remote runspace or a runspace pool? For the usage of it in our tab completion code, it will always be created from powershell.Create(CurrentRunspace), but I'm having trouble to figure out the scenarios it will be used outside tab completion code.

@lzybkr has hopes that it will be used by Script Analyzer too.

daxian-dbw · 2017-04-28T06:11:12Z

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

+            }
+        }
+
+        struct ContextPermissionScope : IDisposable


Why do we need this data structure? It seems it can be replaced by a try-finally block.

Restructuring the code instead and removing it. Good point!

daxian-dbw · 2017-04-28T16:29:31Z

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

+        }
+        public TypeDefinitionAst CurrentTypeDefinitionAst { get; set; }
+
+        public TypeInferenceRuntimePermissions RuntimePermissions { get; set; }


It's more readable if all property declarations can be put right after constructors, so it's easy to find out what type-wide states an instance of such a type has.

daxian-dbw · 2017-04-28T18:03:49Z

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

+                if (filterToCall == null)
+                    filterToCall = o => !IsMemberHidden(o);
+                else
+                    filterToCall = o => !IsMemberHidden(o) && filter(o);


👍 to take into account the hidden members

daxian-dbw · 2017-04-28T18:13:57Z

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

+            else
+            {
+                elementTypes = typename.Type.GetInterfaces().Where(
+                    t => t.GetTypeInfo().IsGenericType && t.GetGenericTypeDefinition() == typeof(IEnumerable<>));


Ditto. About LINQ usage.

daxian-dbw · 2017-04-28T18:19:12Z

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

+                }
+            }
+            IEnumerable<Type> elementTypes;
+            if (typename.Type.IsArray)


Why do we want to include the members of the element type? I assume the following will happen in this case:

[List[string]] $a = [List[string]]::new() $a.C<tab> --> $a.Chars

If that's the case, isn't it confusing? I think element type should be included only in piping scenarios $a | % <tab>.

Maybe it is the naming that is bad. The whole scenario seems to be to get the type from enumerables and arrays

daxian-dbw · 2017-04-28T18:28:41Z

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

+            return (IEnumerable<PSTypeName>)res;
+        }
+
+        internal static bool TryGetRepresentativeTypeNameFromValue(object value, out PSTypeName type)


Can this method be made private? I don't think it's used anywhere outside this type.

daxian-dbw · 2017-04-28T19:04:52Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs

            if (typename.Type != null)
            {
-                if (context.CurrentTypeDefinitionAst == null || context.CurrentTypeDefinitionAst.Type != typename.Type)
+                if (typeInferenceContext.CurrentTypeDefinitionAst == null || typeInferenceContext.CurrentTypeDefinitionAst.Type != typename.Type)


It looks a lot code is duplicated between here and TypeInferenceContext.cs. Duplicate code should be refactored and removed, especially the complex ones like this -- it will be hard to maintain them at 2 places.

daxian-dbw · 2017-04-28T19:14:07Z

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

+                var baseTypeName = baseType.TypeName as TypeName;
+                if (baseTypeName == null) continue;
+                var baseTypeDefinitionAst = baseTypeName._typeDefinitionAst;
+                results.AddRange(GetMembersByInferredType(new PSTypeName(baseTypeDefinitionAst), isStatic, filterToCall));


class foo : Hashtable {}
In this case, baseType._typeDefinitionAst is null, and thus new PSTypeName(baseTypeDefinitionAst) will throw.

daxian-dbw · 2017-04-28T19:31:13Z

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

+        internal IEnumerable<PSTypeName> InferType(Ast ast, TypeInferenceVisitor visitor)
+        {
+            var res = ast.Accept(visitor);
+            if (res == null)


Just curious, in what case will res be null?

I don't think it ever will. This is a case where I wish I could express non-nullable reference types in the type system, but since the signature is an object, I check it. Maybe replace with an assert?

I don't think it ever will.

That's my feeling by looking at the code :) If that's the case, I prefer to an assert.

Where possible, I try to return some sentinel like an empty static array instead of null - when you do that consistently, you can end up with simpler code that doesn't need to test for null.

daxian-dbw · 2017-04-28T19:42:05Z

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

+        public bool TryGetRepresentativeTypeNameFromExpressionSafeEval(ExpressionAst expression, out PSTypeName typeName)
+        {
+            typeName = null;
+            if ((RuntimePermissions & TypeInferenceRuntimePermissions.AllowSafeEval) != TypeInferenceRuntimePermissions.AllowSafeEval)


If the Flag attribute is not necessary, then it's more readable to change this line to

RuntimePermissions != TypeInferenceRuntimePermissions.AllowSafeEval

daxian-dbw · 2017-04-28T19:45:36Z

@lzybkr LINQ is used in many places in the changes, some are new and some are from existing code.
Tab completion code is perf-sensitive, so should we always disallow LINQ usages? What would be the guideline?

For precisely, I'm not talking about LINQ query expressions, but usages like X.Where(predicate), X.SelectMany(..). They do create additional objects. I wonder what is the guideline for those usages.

Adding an overload on AddCommandWithPreferenceSetting for PowerShellExecutionHelper. Updating usages.

powercode · 2017-04-28T21:00:41Z

I discussed this with @lzybkr and @vors on gitter. I wanted to use a preallocated list that was filled in by the visitor. Mostly for ease of debugging, but also for performance. My impression was that Sergei liked the readablilty of linq and Jason was slightly in favor of using the list.

I'm open to both. It would be nice to see some performance data on the existing code to see if it is actually needed. The data sets are seldom large here.
Or maybe they are when analyzing scripts?

powercode · 2017-04-28T21:01:50Z

By the way, @daxian-dbw : what a great review! Working through the issues.

lzybkr · 2017-04-28T21:05:49Z

When LINQ gets used inappropriately - you end up with many allocations, and that affects GC at some point, possibly during your scenario, possibly not.

This is unrelated to yield vs. a list.

powercode · 2017-04-28T21:29:28Z

I have chosen to not make too many changes since the main job of this PR was to enable a new feature.
I tried to keep the code fairly similar to the original to make it easier to see my changes but that is soon a moot point :)
Maybe things like Linq usage can be a separate PR?

powercode · 2017-04-28T22:42:22Z

I believe I have addressed all comments except for LINQ usage.

daxian-dbw · 2017-04-28T23:38:41Z

The impact of LINQ usage doesn't show up immediately. Initially, the perf difference won't be measurable, but as more LINQ usages put in the code gradually, the perf diff will show up. By that time, it would be difficult to refactor the code to replace those uses.
So I vote for minimizing LINQ uses in perf-sensitive codes like Parser, Compiler and Auto-Completion.

Maybe things like Linq usage can be a separate PR?

I agree to make the LINQ usage changes a separate PR.

lzybkr · 2017-05-22T06:02:53Z

test/powershell/engine/Api/TypeInference.Tests.ps1

+
+    }
+
+    It "Inferes type from integer" {


Inferes [](start = 8, length = 7)

Infers - I could overlook this once, but it's copied many times in this file.

lzybkr · 2017-05-22T06:06:38Z

test/powershell/engine/Api/TypeInference.Tests.ps1

+    It "Inferes type from array IndexExpresssion" {
+        $res = [AstTypeInference]::InferTypeOf( { (1, 2, 3)[0] }.Ast)
+        $res.Count | Should Be 1
+        $res.Name | Should be 'System.object'


'System.object' [](start = 30, length = 15)

Hopefully there is a todo somewhere so this result comes out as System.Int32.

Do you expect me to fix this in PR?
I don't have a todo for this. I just tried to capture the current general behavior of arrays in a test.

Then there are other cases too:

(<expr>)[0] (<expr>, <expr2>)[0..1] (<expr>, <expr2>)[0,"1"]

Do we have any nice existing facilities to get the values of an index's target expression?
I guess we want to convert it to an int[], and use the indexes to get the elements of the target (in case it is an arrayliteral)? And return the types inferred from those of the elements that are in range?

It would be nice if you could at least open an issue to track the areas you know we're not currently wrong, but can somewhat easily do better.

powercode · 2017-05-24T20:46:21Z

Opened #3857 to track improvement of type inference for IndexExpresssions. @lzybkr

powercode · 2017-05-24T20:51:16Z

Opened #3858 to track the removal of LINQ from TypeInferenceVisitor. @daxian-dbw

vors · 2017-06-03T03:45:53Z

My impression was that Sergei liked the readablilty of linq and Jason was slightly in favor of using the list.

Not really: I was in favor of stateless passing everything around as parameters, linq was not discussed.

iSazonov · 2017-06-08T05:31:19Z

@powercode Thanks for the great work!

msftclas added the cla-already-signed label Nov 20, 2016

powercode force-pushed the typeinference-visitor branch 2 times, most recently from 16b4aa2 to 56891e4 Compare November 20, 2016 23:25

lzybkr reviewed Nov 21, 2016

View reviewed changes

powercode force-pushed the typeinference-visitor branch from 56891e4 to 91501b3 Compare November 21, 2016 06:21

PowerShellTeam added the Review - Needed label Nov 21, 2016

HemantMahawar removed the Review-Needed label Dec 1, 2016

vors self-assigned this Dec 1, 2016

vors reviewed Dec 1, 2016

View reviewed changes

vors previously requested changes Dec 1, 2016

View reviewed changes

vors added Review - Waiting on Author and removed Review - Needed The PR is being reviewed labels Dec 1, 2016

powercode force-pushed the typeinference-visitor branch from 91501b3 to 985e42c Compare December 1, 2016 23:40

PowerShellTeam added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Dec 2, 2016

vors added Review - Waiting on Author and removed Review - Needed The PR is being reviewed labels Dec 3, 2016

PowerShellTeam added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Dec 5, 2016

powercode force-pushed the typeinference-visitor branch 3 times, most recently from b795b00 to ade399c Compare December 6, 2016 21:12

vors reviewed Dec 21, 2016

View reviewed changes

lzybkr reviewed Dec 21, 2016

View reviewed changes

daxian-dbw reviewed Apr 28, 2017

View reviewed changes

powershell -> use context.Helper with AddCommandWithPreferenceSetting

d41beea

Adding an overload on AddCommandWithPreferenceSetting for PowerShellExecutionHelper. Updating usages.

Removing trailing whitespace

f5c4ade

Adding -Force to Update-TypeData

7b453e6

Staffan Gustafsson added 3 commits April 29, 2017 00:28

Addressing review comments

a41894e

Removing code duplication

3e4b58f

GetMembersByInferredType -> TypeInferenceContext instance

f72ccce

lzybkr reviewed May 22, 2017

View reviewed changes

Correct bad spelling of 'Infers' in tests

fabe8ea

powercode force-pushed the typeinference-visitor branch from e84b326 to fabe8ea Compare May 22, 2017 21:37

powercode mentioned this pull request May 24, 2017

Reduce usage of Linq in type inference code #3858

Closed

lzybkr approved these changes Jun 7, 2017

View reviewed changes

lzybkr merged commit a52adcd into PowerShell:master Jun 7, 2017

powercode deleted the typeinference-visitor branch January 26, 2018 19:04

Adding support for Typeinference based on runtime variable values #2744

Adding support for Typeinference based on runtime variable values #2744

Uh oh!

Conversation

powercode commented Nov 20, 2016

Uh oh!

msftclas commented Nov 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

powercode commented Nov 21, 2016

Uh oh!

vors commented Dec 1, 2016

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!

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!

vors commented Dec 3, 2016

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 Dec 31, 2016

Uh oh!

powercode commented Apr 28, 2017

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!

daxian-dbw Apr 28, 2017 •

edited

Loading