Skip to content

Commit

Permalink
Analyzer to ban uncached regexes (space-wizards#5107)
Browse files Browse the repository at this point in the history
Using static Regex functions that take in a pattern is bad, because they constantly have to be re-parsed. Cache the Regex instance.
  • Loading branch information
PJB3005 authored May 6, 2024
1 parent c83720b commit 4d528dd
Show file tree
Hide file tree
Showing 4 changed files with 127 additions and 1 deletion.
57 changes: 57 additions & 0 deletions Robust.Analyzers.Tests/NoUncachedRegexAnalyzerTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CSharp.Testing;
using Microsoft.CodeAnalysis.Testing;
using Microsoft.CodeAnalysis.Testing.Verifiers;
using NUnit.Framework;
using VerifyCS =
Microsoft.CodeAnalysis.CSharp.Testing.NUnit.AnalyzerVerifier<Robust.Analyzers.NoUncachedRegexAnalyzer>;

namespace Robust.Analyzers.Tests;

[Parallelizable(ParallelScope.All | ParallelScope.Fixtures)]
[TestFixture]
public sealed class NoUncachedRegexAnalyzerTest
{
private static Task Verifier(string code, params DiagnosticResult[] expected)
{
var test = new CSharpAnalyzerTest<NoUncachedRegexAnalyzer, NUnitVerifier>()
{
TestState =
{
Sources = { code }
},
};

// ExpectedDiagnostics cannot be set, so we need to AddRange here...
test.TestState.ExpectedDiagnostics.AddRange(expected);

return test.RunAsync();
}

[Test]
public async Task Test()
{
const string code = """
using System.Text.RegularExpressions;
public static class Foo
{
public static void Bad()
{
Regex.Replace("foo", "bar", "baz");
}
public static void Good()
{
var r = new Regex("bar");
r.Replace("foo", "baz");
}
}
""";

await Verifier(code,
// /0/Test0.cs(7,9): warning RA0026: Usage of a static Regex function that takes in a pattern string. This can cause constant re-parsing of the pattern.
VerifyCS.Diagnostic().WithSpan(7, 9, 7, 43)
);
}
}
66 changes: 66 additions & 0 deletions Robust.Analyzers/NoUncachedRegexAnalyzer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
using System.Collections.Immutable;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using Robust.Roslyn.Shared;

namespace Robust.Analyzers;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class NoUncachedRegexAnalyzer : DiagnosticAnalyzer
{
private const string RegexTypeName = "Regex";
private const string RegexType = $"System.Text.RegularExpressions.{RegexTypeName}";

private static readonly DiagnosticDescriptor Rule = new (
Diagnostics.IdUncachedRegex,
"Use of uncached static Regex function",
"Usage of a static Regex function that takes in a pattern string. This can cause constant re-parsing of the pattern.",
"Usage",
DiagnosticSeverity.Warning,
true);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);

public static readonly HashSet<string> BadFunctions =
[
"Count",
"EnumerateMatches",
"IsMatch",
"Match",
"Matches",
"Replace",
"Split"
];

public override void Initialize(AnalysisContext context)
{
context.EnableConcurrentExecution();
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
context.RegisterOperationAction(CheckInvocation, OperationKind.Invocation);
}

private static void CheckInvocation(OperationAnalysisContext context)
{
if (context.Operation is not IInvocationOperation invocation)
return;

// All Regex functions we care about are static.
var targetMethod = invocation.TargetMethod;
if (!targetMethod.IsStatic)
return;

// Bail early.
if (targetMethod.ContainingType.Name != "Regex")
return;

var regexType = context.Compilation.GetTypeByMetadataName(RegexType);
if (!SymbolEqualityComparer.Default.Equals(regexType, targetMethod.ContainingType))
return;

if (!BadFunctions.Contains(targetMethod.Name))
return;

context.ReportDiagnostic(Diagnostic.Create(Rule, invocation.Syntax.GetLocation()));
}
}
1 change: 1 addition & 0 deletions Robust.Roslyn.Shared/Diagnostics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public static class Diagnostics
public const string IdComponentPauseNoParentAttribute = "RA0023";
public const string IdComponentPauseWrongTypeAttribute = "RA0024";
public const string IdDependencyFieldAssigned = "RA0025";
public const string IdUncachedRegex = "RA0026";

public static SuppressionDescriptor MeansImplicitAssignment =>
new SuppressionDescriptor("RADC1000", "CS0649", "Marked as implicitly assigned.");
Expand Down
4 changes: 3 additions & 1 deletion Robust.Shared/Localization/LocalizationManager.Functions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ namespace Robust.Shared.Localization
{
internal sealed partial class LocalizationManager
{
private static readonly Regex RegexWordMatch = new Regex(@"\w+");

private void AddBuiltInFunctions(FluentBundle bundle)
{
// Grammatical gender / pronouns
Expand Down Expand Up @@ -108,7 +110,7 @@ private ILocValue FuncIndefinite(LocArgs args)
var a = new LocValueString("a");
var an = new LocValueString("an");

var m = Regex.Match(input, @"\w+");
var m = RegexWordMatch.Match(input);
if (m.Success)
{
word = m.Groups[0].Value;
Expand Down

0 comments on commit 4d528dd

Please sign in to comment.