-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Adding support for Typeinference based on runtime variable values #2744
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
Conversation
Hi @powercode, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
16b4aa2
to
56891e4
Compare
@@ -29,7 +29,7 @@ | |||
namespace System.Management.Automation | |||
{ | |||
/// <summary> | |||
/// | |||
/// |
There was a problem hiding this comment.
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.
56891e4
to
91501b3
Compare
@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. |
@powercode can you, please, rebase and resolve the conflicts? |
@@ -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); |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
@@ -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, |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
91501b3
to
985e42c
Compare
Waiting on addressing the code-review comments |
b795b00
to
ade399c
Compare
@@ -6463,26 +6464,7 @@ private static List<CompletionResult> GetSpecialHashTableKeyMembers(params strin | |||
|
|||
internal static PowerShell AddCommandWithPreferenceSetting(PowerShell powershell, string command, Type type = null) |
There was a problem hiding this comment.
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); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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.
If I understand correctly, we still have to
|
@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] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; } |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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<>)); |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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>
.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
@lzybkr LINQ is used in many places in the changes, some are new and some are from existing code. For precisely, I'm not talking about LINQ query expressions, but usages like |
Adding an overload on AddCommandWithPreferenceSetting for PowerShellExecutionHelper. Updating usages.
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. |
By the way, @daxian-dbw : what a great review! Working through the issues. |
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 |
I have chosen to not make too many changes since the main job of this PR was to enable a new feature. |
I believe I have addressed all comments except for LINQ usage. |
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.
I agree to make the LINQ usage changes a separate PR. |
|
||
} | ||
|
||
It "Inferes type from integer" { |
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
e84b326
to
fabe8ea
Compare
Opened #3858 to track the removal of LINQ from TypeInferenceVisitor. @daxian-dbw |
Not really: I was in favor of stateless passing everything around as parameters, linq was not discussed. |
@powercode Thanks for the great work! |
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.