Validate new Rule definition API

[rjmholt]

PSSA2 provides a new way to specify rules, which you can see here:

PSScriptAnalyzer/ScriptAnalyzer2/Builtin/Rules/AvoidEmptyCatchBlock.cs

Lines 12 to 48 in 9ee8c0f

	/// <summary>
	/// AvoidEmptyCatchBlock: Check if any empty catch block is used.
	/// </summary>
	[IdempotentRule]
	[ThreadsafeRule]
	[Rule("AvoidUsingEmptyCatchBlock", typeof(Strings), nameof(Strings.AvoidUsingEmptyCatchBlockDescription))]
	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);
	}
	}
	}
	}

That is based on the following implementation:

PSScriptAnalyzer/ScriptAnalyzer2/Rules/Rule.cs

Lines 12 to 91 in 9ee8c0f

	public abstract class Rule
	{
	protected Rule(RuleInfo ruleInfo)
	{
	RuleInfo = ruleInfo;
	}
	public RuleInfo RuleInfo { get; }
	protected ScriptDiagnostic CreateDiagnostic(string message, IScriptExtent extent)
	=> CreateDiagnostic(message, extent, RuleInfo.DefaultSeverity);
	protected ScriptDiagnostic CreateDiagnostic(string message, IScriptExtent extent, DiagnosticSeverity severity)
	=> CreateDiagnostic(message, extent, severity, corrections: null);
	protected ScriptDiagnostic CreateDiagnostic(string message, IScriptExtent extent, IReadOnlyList<Correction> corrections)
	=> CreateDiagnostic(message, extent, RuleInfo.DefaultSeverity, corrections);
	protected ScriptDiagnostic CreateDiagnostic(string message, IScriptExtent extent, DiagnosticSeverity severity, IReadOnlyList<Correction> corrections)
	{
	return new ScriptDiagnostic(RuleInfo, message, extent, severity, corrections);
	}
	}
	public abstract class Rule<TConfiguration> : Rule where TConfiguration : IRuleConfiguration
	{
	protected Rule(RuleInfo ruleInfo, TConfiguration ruleConfiguration)
	: base(ruleInfo)
	{
	Configuration = ruleConfiguration;
	}
	public TConfiguration Configuration { get; }
	}
	public abstract class ScriptRule : Rule
	{
	protected ScriptRule(RuleInfo ruleInfo) : base(ruleInfo)
	{
	}
	public abstract IEnumerable<ScriptDiagnostic> AnalyzeScript(Ast ast, IReadOnlyList<Token> tokens, string scriptPath);
	protected ScriptAstDiagnostic CreateDiagnostic(string message, Ast ast)
	=> CreateDiagnostic(message, ast, RuleInfo.DefaultSeverity);
	protected ScriptAstDiagnostic CreateDiagnostic(string message, Ast ast, DiagnosticSeverity severity)
	=> CreateDiagnostic(message, ast, severity, corrections: null);
	protected ScriptAstDiagnostic CreateDiagnostic(string message, Ast ast, IReadOnlyList<Correction> corrections)
	=> CreateDiagnostic(message, ast, RuleInfo.DefaultSeverity, corrections);
	protected ScriptAstDiagnostic CreateDiagnostic(string message, Ast ast, DiagnosticSeverity severity, IReadOnlyList<Correction> corrections)
	{
	return new ScriptAstDiagnostic(RuleInfo, message, ast, severity, corrections);
	}
	protected ScriptTokenDiagnostic CreateDiagnostic(string message, Token token)
	=> CreateDiagnostic(message, token, RuleInfo.DefaultSeverity);
	protected ScriptTokenDiagnostic CreateDiagnostic(string message, Token token, DiagnosticSeverity severity)
	=> CreateDiagnostic(message, token, severity, corrections: null);
	protected ScriptTokenDiagnostic CreateDiagnostic(string message, Token token, IReadOnlyList<Correction> corrections)
	=> CreateDiagnostic(message, token, RuleInfo.DefaultSeverity, corrections);
	protected ScriptTokenDiagnostic CreateDiagnostic(string message, Token token, DiagnosticSeverity severity, IReadOnlyList<Correction> corrections)
	{
	return new ScriptTokenDiagnostic(RuleInfo, message, token, severity, corrections);
	}
	}
	public abstract class ScriptRule<TConfiguration> : Rule<TConfiguration> where TConfiguration : IRuleConfiguration
	{
	protected ScriptRule(RuleInfo ruleInfo, TConfiguration ruleConfiguration) : base(ruleInfo, ruleConfiguration)
	{
	}
	public abstract IReadOnlyList<ScriptDiagnostic> AnalyzeScript(Ast ast, IReadOnlyList<Token> tokens, string scriptPath);
	}

This is nice because:

• The rule uses attributes, so we can know about it before instantiating one, and it's also neater and more declarative
• The abstract class can provide convenience methods that simplify invocation, helpfully transform inputs, and even can do diagnostic suppression before a diagnostic record object is allocated
• A rule can be generic in its settings object and then allow for those settings to be injected through the constructor based on a deserialised settings object. This adds boilerplate, but allows the object configuration to be totally up to the implementer based on their settings definition (which can be defined next to the rule) and their constructor logic

However, some downsides of the current implementation are:

• Allowing the same class to implement a rule and, say, a formatter is harder with the abstract class vs an interface (and interfaces should generally be preferred when possible)
• Using a protected method to emit diagnostics means there's no type check for rules that will never emit a diagnostic
• We force constructor injection. This is so you can use settings in your rule at the correct time from the code's perspective (e.g. default property values will work, unlike today), but it means more boilerplate for implementers.

I would very much like to keep the feature where we don't allocate an object for suppressed diagnostics, and it has the advantage that there's one fewer classes for an implementer to think about. Some convenience functions can be preserved with extension methods on interfaces, but some cannot.

So an example alternate implementation might look like:

public interface IScriptRule
{
    IEnumerable<Diagnostic> AnalyzeScript(Ast ast, IReadOnlyList<Token> tokens, string scriptFilePath);
}

public static class ScriptRuleExtensions
{
    // Convenience methods
}

But the main issue there is we lose the elegance of (1) making it easier to construct diagnostics, particularly passing in rule metadata automatically, and (2) being able to filter diagnostics before they are created.

So another alternative would be:

public interface IScriptRule
{
    void AnalyzeScript(IDiagnosticEmitter diagnosticEmitter, Ast ast, IReadOnlyList<Token> tokens, string scriptFilePath);
}

// This could be something like an abstract class instead
// But ideally something we could shift the core implementation of if we needed to
public interface IDiagnosticEmitter
{
    // Rule required so that diagnostics know what rule they came from
    // But we trust the caller -- it would be possible to instantiate another rule and pass it in, but this is undesirable
    // Also having to pass `this` is something of an anti-pattern
    void EmitDiagnostic(IScriptRule rule, Ast ast, IReadOnlyList<Token> tokens, string scriptFilePath);
}

public static class DiagnosticEmitterExtensions
{
    // Convenience methods
}

Basically, the abstract class approach makes life better for both the framework and for rule implementers, but has the big downside that multiple inheritance is impossible... OTOH, there are other ways to do something like take a rule and turn it into a formatter, and single-inheritance generally promotes good cohesion...

[mattpwhite]

For those that want to implement their own rules in a .NET language, is the expectation that a PSA2 reference assembly would be published on NuGet? I’m more curious about the experience as a rule author maintaining an assembly containing rules than the specifics of the actual API. Ideally I’d have something to reference that would support rules for both 5.1 and Core and that would be loosely coupled to the exact version of PSA2 loaded at runtime.

[rjmholt]

is the expectation that a PSA2 reference assembly would be published on NuGet?

Yes that's the plan and primary driver for PSScriptAnalyzer 2, to enable NuGet-based hosting of it (mainly for the PowerShell extension for VSCode). It won't be a reference assembly, it will be the real thing. But there are no plans for a reference assembly currently.