Feature idea: Auto-Correct warnings

[bergmeister]

PSScriptAnalyzer already suggests possible fixes and for certain warnings like e.g. PSAvoidUsingCmdletAliases there is only solution for a fix (apart from suppressing it).
I have already created a working prototype of PSScriptAnalyzer that automatically corrects PSAvoidUsingCmdletAliases and would be happy to do a PR for that.
Would you be happy with that and if so, how should this feature be used? My first thought was to not add a switch in Invoke-ScriptAnalyzer because then we would need to modify the returned object in a way to tell the user that PSScriptAnalyzer has tried to resolve the warning and tell the user to check if he/she is happy with it. Because all the essential information is in the DiagnosticRecord object, it propose to have a new cmdlet (e.g. Invoke-ScriptAnalyzerWarningResolver) to which the user can pass the DiagnosticRecord object directly that the user received from Invoke-ScriptAnalyzer. For convenience another parameter set specifying the file(s) or folder (optionally recursively) might be useful as well. The new cmdlet should return a DiagnosticRecord as well with a description and a line diff (before/after) of the applied changes.
What are your thoughts on it?

[kapilmb]

Hey @bergmeister, adding something to PSScriptAnalyzer that fixes such warnings will be great! We already do that in the Invoke-Formatter cmdlet but only with formatting rules.

I think we have 3 options to tackle this problem:

1. Write a separate cmdlet that takes in the DiagnosticRecord objects produced by Invoke-ScriptAnalyzer
2. Write a separate cmdlet whose only function is to analyze and fix
3. Add a -Fix switch to Invoke-ScriptAnalyzer

We have been thinking about this for a while and I think the best approach right now would be to have a -Fix switch in the Invoke-ScriptAnalyzer cmdlet instead of the first 2 options. Let me explain.

Regarding the first option, the biggest problem with only handling DiagnosticRecord objects in a separate cmdlet is that you need to apply the fixes such that the extents in other DiagnosticRecord objects do not get invalidated. For example, let's say rule1 outputs extent1 for correction and rule2 outputs extent2. Suppose you first apply the fix corresponding to extent1. How do we know that after applying the first fix, extent2 is still valid? A simple approach is to pick out non-overlapping extents and applying them in descending order of line/col number. But then how would you handle cases where extents overlap? This problem rules out having a separate cmdlet just consume the DiagnosticRecord objects produced by Invoke-ScriptAnalyzer.

The second option is somewhat reasonable but it would involve a lot of duplication from the Invoke-ScriptAnalyzer cmdlet - something that I learned after releasing the Invoke-Formatter cmdlet. Duplication is a maintenance nightmare and hence in retrospect it would've been wiser to have a -fix switch to Invoke-ScriptAnalyzer even though it would have necessitated changing the cmdlet's output type when -fix is provided. Under normal circumstances, i.e., without the -fix switch, Invoke-ScriptAnalyzer would provide DiagnosticRecords but when -fix switch is provided, the cmdlet outputs a fixed string when a string input is given (-ScriptDefinition) and fixes the file contents when path (-Path) is given.

[bergmeister]

OK. It was interesting to read your thoughts (and I also learned about Invoke-Formatter, maybe you should mention it in the main README.md?).
I can understand your concerns about the first and second approach and agree that the 3rd approach is easier to implement (in fact that's how my prototype works at the moment) and possibly maintain. My prototype is just a crude method in the AvoidAlias class that utilises the properties available during the execution of AnalyzeScript(...) to fix the rule. This means that rule fixes can easily be implemented on a per-rule basis (using a e.g. a new method on IScriptRule?) and on a high level all we need to do, is make the -Fix switch available to the class. It might be good to prompt the user to confirm when using the switch to warn that this will change the files (and maybe declare this feature at first as experimental)? Maybe also implementing SupportsShouldProcess (so that one can use the -WhatIf option) would make sense and should not be difficult. Are you happy if I give you a PR of the finished prototype in a minimum viable implementation without considering all the special edge cases (it should be the user's responsibility to use source control and review the changes afterwards) so that it would work in 98% of the cases? From that on, one can then build on that and incrementally improve it.

[kapilmb]

I must've forgotten to include information regarding Invoke-Formatter cmdlet in the readme. Thanks for pointing that out.

Regarding -fix implementation, there is a slight problem with letting the rules fix the content themselves. Typically, rules can run concurrently on a file and so if one rule modifies the content then it may invalidate the content that is being analyzed by some other concurrently running rule .

I would recommend the implementation to be similar to how the Invoke-Formatter cmdlet implements the fixing mechanism, unless of course you have a better suggestion. Here are some pointers about the existing infrastructure that provides the fixing capability.

1. On lines L1535 and L1553 there are two versions of ScriptAnalyzer.Fix method.
2. Invoke-Formatter cmdlet.

Contributions are always welcome - I am just trying to make sure that we keep things consistent functionality and implementation wise.

[bergmeister]

@kapilmb : I have committed now the minimum viable feature in my fork here. I had a look at the code structure and Invoke-Formatter but I think I found a simpler solution that fits more neatly into the existing code and therefore does not result in code duplication.
I solely use the information available on the DiagnosticRecord class to fix it and added a new CanBeFixedAutomatically property to it to indicate whether it can be used for the generic auto-fix algorithm (this way each rule can still declare separately if it has/can set the properties accordingly such that the auto-fix algorithm can be applied). After every fix, I re-read the whole file to update the properties and repeat until no more fixable warnings are left. At the moment, it only works for PSAvoidUsingCmdletAliases and using the 'File' ParameterSet. I have built and tested it using .Net Standard 1.6 on PowerShell Core on Windows 10. Please let me know what you think about it.

[bergmeister]

@kapilmb : Just a friendly reminder whether you had a look at my proposed implementation here and if you think that it would be suitable for a PR?

[kapilmb]

@bergmeister Sorry I haven't been able to devote much time here. I just went through your changes and left some comments. I would still prefer the -fix switch to follow the pattern that Formatter.Fix method uses because that would mean less code duplication and a more uniform solution for the fixing problem.

[bergmeister]

Will close now since the PR was merged.