Add notes and sections covering Incompatibility Mitigations and Static Code Analysis #1

dantraMSFT · 2018-02-06T00:53:45Z

Fixed a few spelling errors
Added notes in sections '> NOTE:'
Updated ExperimentalFeatures manifest example to be an array of dictionaries (multiple experimental features)
Added section discussing mitigations for incompatible features
Added section discussion static code analysis to detect declaration problems and removing references to experiments that are completed.

…c Code Analysis

daxian-dbw · 2018-02-06T01:02:08Z

/cc @SteveL-MSFT @JamesWTruher It would be helpful if you can review.

SteveL-MSFT

A few small changes and I think it's ready for public consumption.

SteveL-MSFT · 2018-02-06T18:10:02Z

2-Draft-Accepted/RFCNNNN-Support-Experimental-Features.md

+2: Validating references to an experimental feature have been removed.
+At some point, the experiment is completed and either removed from or incorporated into the code base. At this point, a tool to scan script or compiled code would be needed to ensure any references to the experiment have been removed.
+
+> NOTE: Will consumers require builds with no experimental code? If so, how to do we ensure this?


I think it may be sufficient to have policy settings so users cannot enable experimental code if there are concerns for enterprises.

While that works for enterprise, it doesn't address cloud. TBH: I'm more interested in ensuring experiments are declared correctly when added and dead code is removed when the experiment is complete.

Policy settings can be set in powershell.config.json and take effect without GPO.
On windows, the same policy settings in registry (GPO) take precedence over the settings in powershell.config.json.

SteveL-MSFT · 2018-02-06T18:12:09Z

2-Draft-Accepted/RFCNNNN-Support-Experimental-Features.md

@@ -457,6 +470,27 @@ as well as a problem that we have to solve before using the experimental feature

 > Not sure how to handle it yet ...


For testing, I think we can simplify this. Due to the nature of being experimental, I think our regular test passes should run without any experimental features enabled. Separately, we should run the experimental feature tests with all experimental features enabled and all of the CI/Feature tests.

I'm not sure that this will work. I easily envision conflicting or mutually exclusive experimental features as mentioned. I wonder if we are going to need more tags.

I agree our regular test pass should run without any experimental feature enabled.

For the experimental features, we need to run test passes for each of them (one per test run) enabled. We will need more tags because tests targeting an experimental feature should be skipped sometimes.

JamesWTruher · 2018-02-06T19:04:32Z

2-Draft-Accepted/RFCNNNN-Support-Experimental-Features.md

@@ -80,6 +83,7 @@ The `JSON` schema should look like this:
  ]
 }
 ```
+> NOTE: Should we consider reading experimental feature lists also from the per-user configuration file?

 PowerShell reads the experimental feature list from the configuration file only once when starting up.


I think we should be pretty explicit here. The implication is that since we read the config file once during startup, it's implied that changing experimental features at runtime, I think that should be an explicit statement. Also, we should be explicit that the config file is read before engine start

Updated the words to be more explicit. #closed

JamesWTruher · 2018-02-06T19:05:42Z

2-Draft-Accepted/RFCNNNN-Support-Experimental-Features.md

@@ -327,8 +337,11 @@ For experimental features in PowerShell engine,
 an internal enum `EngineExperimentalFeatures` will be used to track the names of all available engine experimental features.


internal? Should this be public rather than internal?

The idea is to add a cmdlet to expose this information to users.
The members of the enum will change consistently, so it would be a consistent breaking change if we make it public.

Consider the possibility of third party experiments relying on engine experiments. For example, an editor experiment that depends on a language experiment. The editor experiment would need to either define it's own then check for the language experiment being enabled or expose it's capabilities as the same experiment.

Scripts and modules may also take dependencies on engine experiments so, whatever the form, the cooperation is likely desirable; especially for new features and capabilities.

Updated the PR to use a static read-only collection to track all engine experimental features.

For the scenario that third party experiments depending on engine experiments, it's covered in the Experimental Feature Dependency section. A third party experiment can specify a dependency on an engine experimental feature, but the third party experiment needs to be a separate experimental feature.
#closed

JamesWTruher · 2018-02-06T19:06:19Z

2-Draft-Accepted/RFCNNNN-Support-Experimental-Features.md

 such as `XXX-Description` where `XXX` is the feature name.
+
+> NOTE: Do experimental features really need resource strings for the description?  They are not likely to be localized and it will simplify declaration since the name and description can be placed in a table as a single unit and avoid misnamed or missing description resources.


probably not - it's probably ok not to require this.

as a single unit and avoid misnamed or missing description resources.

I think that make senses. Then the enum could really be a static table instead.

Updated the PR to use a static read-only collection to track all engine experimental features.
#closed

JamesWTruher · 2018-02-06T19:10:08Z

2-Draft-Accepted/RFCNNNN-Support-Experimental-Features.md

@@ -457,6 +470,27 @@ as well as a problem that we have to solve before using the experimental feature

 > Not sure how to handle it yet ...


I'm not sure that this will work. I easily envision conflicting or mutually exclusive experimental features as mentioned. I wonder if we are going to need more tags.

daxian-dbw · 2018-02-06T19:53:52Z

2-Draft-Accepted/RFCNNNN-Support-Experimental-Features.md

@@ -56,6 +56,9 @@ For example:
 PSDomainSpecificLanguage
 PSFileSystemProviderV2
 ```
+> NOTE: Can external modules participate in an experimental feature defined by the engine or another external module? For the engine: If PowerShell class support were to support exporting a script class from a module, support tools, such as PSScriptAnalyzer would likely participate with experimental rules for these classes.


Yes, they can, but they won't be part of the engine experimental feature, but experimental features that depend on the engine experimental feature. I think this scenario falls into the Experimental Feature Dependency section, where a module can expose experimentally features that depends on another experimental feature.

daxian-dbw · 2018-02-06T19:58:55Z

2-Draft-Accepted/RFCNNNN-Support-Experimental-Features.md

@@ -258,6 +264,8 @@ So a `Parameter` attribute can be associated with an experimental feature.
 When the experimental feature is enabled,
 PowerShell can choose to process or ignore the `Parameter` attribute depending on the `FeatureAction`.

+> NOTE: Can a parameter be marked with multiple experiment names? The use case is a new parameter that appears in 2 out of N parameter sets.


For every parameter set a parameter belongs to, it will have a Parameter attribute declared for that parameter set, whose ParamemterSetName is the name of the parameter set. So it's fine that a parameter can only be marked with one experimental feature name.

Perhaps I didn't phrase the question clearly. Can a given parameter participate in more than one experiments? My use case is primarily UX, where I'm altering existing parameters for better UX. In experiment 1, I define parameters A B and C, in experiment 2 I define A B D E. A and B are common to both experiments.

daxian-dbw · 2018-02-06T20:01:58Z

2-Draft-Accepted/RFCNNNN-Support-Experimental-Features.md

@@ -308,6 +316,8 @@ As for the type members with the `[Experimental()]` attributes,
 [`TypeDefiner`](https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/parser/PSType.cs#L17) needs to be updated to process or skip a type member when processing a `TypeDefinitionAst` node,
 depending on whether the experimental feature is enabled and the `FeatureAction` value.

+> NOTE: When are errors generated, if any, if an experimental script function or class member is referenced, such as within a script module or within the class implementation?


The errors will be generated at runtime. For a type reference, powershell will throw "type cannot be found". For a member reference, it will throw "member cannot be found".

For script functions, are the functions stripped or are stubs used to throw an explicit error. My thought we could end up with unintended false positives resolving to something else.

For classes, we could end up a with class that doesn't compile due to internal references to members that are 'removed' due to the experiment not being enabled.

How these are reported should be a dev experience issue, not an end user issue. (IMHO)

For script functions, if the associated experimental feature is not enabled, the functions will not be logged in the function table, so any reference to the script function would result in a "function not found" error at runtime.

For classes, if a class is referencing another class that is experimental, then depending on the manner of the reference, the former class itself or members of it should be marked with the same experimental feature.
Scenarios:

class Foo : Bar {...} -- Bar is an experimental feature, then Foo must also be an experimental feature.

class Foo { [Bar] $member } -- Bar is an experimental feature, then $member must also be an experimental feature.

daxian-dbw · 2018-02-06T20:03:39Z

2-Draft-Accepted/RFCNNNN-Support-Experimental-Features.md

@@ -327,8 +337,11 @@ For experimental features in PowerShell engine,
 an internal enum `EngineExperimentalFeatures` will be used to track the names of all available engine experimental features.


The idea is to add a cmdlet to expose this information to users.
The members of the enum will change consistently, so it would be a consistent breaking change if we make it public.

daxian-dbw · 2018-02-06T20:04:47Z

2-Draft-Accepted/RFCNNNN-Support-Experimental-Features.md

 such as `XXX-Description` where `XXX` is the feature name.
+
+> NOTE: Do experimental features really need resource strings for the description?  They are not likely to be localized and it will simplify declaration since the name and description can be placed in a table as a single unit and avoid misnamed or missing description resources.


as a single unit and avoid misnamed or missing description resources.

I think that make senses. Then the enum could really be a static table instead.

daxian-dbw · 2018-02-20T19:46:59Z

2-Draft-Accepted/RFCNNNN-Support-Experimental-Features.md

@@ -89,6 +93,8 @@ PowerShell reads the experimental feature list from the configuration file only
  and read the list if not yet.
  This is to make sure experimental features can be enabled when an application is hosting PowerShell.

+> NOTE: Current command-line parsing may make some experimental features difficult to flag due to the amount of work that occurs before parsing is completed. This is seen on Linux with Logging overrides.  It may be necessary to move parsing earlier in the startup call-path.


By looking at the code, at least the CommandLineParameterParser needs to be instantiated before parsing the command-line, so if there is an experimental feature added to CommandLineParameterParser, it may cause the configuration file overriding with -settingsfile to not work anyways.

I start to think that maybe we shouldn't use -settingsfile to override the configuration file at PSHome. Instead, we probably should use an environment variable, so that it can be checked and enforced the first thing when console starts up.

We should consider moving command-line parsing as early in startup as possible. For the case where the CommandLineParser is introducing an experimental feature, it could internally parse a second time if the experimental feature is enabled by the settings file. This may require the CommandLine parser to populate the hashset or build the hash set and populate it at the end of parsing. Finally, for the few cases where updates are made as a side-effect of parsing, such as with log settings, these updates should be performed after parsing completes. In short, distinguishing between parse an apply.

Address some of the comments and use syntax line break

daxian-dbw · 2018-02-20T23:31:35Z

2-Draft-Accepted/RFCNNNN-Support-Experimental-Features.md

@@ -81,14 +84,19 @@ The `JSON` schema should look like this:
 }
 ```

-PowerShell reads the experimental feature list from the configuration file only once when starting up.
+> NOTE: Should we consider reading experimental feature lists also from the per-user configuration file?


Good question. If we should, which one takes precedence?

I would expect the per-user configuration to take precedence.

daxian-dbw · 2018-02-21T00:15:39Z

2-Draft-Accepted/RFCNNNN-Support-Experimental-Features.md

+- An example of an unintended incompatibility can occur when two experimental features prove to be incompatible through usage.
+
+A minimal implementation will support defining a list of experimental feature pairs in the `powershell.config.json` file.
+At startup, attempts to use incompatible features would produce warnings and consider both as disabled.


We need to be more explicit on how conflicting features would be described, both in ExperimentalFeature and in PrivateData.ExperimentalFeatures from a module manifest.

Dan and I discussed this and decided to not get into details on this section.

Incorporate discussion into the RFC

Add notes and sections covering Incompatibility Mitigations and Stati…

cc93c8f

…c Code Analysis

daxian-dbw self-requested a review February 6, 2018 00:55

dantraMSFT and others added 2 commits February 5, 2018 17:03

Fix typo in PrivateData example

fdc7092

Add back two new lines

3b2d823

SteveL-MSFT suggested changes Feb 6, 2018

View reviewed changes

JamesWTruher reviewed Feb 6, 2018

View reviewed changes

daxian-dbw reviewed Feb 6, 2018

View reviewed changes

daxian-dbw reviewed Feb 20, 2018

View reviewed changes

daxian-dbw and others added 2 commits February 20, 2018 12:53

Address some of the comments and use syntax line break

785a675

Merge pull request #1 from daxian-dbw/myexp

55bd45c

Address some of the comments and use syntax line break

daxian-dbw reviewed Feb 21, 2018

View reviewed changes

daxian-dbw and others added 2 commits February 22, 2018 17:25

Incorporate discussion into the RFC

57c989c

Merge pull request #2 from daxian-dbw/myexp

151eaf7

Incorporate discussion into the RFC

daxian-dbw merged commit 1a3d4df into daxian-dbw:experimental Feb 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add notes and sections covering Incompatibility Mitigations and Static Code Analysis #1

Add notes and sections covering Incompatibility Mitigations and Static Code Analysis #1

dantraMSFT commented Feb 6, 2018

daxian-dbw commented Feb 6, 2018

SteveL-MSFT left a comment

SteveL-MSFT Feb 6, 2018

dantraMSFT Feb 7, 2018

daxian-dbw Feb 20, 2018 •

edited

SteveL-MSFT Feb 6, 2018

JamesWTruher Feb 6, 2018

daxian-dbw Feb 20, 2018

JamesWTruher Feb 6, 2018

daxian-dbw Feb 20, 2018 •

edited

JamesWTruher Feb 6, 2018

daxian-dbw Feb 6, 2018

dantraMSFT Feb 6, 2018

daxian-dbw Feb 20, 2018 •

edited

JamesWTruher Feb 6, 2018

daxian-dbw Feb 6, 2018

daxian-dbw Feb 20, 2018 •

edited

JamesWTruher Feb 6, 2018

daxian-dbw Feb 6, 2018

daxian-dbw Feb 6, 2018

dantraMSFT Feb 6, 2018

daxian-dbw Feb 6, 2018

dantraMSFT Feb 6, 2018 •

edited

daxian-dbw Feb 20, 2018

daxian-dbw Feb 6, 2018

daxian-dbw Feb 6, 2018

daxian-dbw Feb 20, 2018

dantraMSFT Feb 21, 2018

daxian-dbw Feb 20, 2018

dantraMSFT Feb 21, 2018

daxian-dbw Feb 21, 2018

daxian-dbw Feb 23, 2018

		@@ -457,6 +470,27 @@ as well as a problem that we have to solve before using the experimental feature

		> Not sure how to handle it yet ...

		@@ -327,8 +337,11 @@ For experimental features in PowerShell engine,
		an internal enum `EngineExperimentalFeatures` will be used to track the names of all available engine experimental features.

		such as `XXX-Description` where `XXX` is the feature name.

		> NOTE: Do experimental features really need resource strings for the description? They are not likely to be localized and it will simplify declaration since the name and description can be placed in a table as a single unit and avoid misnamed or missing description resources.

Add notes and sections covering Incompatibility Mitigations and Static Code Analysis #1

Add notes and sections covering Incompatibility Mitigations and Static Code Analysis #1

Conversation

dantraMSFT commented Feb 6, 2018

daxian-dbw commented Feb 6, 2018

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Feb 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Feb 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Feb 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Feb 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantraMSFT Feb 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Feb 20, 2018 •

edited

daxian-dbw Feb 20, 2018 •

edited

daxian-dbw Feb 20, 2018 •

edited

daxian-dbw Feb 20, 2018 •

edited

dantraMSFT Feb 6, 2018 •

edited