Skip to content

Add beginnings of PSScriptAnalyzer 2.0 #1476

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

Merged
merged 2 commits into from
Jun 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions ScriptAnalyzer2/Builder/BuiltinRulesBuilder.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
using Microsoft.PowerShell.ScriptAnalyzer.Builtin;
using Microsoft.PowerShell.ScriptAnalyzer.Configuration;
using Microsoft.PowerShell.ScriptAnalyzer.Instantiation;
using System;
using System.Collections.Generic;

namespace Microsoft.PowerShell.ScriptAnalyzer.Builder
{
public class BuiltinRulesBuilder
{
private IReadOnlyDictionary<string, IRuleConfiguration> _ruleConfiguration;

private IRuleComponentProvider _ruleComponents;

public BuiltinRulesBuilder WithRuleConfiguration(IReadOnlyDictionary<string, IRuleConfiguration> ruleConfigurationCollection)
{
_ruleConfiguration = ruleConfigurationCollection;
return this;
}

public BuiltinRulesBuilder WithRuleComponentProvider(IRuleComponentProvider ruleComponentProvider)
{
_ruleComponents = ruleComponentProvider;
return this;
}

public BuiltinRulesBuilder WithRuleComponentBuilder(Action<RuleComponentProviderBuilder> configureRuleComponents)
{
var ruleComponentProviderBuilder = new RuleComponentProviderBuilder();
configureRuleComponents(ruleComponentProviderBuilder);
_ruleComponents = ruleComponentProviderBuilder.Build();
return this;
}

public TypeRuleProvider Build()
{
return TypeRuleProvider.FromTypes(
_ruleConfiguration ?? Default.RuleConfiguration,
_ruleComponents ?? Default.RuleComponentProvider,
BuiltinRules.DefaultRules);
}
}
}
57 changes: 57 additions & 0 deletions ScriptAnalyzer2/Builder/ConfiguredBuilding.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
using Microsoft.PowerShell.ScriptAnalyzer.Configuration;
using Microsoft.PowerShell.ScriptAnalyzer.Execution;
using Microsoft.PowerShell.ScriptAnalyzer.Instantiation;
using Microsoft.PowerShell.ScriptAnalyzer.Utils;
using System;
using System.Collections.Generic;
using System.IO;
using System.Text;

namespace Microsoft.PowerShell.ScriptAnalyzer.Builder
{
public static class ConfiguredBuilding
{
public static ScriptAnalyzer CreateScriptAnalyzer(this IScriptAnalyzerConfiguration configuration)
{
var analyzerBuilder = new ScriptAnalyzerBuilder();

switch (configuration.BuiltinRules ?? BuiltinRulePreference.Default)
{
case BuiltinRulePreference.Aggressive:
case BuiltinRulePreference.Default:
analyzerBuilder.AddBuiltinRules();
break;
}

switch (configuration.RuleExecution ?? RuleExecutionMode.Default)
{
case RuleExecutionMode.Default:
case RuleExecutionMode.Parallel:
analyzerBuilder.WithRuleExecutorFactory(new ParallelLinqRuleExecutorFactory());
break;

case RuleExecutionMode.Sequential:
analyzerBuilder.WithRuleExecutorFactory(new SequentialRuleExecutorFactory());
break;
}

var componentProvider = new RuleComponentProviderBuilder().Build();

if (configuration.RulePaths != null)
{
foreach (string rulePath in configuration.RulePaths)
{
string extension = Path.GetExtension(rulePath);

if (extension.CaseInsensitiveEquals(".dll"))
{
analyzerBuilder.AddRuleProvider(TypeRuleProvider.FromAssemblyFile(configuration.RuleConfiguration, componentProvider, rulePath));
break;
}
}
}

return analyzerBuilder.Build();
}
}
}
58 changes: 58 additions & 0 deletions ScriptAnalyzer2/Builder/ScriptAnalyzerBuilder.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
using Microsoft.PowerShell.ScriptAnalyzer.Builtin;
using Microsoft.PowerShell.ScriptAnalyzer.Execution;
using Microsoft.PowerShell.ScriptAnalyzer.Instantiation;
using System;
using System.Collections.Generic;

namespace Microsoft.PowerShell.ScriptAnalyzer.Builder
{
public class ScriptAnalyzerBuilder
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we please try to keep the number of publicly exposed classes/methods generally minimal and instead use internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very much agree. My intent is to try and keep things behind interfaces where it makes sense to. In this case, I'm intending for the builder to be public for hosts, but certainly we will review the API surface

{
private readonly List<IRuleProvider> _ruleProviders;

private IRuleExecutorFactory _ruleExecutorFactory;

public ScriptAnalyzerBuilder()
{
_ruleProviders = new List<IRuleProvider>();
}

public ScriptAnalyzerBuilder WithRuleExecutorFactory(IRuleExecutorFactory ruleExecutorFactory)
{
_ruleExecutorFactory = ruleExecutorFactory;
return this;
}

public ScriptAnalyzerBuilder AddRuleProvider(IRuleProvider ruleProvider)
{
_ruleProviders.Add(ruleProvider);
return this;
}

public ScriptAnalyzerBuilder AddBuiltinRules()
{
_ruleProviders.Add(TypeRuleProvider.FromTypes(
Default.RuleConfiguration,
Default.RuleComponentProvider,
BuiltinRules.DefaultRules));
return this;
}

public ScriptAnalyzerBuilder AddBuiltinRules(Action<BuiltinRulesBuilder> configureBuiltinRules)
{
var builtinRulesBuilder = new BuiltinRulesBuilder();
configureBuiltinRules(builtinRulesBuilder);
_ruleProviders.Add(builtinRulesBuilder.Build());
return this;
}

public ScriptAnalyzer Build()
{
IRuleProvider ruleProvider = _ruleProviders.Count == 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

To me it feels like the special case of 1 rule should be handled inside CompositeRuleProvider from a consumer's perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's done here for efficiency. We can decide what's best when we're further along

? _ruleProviders[0]
: new CompositeRuleProvider(_ruleProviders);

return new ScriptAnalyzer(ruleProvider, _ruleExecutorFactory ?? Default.RuleExecutorFactory);
}
}
}
61 changes: 61 additions & 0 deletions ScriptAnalyzer2/Builtin/BuiltinRuleProvider.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
using Microsoft.PowerShell.ScriptAnalyzer.Builder;
using Microsoft.PowerShell.ScriptAnalyzer.Builtin.Rules;
using Microsoft.PowerShell.ScriptAnalyzer.Configuration;
using Microsoft.PowerShell.ScriptAnalyzer.Execution;
using Microsoft.PowerShell.ScriptAnalyzer.Runtime;
using Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules;
using System;
using System.Collections.Generic;
using System.Management.Automation.Runspaces;

namespace Microsoft.PowerShell.ScriptAnalyzer.Builtin
{
public static class BuiltinRules
{
public static IReadOnlyList<Type> DefaultRules { get; } = new[]
{
typeof(AvoidEmptyCatchBlock),
typeof(AvoidGlobalVars),
typeof(AvoidPositionalParameters),
typeof(AvoidUsingWMICmdlet),
typeof(UseDeclaredVarsMoreThanAssignments),
typeof(UseShouldProcessForStateChangingFunctions),
};
}

public static class Default
{
private static readonly Lazy<IRuleComponentProvider> s_ruleComponentProviderLazy = new Lazy<IRuleComponentProvider>(BuildRuleComponentProvider);

public static IReadOnlyDictionary<string, IRuleConfiguration> RuleConfiguration { get; } = new Dictionary<string, IRuleConfiguration>(StringComparer.OrdinalIgnoreCase)
{
{ "PS/AvoidUsingEmptyCatchBlock", null },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we eliminate the hard-coding here and use something like an attribute on the rule whether to run the rule by default or rely on something like typeof/nameof ? The PS prefix should also be defined only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think some of this will change. The key part though is that I want to improve cold start performance by eliminating reflection and should also have a nice declarative way to list rules.

{ "PS/AvoidGlobalVars", null },
{ "PS/AvoidUsingPositionalParameters", null },
{ "PS/AvoidUsingWMICmdlet", null },
{ "PS/UseDeclaredVarsMoreThanAssignments", null },
{ "PS/UseShouldProcessForStateChangingFunctions", null },
};

public static IRuleExecutorFactory RuleExecutorFactory { get; } = new ParallelLinqRuleExecutorFactory();

public static IRuleComponentProvider RuleComponentProvider => s_ruleComponentProviderLazy.Value;

private static IRuleComponentProvider BuildRuleComponentProvider()
{
return new RuleComponentProviderBuilder()
.AddSingleton(InstantiatePowerShellCommandDatabase())
.Build();
}

private static IPowerShellCommandDatabase InstantiatePowerShellCommandDatabase()
{
using (Runspace runspace = RunspaceFactory.CreateRunspace())
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about specifying the runspace pool as something like number of rules plus 2 (or whatever makes sense)? Just an idea. At the moment we do hard-code the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This particular concept I think I will rework and we can figure out the runspace configuration after that

{
runspace.Open();
return SessionStateCommandDatabase.Create(runspace.SessionStateProxy.InvokeCommand);
}
}
}

}
54 changes: 54 additions & 0 deletions ScriptAnalyzer2/Builtin/Rules/AvoidEmptyCatchBlock.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Management.Automation.Language;
using System.Globalization;
using Microsoft.PowerShell.ScriptAnalyzer.Rules;

namespace Microsoft.PowerShell.ScriptAnalyzer.Builtin.Rules
{
/// <summary>
/// AvoidEmptyCatchBlock: Check if any empty catch block is used.
/// </summary>
[IdempotentRule]
Copy link
Collaborator

Choose a reason for hiding this comment

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

When would a rule not be idempotent? To me it would be something like 'not depending on external state, like command info cache, which indirectly depends on the system it runs on'. If that is true then to me it rather feel like what we are saying is that the rule can run without PowerShell. Another attribute to declare whether the rule can run on its own (i.e. no dependency on internal engine or AST helper methods)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I imagine non-idempotent rules as ones that build some state as they go, like a list. Those need to be reset before re-running. There's more nuance here I think we should discuss though. Like rules that are threadsafe with other instances of the rule vs rules where the same instance can be reused everywhere. I think I've misused the term idempotent as a standin for that

[ThreadsafeRule]
[RuleDescription(typeof(Strings), nameof(Strings.AvoidUsingEmptyCatchBlockDescription))]
[Rule("AvoidUsingEmptyCatchBlock")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

RuleName feels like a better attribute name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One question is whether RuleDescription should be subsumed into Rule, at which point, Rule would make more sense.

public class AvoidEmptyCatchBlock : ScriptRule
{
public AvoidEmptyCatchBlock(RuleInfo ruleInfo)
: base(ruleInfo)
{
}

/// <summary>
/// AnalyzeScript: Analyze the script to check if any empty catch block is used.
/// </summary>
public override IEnumerable<ScriptDiagnostic> AnalyzeScript(Ast ast, IReadOnlyList<Token> tokens, string fileName)
{
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);

// Finds all CommandAsts.
IEnumerable<Ast> foundAsts = ast.FindAll(testAst => testAst is CatchClauseAst, true);

// Iterates all CatchClauseAst and check the statements count.
foreach (Ast foundAst in foundAsts)
{
CatchClauseAst catchAst = (CatchClauseAst)foundAst;

if (catchAst.Body.Statements.Count == 0)
{
yield return CreateDiagnostic(
string.Format(CultureInfo.CurrentCulture, Strings.AvoidEmptyCatchBlockError),
catchAst);
}
}
}
}
}




57 changes: 57 additions & 0 deletions ScriptAnalyzer2/Builtin/Rules/AvoidGlobalVars.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Management.Automation.Language;
using System.Globalization;
using Microsoft.PowerShell.ScriptAnalyzer.Rules;
using Microsoft.PowerShell.ScriptAnalyzer;
using Microsoft.PowerShell.ScriptAnalyzer.Builtin.Rules;

namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
{
/// <summary>
/// AvoidGlobalVars: Analyzes the ast to check that global variables are not used.
/// </summary>
[IdempotentRule]
[ThreadsafeRule]
[RuleDescription(typeof(Strings), nameof(Strings.AvoidGlobalVarsDescription))]
[Rule("AvoidGlobalVars")]
public class AvoidGlobalVars : ScriptRule
{
public AvoidGlobalVars(RuleInfo ruleInfo) : base(ruleInfo)
{
}

/// <summary>
/// AnalyzeScript: Analyzes the ast to check that global variables are not used. From the ILintScriptRule interface.
/// </summary>
/// <param name="ast">The script's ast</param>
/// <param name="fileName">The script's file name</param>
/// <returns>A List of diagnostic results of this rule</returns>
public override IEnumerable<ScriptDiagnostic> AnalyzeScript(Ast ast, IReadOnlyList<Token> tokens, string fileName)
{
IEnumerable<Ast> varAsts = ast.FindAll(testAst => testAst is VariableExpressionAst, true);

if (varAsts == null)
{
yield break;
}

foreach (VariableExpressionAst varAst in varAsts)
{
if (varAst.VariablePath.IsGlobal)
{
yield return CreateDiagnostic(
string.Format(CultureInfo.CurrentCulture, Strings.AvoidGlobalVarsError, varAst.VariablePath.UserPath),
varAst);
}
}
}
}
}




Loading