Skip to content
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

Adding support for Typeinference based on runtime variable values #2744

Merged
merged 19 commits into from Jun 7, 2017

Conversation

powercode
Copy link
Collaborator

fixes #2567

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
Copy link

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;

@powercode powercode force-pushed the typeinference-visitor branch 2 times, most recently from 16b4aa2 to 56891e4 Compare November 20, 2016 23:25
@@ -29,7 +29,7 @@
namespace System.Management.Automation
{
/// <summary>
///
///
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@powercode
Copy link
Collaborator Author

@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
Copy link
Collaborator

vors commented Dec 1, 2016

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

@vors vors self-assigned this Dec 1, 2016
@@ -643,7 +643,7 @@ private static List<CompletionResult> InvokeLegacyTabExpansion(PowerShell powers
char quote;
var lastword = LastWordFinder.FindLastWord(legacyInput, out replacementIndex, out quote);
replacementLength = legacyInput.Length - replacementIndex;
var helper = new CompletionExecutionHelper(powershell);
var helper = new PowerShellExecutionHelper(powershell);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -96,8 +96,14 @@ private static bool IsCursorOutsideOfExtent(IScriptPosition cursor, IScriptExten
return cursor.Offset < extent.StartOffset || cursor.Offset > extent.EndOffset;
}

internal CompletionContext CreateCompletionContext(ExecutionContext executionContext)
internal CompletionContext CreateCompletionContext(CompletionContext completionContext)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

vors
vors previously requested changes Dec 1, 2016
@@ -134,7 +144,8 @@ internal CompletionContext CreateCompletionContext(ExecutionContext executionCon
Options = _options,
ExecutionContext = executionContext,
ReplacementIndex = adjustLineAndColumn ? _cursorPosition.Offset : 0,
CurrentTypeDefinitionAst = Ast.GetAncestorTypeDefinitionAst(asts.Last()),
TypeInferenceContext = typeInferenceContext,
Helper = typeInferenceContext.Helper,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -278,8 +289,7 @@ private static bool IsTokenTheSame(Token x, Token y)

internal List<CompletionResult> GetResults(PowerShell powerShell, out int replacementIndex, out int replacementLength)
{
var completionContext = CreateCompletionContext(powerShell.GetContextFromTLS());
completionContext.Helper = new CompletionExecutionHelper(powerShell);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -1167,7 +1177,7 @@ private List<CompletionResult> GetResultForString(CompletionContext completionCo
cursorIndexInString = strValue.Length;

var analysis = new CompletionAnalysis(_ast, _tokens, _cursorPosition, _options);
var subContext = analysis.CreateCompletionContext(completionContext.ExecutionContext);
var subContext = analysis.CreateCompletionContext(completionContext);
subContext.Helper = completionContext.Helper;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed we should :)

/// Enum describing permissions to use runtime evaluation during type inference
/// </summary>
[Flags]
public enum TypeInferenceRuntimePermissions {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lzybkr ?

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

/// <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 = TypeInferenceRuntimePermissions.None)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return results;
}

internal static PowerShell AddCommandWithPreferenceSetting(PowerShell powershell, string command, Type type = null)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return powershell;
}

private static bool ParseCimCommandsTypeName(PSTypeName typename, out string cimNamespace, out string className)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return TypeInferenceContext.EmptyPSTypeNameArray;
}

private IEnumerable<PSTypeName> InferTypesFrom(CommandAst commandAst)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is too big, please break it down

}
}

private IEnumerable<PSTypeName> InferTypesFrom(MemberExpressionAst memberExpressionAst)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is too big

}
}

private IEnumerable<PSTypeName> InferTypeFrom(VariableExpressionAst variableExpressionAst)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is too big

@vors vors added Review - Waiting on Author and removed Review - Needed The PR is being reviewed labels Dec 1, 2016
@PowerShellTeam PowerShellTeam added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Dec 2, 2016
@vors vors added Review - Waiting on Author and removed Review - Needed The PR is being reviewed labels Dec 3, 2016
@vors
Copy link
Collaborator

vors commented Dec 3, 2016

Waiting on addressing the code-review comments

@PowerShellTeam PowerShellTeam added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Dec 5, 2016
@powercode powercode force-pushed the typeinference-visitor branch 3 times, most recently from b795b00 to ade399c Compare December 6, 2016 21:12
@@ -6463,26 +6464,7 @@ private static List<CompletionResult> GetSpecialHashTableKeyMembers(params strin

internal static PowerShell AddCommandWithPreferenceSetting(PowerShell powershell, string command, Type type = null)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return dynamicKeywordAst.CommandElements[0].Accept(this);
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New line at the end of file to make git happy

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixing

/// Enum describing permissions to use runtime evaluation during type inference
/// </summary>
[Flags]
public enum TypeInferenceRuntimePermissions {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

public bool TryGetRepresentativeTypeNameFromExpressionSafeEval(ExpressionAst expression, out PSTypeName typeName)
{
typeName = null;
if (!RuntimePermissions.HasFlag(TypeInferenceRuntimePermissions.AllowSafeEval))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixing

var pathValue = ((StringConstantExpressionAst)pathArgumentPair.Argument).Value;
try
{
commandInfo = commandInfo.CreateGetCommandCopy(new object[] { "-" + pathParameterName, pathValue });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixing

return dynamicKeywordAst.CommandElements[0].Accept(this);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@powercode
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to declare the Flag attribute?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it should not

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check!

/// <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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PowerShell powerShell,TypeInferenceRuntimePermissions

Need a space between powerShell,TypeInferenceRuntimePermissions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

struct ContextPermissionScope : IDisposable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restructuring the code instead and removing it. Good point!

}
public TypeDefinitionAst CurrentTypeDefinitionAst { get; set; }

public TypeInferenceRuntimePermissions RuntimePermissions { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (filterToCall == null)
filterToCall = o => !IsMemberHidden(o);
else
filterToCall = o => !IsMemberHidden(o) && filter(o);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to take into account the hidden members

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto. About LINQ usage.

}
}
IEnumerable<Type> elementTypes;
if (typename.Type.IsArray)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return (IEnumerable<PSTypeName>)res;
}

internal static bool TryGetRepresentativeTypeNameFromValue(object value, out PSTypeName type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (typename.Type != null)
{
if (context.CurrentTypeDefinitionAst == null || context.CurrentTypeDefinitionAst.Type != typename.Type)
if (typeInferenceContext.CurrentTypeDefinitionAst == null || typeInferenceContext.CurrentTypeDefinitionAst.Type != typename.Type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, in what case will res be null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

RuntimePermissions != TypeInferenceRuntimePermissions.AllowSafeEval

@daxian-dbw
Copy link
Member

daxian-dbw commented Apr 28, 2017

@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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@lzybkr
Copy link
Member

lzybkr commented Apr 28, 2017

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
Copy link
Collaborator Author

powercode commented Apr 28, 2017

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
Copy link
Collaborator Author

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

@daxian-dbw
Copy link
Member

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.


}

It "Inferes type from integer" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

powercode commented May 24, 2017

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

@powercode
Copy link
Collaborator Author

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

@vors
Copy link
Collaborator

vors commented Jun 3, 2017

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.

@lzybkr lzybkr merged commit a52adcd into PowerShell:master Jun 7, 2017
@iSazonov
Copy link
Collaborator

iSazonov commented Jun 8, 2017

@powercode Thanks for the great work!

@powercode powercode deleted the typeinference-visitor branch January 26, 2018 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review - Needed The PR is being reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the type of a variable for tab expansion
8 participants