Cleanup uses of `CommandTypes.Workflow` and `WorkflowInfo` #6708

daxian-dbw · 2018-04-23T23:02:58Z

PR Summary

Clean up code related to the uses of CommandTypes.Workflow and WorkflowInfo in System.Management.Automation. This change mainly affects help provider code.

This PR includes a few minor breaking changes:

Change the public constructors of WorkflowInfo to internal. We don't support workflow in PSCore, so I think it makes sense to not allow people to create Workflow instances.
Remove the type System.Management.Automation.DebugSource since it's only used for workflow debugging.
Remove the overload of SetParent from the abstract class Debugger that is only used for workflow debugging.
Remove the same overload of SetParent from the derived class RemotingJobDebugger.

PR Checklist

daxian-dbw · 2018-04-23T23:06:58Z

src/System.Management.Automation/FormatAndOutput/DefaultFormatters/Help_format_ps1xml.cs

                MamlPossibleValueControl,
                MamlTrueFalseShortControl,
                MamlIndentedSyntaxControl,
-                CommonWorkflowParametersControl,


This entry is with the index 16 in the array sharedControls, so for an index that is greater than 16, we need to adjust it by subtracting 1. This is why you will see many index changes below.

You could leave null in this entry.
In another PR - you could introduce names for each shared control to make the file more friendly to human editing now that we no longer use a tool to generate it.

Will do in a separate PR #6712

lzybkr · 2018-04-23T23:08:57Z

src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs

-                        }
-
+                        sessionState.ExportedFunctions.Add(entry.Value);
+                        string message = StringUtil.Format(Modules.ExportingFunction, entry.Key);


Not for this PR - but the result of this format call is typically thrown away. It should be possible to check if methods like WriteVerbose will do anything before formatting, or at least add overloads that do the formatting only after deciding the formatted string is needed.

I think it would be the best to add a few overloads to WriteVerbose, which take a format string and arguments. So we can defer the construction of message until we know for sure we are going to write out verbose messages. What do you think?

That might be easiest to adopt.

Best is to use an if before the call, but folks tend to not like that style because it's verbose.

Accepting a delegate is also a clean solution, but that can allocate as well, so might not be an improvement.

Open #6740 to track this investigation.

lzybkr · 2018-04-23T23:14:37Z

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

                    _context.EngineHostInterface.ExternalHost,
                    _context.SessionState.Path.CurrentLocation,
-                    GetFunctionToSourceMap());
+                    new Dictionary<string, DebugSource>());


It's not obvious from the diffs, but it looks like this dictionary might not be needed anymore.
If it's needed and clear elsewhere in the code, then fine, but if it's not needed or unclear, a comment would be good.

Yes, I noticed that too. The implementation of SetParent in RemotingJobDebugger just throws away all the arguments. However, SetParent is a virtual method, theoretically, someone could derive the Debugger class and implement SetParent that actually use the dictionary ... So, it's a breaking change that I'm not sure if I can make.

@PaulHigin Is it OK to remove the functionSourceMap parameter from public virtual void SetParent?
If it's OK, I can clean up the DebugSource type as a whole.

@PaulHigin Could you please take a look and weigh in? Thanks!

Yes, the GetFunctionToSourceMap() is only for the workflow debugger. You can remove the dictionary from SetParent() and also the GetFunctionSource() private method.

Thanks @PaulHigin for the insight!
I have addressed your comment. Also removed the DebugSource type, which is not used anywhere else.

lzybkr · 2018-04-23T23:21:31Z

Assuming ModuleInfo.ExportedWorkflows will remain forever - it would be good to decide what to do about it - throwing an error seems unwise, but what about warning that it is ignore? And should it return the entries in the psd1 (reflecting the psd1 contents) or should it always return an empty array, reflecting what is actually imported? The property name would imply the former, but in reality, it typically is what is imported.

iSazonov · 2018-04-26T06:27:00Z

Seems if we return an empty array it will be a breaking change - in the case a throw is better then a warning.
If we want remove this in future we could mark this with [obsolete] and write a warning in scripts.

daxian-dbw · 2018-04-26T14:33:43Z

Currently, ModuleInfo.ExportedWorkflows returns an empty Dictionary<string, FunctionInfo> (not changed in this PR). I think the current behavior makes sense. We don't support XAML module and don't support the workflow keyword in the script, so there is no way to import a module that declares any workflow in it.

As to Get-Module -list, for a script module that has workflow foo {} defined in it, the module analysis does find foo as an exported module member, but consider foo to be a function (this seems wrong to me, module analysis probably shouldn't return any exported commands in case a workflow definition is found in AST, because that module won't be able to be loaded). The ExportedWorkflows property of the returned PSModuleInfo object is still an empty dictionary.

PaulHigin · 2018-04-26T22:46:35Z

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

                    _context.EngineHostInterface.ExternalHost,
                    _context.SessionState.Path.CurrentLocation,
-                    GetFunctionToSourceMap());
+                    new Dictionary<string, DebugSource>());


Yes, the GetFunctionToSourceMap() is only for the workflow debugger. You can remove the dictionary from SetParent() and also the GetFunctionSource() private method.

daxian-dbw added 2 commits April 20, 2018 03:32

Clean up WorkflowInfo and CommandTypes.Workflow related code

f3bf79f

[Feature] Minor updates

a02fa5d

daxian-dbw requested review from BrucePay, SteveL-MSFT, adityapatwardhan, anmenaga and lzybkr April 23, 2018 23:02

daxian-dbw requested review from Francisco-Gamino, PaulHigin and dantraMSFT as code owners April 23, 2018 23:02

daxian-dbw added the Breaking-Change breaking change that may affect users label Apr 23, 2018

daxian-dbw commented Apr 23, 2018

View reviewed changes

lzybkr reviewed Apr 23, 2018

View reviewed changes

This was referenced Apr 24, 2018

Clean up 'Help_format_ps1xml.cs' to not index into an array of shared controls #6712

Closed

Add overloads to WriteVerbose/WriteWarning to defer string construction until it's necessary #6740

Closed

adityapatwardhan approved these changes Apr 26, 2018

View reviewed changes

PaulHigin suggested changes Apr 26, 2018

View reviewed changes

[Feature] Address Paul's feedback

26a87b2

daxian-dbw requested a review from mirichmo as a code owner April 27, 2018 00:38

PaulHigin approved these changes Apr 27, 2018

View reviewed changes

daxian-dbw merged commit 8aec158 into PowerShell:master Apr 28, 2018

daxian-dbw deleted the cleanup-workflow branch April 28, 2018 06:02

KirkMunro mentioned this pull request Nov 12, 2019

Binary modules that worked in PowerShell 6 may not work in 7 #11053

Closed

Cleanup uses of CommandTypes.Workflow and WorkflowInfo #6708

Cleanup uses of CommandTypes.Workflow and WorkflowInfo #6708

Uh oh!

Conversation

daxian-dbw commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzybkr commented Apr 23, 2018

Uh oh!

iSazonov commented Apr 26, 2018

Uh oh!

daxian-dbw commented Apr 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Cleanup uses of `CommandTypes.Workflow` and `WorkflowInfo` #6708

Cleanup uses of `CommandTypes.Workflow` and `WorkflowInfo` #6708

daxian-dbw commented Apr 23, 2018 •

edited

Loading