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

Add support to experimental features #7242

Merged
merged 17 commits into from Jul 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
eed9842
First batch: Basic experimental feature flag and attributes enforced
daxian-dbw Jun 2, 2018
930350e
Update code that creates attributes to support the new attribute changes
daxian-dbw Jun 15, 2018
8a5ddbb
Support experimental feature in modules
daxian-dbw Jun 19, 2018
7770aa5
Add 'Get-ExperimentalFeature' cmdlet
daxian-dbw Jun 26, 2018
dc2d741
Add logging, formatting, and '$EnabledExperimentalFeatures'
daxian-dbw Jun 28, 2018
d8d3344
Generate logging resources for Unix platforms
daxian-dbw Jul 3, 2018
3c3ddeb
Alter the analysis cache file name properly
daxian-dbw Jul 3, 2018
d2e9151
[Feature] Add tests for experimental feature work
daxian-dbw Jul 6, 2018
bc8abf8
[Feature] Fix two tests
daxian-dbw Jul 6, 2018
2fce1bf
[Feature] Root caused test failure issue and fix it
daxian-dbw Jul 9, 2018
1033422
Address Feedback: functional changes and method/variable renaming
daxian-dbw Jul 10, 2018
b616cf5
[Feature] Address Feedback: style changes and fixing typo
daxian-dbw Jul 10, 2018
c7c35bd
[Feature] Address Feedback: make value tuple element types explicit
daxian-dbw Jul 10, 2018
2c9519b
[Feature] Address Feedback: Use better error message and add more tests
daxian-dbw Jul 10, 2018
0ac878f
[Feature] Address Feedback: Fix style and xml doc issues. Update tests
daxian-dbw Jul 10, 2018
add14e4
[Feature] Address Feedback: Do not override user defined cache path
daxian-dbw Jul 11, 2018
d12230f
[Feature] Address Feedback: Use value tuple (int, string) as the sort…
daxian-dbw Jul 12, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -98,6 +98,29 @@
value="0xD003"
version="1"
/>
<!--ExperimentalFeature event-->
<event
channel="C_OPERATIONAL"
level="win:Error"
message="$(string.PS_PROVIDER.event.E_O_ExperimentalFeatureInvalidName.message)"
opcode="Constructor"
symbol="ExperimentalFeatureInvalidName"
task="ExperimentalFeature"
template="T_ExperimentalFeatureInvalidName"
value="0x3001"
version="1"
/>
<event
channel="C_OPERATIONAL"
level="win:Error"
message="$(string.PS_PROVIDER.event.E_O_ExperimentalFeatureReadConfigError.message)"
opcode="Constructor"
symbol="ExperimentalFeatureReadConfigError"
task="ExperimentalFeature"
template="T_ExperimentalFeatureReadConfigError"
value="0x3002"
version="1"
/>
<!--M3P events-->
<event
channel="C_ANALYTIC"
Expand Down Expand Up @@ -2368,6 +2391,12 @@
symbol="T_EXECUTEPIEPLINE"
value="106"
/>
<task
message="$(string.PS_PROVIDER.task.T_ExperimentalFeature.message)"
name="ExperimentalFeature"
symbol="T_EXPERIMENTALFEATURE"
value="107"
/>
<task
message="$(string.PS_PROVIDER.task.T_ScheduledJob.message)"
name="ScheduledJob"
Expand Down Expand Up @@ -3927,6 +3956,30 @@
name="InnerException"
/>
</template>
<template tid="T_ExperimentalFeatureInvalidName">
<data
inType="win:UnicodeString"
name="Name"
/>
<data
inType="win:UnicodeString"
name="Message"
/>
</template>
<template tid="T_ExperimentalFeatureReadConfigError">
<data
inType="win:UnicodeString"
name="Name"
/>
<data
inType="win:UnicodeString"
name="Message"
/>
<data
inType="win:UnicodeString"
name="StackTrace"
/>
</template>
<template tid="T_TrackingGuid">
<data
inType="win:GUID"
Expand Down Expand Up @@ -5428,6 +5481,18 @@
id="PS_PROVIDER.task.T_ScheduledJob.message"
value="PowerShell Scheduled Jobs"
/>
<string
id="PS_PROVIDER.event.E_O_ExperimentalFeatureInvalidName.message"
value="Experimental Feature Initialization: Ignore the experimental feature '%1' from the config file. %2"
/>
<string
id="PS_PROVIDER.event.E_O_ExperimentalFeatureReadConfigError.message"
value="Experimental Feature Initialization: Failed to read the config file.%n Exception: %1 %n Message: %2 %n StackTrace: %3 %n"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a space needed between file. and %n

Copy link
Member Author

Choose a reason for hiding this comment

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

%n is a new line, so no space is needed here. For Exception: %1 %n Message: %2 %n StackTrace: %3 %n, the space character before %n is not necessary, but it's been that way in other places of this manifest file, so I followed the existing pattern.

/>
<string
id="PS_PROVIDER.task.T_ExperimentalFeature.message"
value="PowerShell Experimental Features"
/>
<string
id="PS_PROVIDER.task.T_NamedPipe.message"
value="PowerShell Named Pipe IPC"
Expand Down
10 changes: 8 additions & 2 deletions src/System.Management.Automation/CoreCLR/EventResource.cs
Expand Up @@ -3,8 +3,8 @@

#if UNIX
/*
This code was generated by the tools\ResxGen\ResxGen.ps1 run against PowerShell-Core-Instrumentation.man.
To add or change logged events and the associated resources, edit PowerShell-Core-Instrumentation.man
This code was generated by the tools\ResxGen\ResxGen.ps1 run against PowerShell.Core.Instrumentation.man.
To add or change logged events and the associated resources, edit PowerShell.Core.Instrumentation.man
then rerun ResxGen.ps1 to produce an updated CS and Resx file.
*/
using System.Collections.Generic;
Expand Down Expand Up @@ -133,6 +133,12 @@ public static string GetMessage(int eventId, out int parameterCount)
case 12039:
parameterCount = 0;
return "PS_PROVIDEReventE_A_RUNSPACEPOOL_TRANSFER";
case 12289:
parameterCount = 2;
return "PS_PROVIDEReventE_O_ExperimentalFeatureInvalidName";
case 12290:
parameterCount = 3;
return "PS_PROVIDEReventE_O_ExperimentalFeatureReadConfigError";
case 24577:
parameterCount = 1;
return "PS_PROVIDEReventE_O_ISEExecuteScript";
Expand Down
Expand Up @@ -230,6 +230,10 @@ internal static IEnumerable<ExtendedTypeDefinition> GetFormatData()
"System.Management.Automation.PSModuleInfo",
ViewsOf_System_Management_Automation_PSModuleInfo());

yield return new ExtendedTypeDefinition(
"System.Management.Automation.ExperimentalFeature",
ViewsOf_System_Management_Automation_ExperimentalFeature());

var td46 = new ExtendedTypeDefinition(
"Microsoft.PowerShell.Commands.BasicHtmlWebResponseObject",
ViewsOf_Microsoft_PowerShell_Commands_BasicHtmlWebResponseObject());
Expand Down Expand Up @@ -1240,6 +1244,33 @@ private static IEnumerable<FormatViewDefinition> ViewsOf_System_Management_Autom
.EndList());
}

private static IEnumerable<FormatViewDefinition> ViewsOf_System_Management_Automation_ExperimentalFeature()
{
yield return new FormatViewDefinition("ExperimentalFeature",
TableControl.Create()
.AddHeader(Alignment.Left, width: 35)
Copy link
Member

Choose a reason for hiding this comment

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

Seems like Names should be terse (and unique), so 35 seems quite wide.

Copy link
Member Author

Choose a reason for hiding this comment

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

When it's a module feature (feature declared in module), the feature name would be longer than the module name. The width for module name is 35, and I also use 35 here for the feature name. If you still think we should use a smaller size, I can make that change.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, for a module it makes sense. Let's leave it at 35 for now. Formatting changes aren't considered breaking so we can adjust later.

.AddHeader(Alignment.Right, width: 7)
.AddHeader(Alignment.Left, width: 35)
.AddHeader(Alignment.Left)
.StartRowDefinition()
.AddPropertyColumn("Name")
.AddPropertyColumn("Enabled")
.AddPropertyColumn("Source")
.AddPropertyColumn("Description")
.EndRowDefinition()
.EndTable());

yield return new FormatViewDefinition("ExperimentalFeature",
ListControl.Create()
.StartEntry()
.AddItemProperty("Name")
.AddItemProperty("Enabled")
.AddItemProperty("Source")
.AddItemProperty("Description")
.EndEntry()
.EndList());
}

private static IEnumerable<FormatViewDefinition> ViewsOf_Microsoft_PowerShell_Commands_BasicHtmlWebResponseObject()
{
yield return new FormatViewDefinition("Microsoft.PowerShell.Commands.BasicHtmlWebResponseObject",
Expand Down
43 changes: 43 additions & 0 deletions src/System.Management.Automation/engine/Attributes.cs
Expand Up @@ -620,12 +620,55 @@ public ParameterAttribute()
{
}

/// <summary>
/// Initializes a new instance that is associated with an experimental feature.
/// </summary>
public ParameterAttribute(string experimentName, ExperimentAction experimentAction)
{
ExperimentalAttribute.ValidateArguments(experimentName, experimentAction);
ExperimentName = experimentName;
ExperimentAction = experimentAction;
}

private string _parameterSetName = ParameterAttribute.AllParameterSets;

private string _helpMessage;
private string _helpMessageBaseName;
private string _helpMessageResourceId;

#region Experimental Feature Related Properties

/// <summary>
/// Get name of the experimental feature this attribute is associated with.
/// </summary>
public string ExperimentName { get; }

/// <summary>
/// Get action for engine to take when the experimental feature is enabled.
/// </summary>
public ExperimentAction ExperimentAction { get; }

internal bool ToHide => EffectiveAction == ExperimentAction.Hide;
internal bool ToShow => EffectiveAction == ExperimentAction.Show;

/// <summary>
/// Get effective action to take at run time.
/// </summary>
private ExperimentAction EffectiveAction
{
get
{
if (_effectiveAction == ExperimentAction.None)
{
_effectiveAction = ExperimentalFeature.GetActionToTake(ExperimentName, ExperimentAction);
}
return _effectiveAction;
}
}
private ExperimentAction _effectiveAction = default(ExperimentAction);

#endregion

/// <summary>
/// Gets and sets the parameter position. If not set, the parameter is named.
/// </summary>
Expand Down
Expand Up @@ -1756,13 +1756,15 @@ private List<CompletionResult> GetResultForAttributeArgument(CompletionContext c
{
PropertyInfo[] propertyInfos = attributeType.GetProperties(BindingFlags.Public | BindingFlags.Instance);
List<CompletionResult> result = new List<CompletionResult>();
foreach (PropertyInfo pro in propertyInfos)
foreach (PropertyInfo property in propertyInfos)
{
//Ignore TypeId (all attributes inherit it)
if (pro.Name != "TypeId" && (pro.Name.StartsWith(argName, StringComparison.OrdinalIgnoreCase)))
// Ignore getter-only properties, including 'TypeId' (all attributes inherit it).
if (!property.CanWrite) { continue; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems our convention is

if (!property.CanWrite) continue;

or

if (!property.CanWrite)
{
    continue;
}

I'd prefer second.

Copy link
Member Author

Choose a reason for hiding this comment

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

We use if () { // simple statement } all over the code base. But I don't think we should allow the first pattern in your comment. The if body should always be in { }.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Certainly agree that we should always use braces.

In a world where we no longer use teletypes (i.e. where screens are big and newlines are cheap), I tend to feel that we should always just hang the line so that as my eyes scan down a document at an approximate indentation level and always know what's going on. But, that's just a style preference, and inline-braces are still pretty good to me.


if (property.Name.StartsWith(argName, StringComparison.OrdinalIgnoreCase))
{
result.Add(new CompletionResult(pro.Name, pro.Name, CompletionResultType.Property,
pro.PropertyType.ToString() + " " + pro.Name));
result.Add(new CompletionResult(property.Name, property.Name, CompletionResultType.Property,
property.PropertyType.ToString() + " " + property.Name));
}
}
return result;
Expand Down
Expand Up @@ -1698,12 +1698,10 @@ internal static string ConcatenateStringPathArguments(CommandElementAst stringAs

foreach (ValidateArgumentsAttribute att in parameter.Parameter.ValidationAttributes)
{
if (att is ValidateSetAttribute)
if (att is ValidateSetAttribute setAtt)
{
RemoveLastNullCompletionResult(result);

var setAtt = (ValidateSetAttribute)att;

string wordToComplete = context.WordToComplete;
string quote = HandleDoubleAndSingleQuote(ref wordToComplete);

Expand All @@ -1712,6 +1710,8 @@ internal static string ConcatenateStringPathArguments(CommandElementAst stringAs

foreach (string value in setAtt.ValidValues)
{
if (value == string.Empty) { continue; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume we're deliberately treating null differently, and hence no String.IsNullOrEmpty. Would whitespace be valid?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's impossible for a null to appear in ValidateSetAttribute.ValidValues.


if (wordToComplete.Equals(value, StringComparison.OrdinalIgnoreCase))
{
string completionText = quote == string.Empty ? value : quote + value + quote;
Expand Down
13 changes: 7 additions & 6 deletions src/System.Management.Automation/engine/CommandProcessor.cs
Expand Up @@ -18,7 +18,7 @@ namespace System.Management.Automation
/// </summary>
internal class CommandProcessor : CommandProcessorBase
{
#region ctor
#region ctor

static CommandProcessor()
{
Expand Down Expand Up @@ -87,9 +87,10 @@ internal CommandProcessor(IScriptCommandInfo scriptCommandInfo, ExecutionContext

// CommandProcessor

#endregion ctor
#endregion ctor

#region internal members

#region internal members
/// <summary>
/// Returns a CmdletParameterBinderController for the specified command
/// </summary>
Expand Down Expand Up @@ -393,9 +394,9 @@ internal override void ProcessRecord()
}
}

#endregion public_methods
#endregion public_methods

#region helper_methods
#region helper_methods

/// <summary>
/// Tells whether it is the first call to Read
Expand Down Expand Up @@ -849,7 +850,7 @@ internal override bool IsHelpRequested(out string helpTarget, out HelpCategory h
return base.IsHelpRequested(out helpTarget, out helpCategory);
}

#endregion helper_methods
#endregion helper_methods
}
}

21 changes: 19 additions & 2 deletions src/System.Management.Automation/engine/CommandProcessorBase.cs
Expand Up @@ -34,14 +34,31 @@ internal CommandProcessorBase()
/// The metadata about the command to run.
/// </param>
///
internal CommandProcessorBase(
CommandInfo commandInfo)
internal CommandProcessorBase(CommandInfo commandInfo)
{
if (commandInfo == null)
{
throw PSTraceSource.NewArgumentNullException("commandInfo");
}

if (commandInfo is IScriptCommandInfo scriptCommand)
{
ExperimentalAttribute expAttribute = scriptCommand.ScriptBlock.ExperimentalAttribute;
if (expAttribute != null && expAttribute.ToHide)
{
string errorTemplate = expAttribute.ExperimentAction == ExperimentAction.Hide
? DiscoveryExceptions.ScriptDisabledWhenFeatureOn
: DiscoveryExceptions.ScriptDisabledWhenFeatureOff;
string errorMsg = StringUtil.Format(errorTemplate, expAttribute.ExperimentName);
ErrorRecord errorRecord = new ErrorRecord(
new InvalidOperationException(errorMsg),
"ScriptCommandDisabled",
Copy link
Collaborator

Choose a reason for hiding this comment

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

No preference in any direction, but do we need new resource entries for new error IDs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessarily. Here, we use the same error id but the message could change depending on the experiment action.

ErrorCategory.InvalidOperation,
commandInfo);
throw new CmdletInvocationException(errorRecord);
}
}

CommandInfo = commandInfo;
}

Expand Down
Expand Up @@ -64,6 +64,18 @@ internal CompiledCommandParameter(RuntimeDefinedParameter runtimeDefinedParamete
// First, process attributes that aren't type conversions
foreach (Attribute attribute in runtimeDefinedParameter.Attributes)
{
if (processingDynamicParameters)
{
// When processing dynamic parameters, the attribute list may contain experimental attributes
// and disabled parameter attributes. We should ignore those attributes.
// When processing non-dynamic parameters, the experimental attributes and disabled parameter
// attributes have already been filtered out when constructing the RuntimeDefinedParameter.
if (attribute is ExperimentalAttribute || attribute is ParameterAttribute param && param.ToHide)
{
continue;
}
}

if (!(attribute is ArgumentTypeConverterAttribute))
{
ProcessAttribute(runtimeDefinedParameter.Name, attribute, ref validationAttributes, ref argTransformationAttributes, ref aliases);
Expand All @@ -78,7 +90,7 @@ internal CompiledCommandParameter(RuntimeDefinedParameter runtimeDefinedParamete
}

// Now process type converters
foreach (ArgumentTypeConverterAttribute attribute in runtimeDefinedParameter.Attributes.OfType<ArgumentTypeConverterAttribute>())
foreach (var attribute in runtimeDefinedParameter.Attributes.OfType<ArgumentTypeConverterAttribute>())
{
ProcessAttribute(runtimeDefinedParameter.Name, attribute, ref validationAttributes, ref argTransformationAttributes, ref aliases);
}
Expand Down Expand Up @@ -169,7 +181,15 @@ internal CompiledCommandParameter(MemberInfo member, bool processingDynamicParam

foreach (Attribute attr in memberAttributes)
{
ProcessAttribute(member.Name, attr, ref validationAttributes, ref argTransformationAttributes, ref aliases);
switch (attr)
{
case ExperimentalAttribute _:
case ParameterAttribute param when param.ToHide:
break;
default:
ProcessAttribute(member.Name, attr, ref validationAttributes, ref argTransformationAttributes, ref aliases);
break;
}
}

this.ValidationAttributes = validationAttributes == null
Expand Down Expand Up @@ -436,7 +456,6 @@ internal IEnumerable<ParameterSetSpecificMetadata> GetMatchingParameterSetData(u
ref Collection<ArgumentTransformationAttribute> argTransformationAttributes,
ref string[] aliases)
{
// NTRAID#Windows Out Of Band Releases-926374-2005/12/22-JonN
if (attribute == null)
return;

Expand Down