Skip to content

Commit

Permalink
Enable regex generator nullable reference types validation (#80142)
Browse files Browse the repository at this point in the history
* Enable regex generator nullable reference types

Multiple times in the past we've audited the regex source generator for nullable reference type annotations, but as it was only compiled officially for netstandard2.0, any work we did to get it green eventually rotted.  This follows the work done to enable also building a .NET Core target for the json generator to do the same for the regex generator.  In doing so, I also took the opportunity to clean up the REGEXGENERATOR compilation constant.

* Add SetTargetFramework to ProjectReferences

* Fix regex unit tests
  • Loading branch information
stephentoub authored Jan 4, 2023
1 parent 2975fa4 commit 11ab33a
Show file tree
Hide file tree
Showing 21 changed files with 304 additions and 286 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@
<ItemGroup>
<ProjectReference Include="$(LibrariesProjectRoot)System.Text.RegularExpressions\gen\System.Text.RegularExpressions.Generator.csproj"
ReferenceOutputAssembly="false"
SetTargetFramework="TargetFramework=netstandard2.0"
OutputItemType="Analyzer" />
<Reference Include="System.Collections" />
<Reference Include="System.Collections.NonGeneric" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@
<ItemGroup>
<ProjectReference Include="$(LibrariesProjectRoot)System.Text.RegularExpressions\gen\System.Text.RegularExpressions.Generator.csproj"
ReferenceOutputAssembly="false"
SetTargetFramework="TargetFramework=netstandard2.0"
OutputItemType="Analyzer" />
<Reference Include="System.Collections" />
<Reference Include="System.Collections.Concurrent" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,7 @@
<ItemGroup>
<ProjectReference Include="$(LibrariesProjectRoot)System.Text.RegularExpressions\gen\System.Text.RegularExpressions.Generator.csproj"
ReferenceOutputAssembly="false"
SetTargetFramework="TargetFramework=netstandard2.0"
OutputItemType="Analyzer" />
<Reference Include="System.Collections" />
<Reference Include="System.Collections.Concurrent" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,7 @@
<ItemGroup>
<ProjectReference Include="$(LibrariesProjectRoot)System.Text.RegularExpressions\gen\System.Text.RegularExpressions.Generator.csproj"
ReferenceOutputAssembly="false"
SetTargetFramework="TargetFramework=netstandard2.0"
OutputItemType="Analyzer" />

<PackageReference Include="Moq" Version="$(MoqVersion)" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -843,7 +843,7 @@ void EmitFixedSet_LeftToRight()
3 => $"{span}.IndexOfAny({Literal(primarySet.Chars[0])}, {Literal(primarySet.Chars[1])}, {Literal(primarySet.Chars[2])})",
_ => $"{span}.IndexOfAny({Literal(new string(primarySet.Chars))})",
} :
(primarySet.Range.Value.LowInclusive == primarySet.Range.Value.HighInclusive, primarySet.Range.Value.Negated) switch
(primarySet.Range!.Value.LowInclusive == primarySet.Range.Value.HighInclusive, primarySet.Range.Value.Negated) switch
{
(false, false) => $"{span}.IndexOfAnyInRange({Literal(primarySet.Range.Value.LowInclusive)}, {Literal(primarySet.Range.Value.HighInclusive)})",
(true, false) => $"{span}.IndexOf({Literal(primarySet.Range.Value.LowInclusive)})",
Expand Down Expand Up @@ -2920,7 +2920,7 @@ void EmitSingleCharLoop(RegexNode node, RegexNode? subsequent = null, bool emitL
if (!rtl &&
node.N > 1 && // no point in using IndexOf for small loops, in particular optionals
subsequent?.FindStartingLiteralNode() is RegexNode literalNode &&
TryEmitIndexOf(literalNode, useLast: true, negate: false, out int literalLength, out string indexOfExpr))
TryEmitIndexOf(literalNode, useLast: true, negate: false, out int literalLength, out string? indexOfExpr))
{
writer.WriteLine($"if ({startingPos} >= {endingPos} ||");

Expand Down Expand Up @@ -3685,7 +3685,7 @@ void EmitSingleCharAtomicLoop(RegexNode node, bool emitLengthChecksIfRequired =
TransferSliceStaticPosToPos();
writer.WriteLine($"int {iterationLocal} = inputSpan.Length - pos;");
}
else if (maxIterations == int.MaxValue && TryEmitIndexOf(node, useLast: false, negate: true, out _, out string indexOfExpr))
else if (maxIterations == int.MaxValue && TryEmitIndexOf(node, useLast: false, negate: true, out _, out string? indexOfExpr))
{
// We're unbounded and we can use an IndexOf method to perform the search. The unbounded restriction is
// purely for simplicity; it could be removed in the future with additional code to handle that case.
Expand Down Expand Up @@ -4351,8 +4351,8 @@ private static bool TryEmitIndexOf(
if (node.Kind == RegexNodeKind.Multi)
{
Debug.Assert(!negate, "Negation isn't appropriate for a multi");
indexOfExpr = $"{last}IndexOf({Literal(node.Str)})";
literalLength = node.Str.Length;
indexOfExpr = $"{last}IndexOf({Literal(node.Str!)})";
literalLength = node.Str!.Length;
return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public partial class RegexGenerator
return null;
}

IMethodSymbol regexMethodSymbol = context.TargetSymbol as IMethodSymbol;
IMethodSymbol? regexMethodSymbol = context.TargetSymbol as IMethodSymbol;
if (regexMethodSymbol is null)
{
return null;
Expand Down Expand Up @@ -95,7 +95,7 @@ public partial class RegexGenerator
// int matchTimeoutMilliseconds, or string cultureName.
else if (items.Length == 3)
{
if (items[2].Type.SpecialType == SpecialType.System_Int32)
if (items[2].Type?.SpecialType == SpecialType.System_Int32)
{
matchTimeout = items[2].Value as int?;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<!--
Source generators must target netstandard2.0, which doesn't support nullable reference types. In order
to enable the nullable reference type compiler checks, we also target NetCoreAppToolCurrent. Note that
this doesn't use the live shared framework but an LKG targeting pack instead to avoid layering concerns.
-->
<TargetFrameworks>netstandard2.0;$(NetCoreAppToolCurrent)</TargetFrameworks>
<EnableDefaultItems>true</EnableDefaultItems>
<EnableDefaultEmbeddedResourceItems>false</EnableDefaultEmbeddedResourceItems>
<UsingToolXliff>true</UsingToolXliff>
<CLSCompliant>false</CLSCompliant>
<NoWarn>$(NoWarn);CS0436;CS0649</NoWarn>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<DefineConstants>$(DefineConstants);REGEXGENERATOR</DefineConstants>
<AnalyzerLanguage>cs</AnalyzerLanguage>
</PropertyGroup>

Expand Down Expand Up @@ -46,4 +50,9 @@
<Compile Include="..\src\System\Collections\HashtableExtensions.cs" Link="Production\HashtableExtensions.cs" />
</ItemGroup>

<!-- Don't reference System.Text.RegularExpressions from the LKG, as shared sources are compiled into this project. -->
<ItemGroup Condition="'$(TargetFramework)' == '$(NetCoreAppToolCurrent)'">
<DefaultReferenceExclusion Include="System.Text.RegularExpressions" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,13 @@ private static bool ValidateParameters(ImmutableArray<IArgumentOperation> argume
for (int i = 0; i < arguments.Length; i++)
{
IArgumentOperation argument = arguments[i];
string argumentName = argument.Parameter.Name;
string? argumentName = argument.Parameter?.Name;

// If the argument name is null (e.g. an __arglist), then we don't emit a diagnostic.
if (argumentName is null)
{
return false;
}

// If one of the arguments is a timeout, then we don't emit a diagnostic.
if (argumentName.Equals(timeoutArgumentName, StringComparison.OrdinalIgnoreCase) ||
Expand All @@ -180,7 +186,7 @@ private static bool ValidateParameters(ImmutableArray<IArgumentOperation> argume
// If the argument is the pattern, then we validate that it is constant and we store the index.
if (argumentName.Equals(PatternArgumentName, StringComparison.OrdinalIgnoreCase))
{
if (!IsConstant(argument))
if (!argument.Value.ConstantValue.HasValue)
{
return false;
}
Expand All @@ -191,12 +197,12 @@ private static bool ValidateParameters(ImmutableArray<IArgumentOperation> argume
// If the argument is the options, then we validate that it is constant, that it doesn't have RegexOptions.NonBacktracking, and we store the index.
if (argumentName.Equals(OptionsArgumentName, StringComparison.OrdinalIgnoreCase))
{
if (!IsConstant(argument))
if (!argument.Value.ConstantValue.HasValue)
{
return false;
}

RegexOptions value = (RegexOptions)((int)argument.Value.ConstantValue.Value);
RegexOptions value = (RegexOptions)(int)argument.Value.ConstantValue.Value!;
if ((value & RegexOptions.NonBacktracking) > 0)
{
return false;
Expand All @@ -209,15 +215,6 @@ private static bool ValidateParameters(ImmutableArray<IArgumentOperation> argume
return true;
}

/// <summary>
/// Ensures that the input to the constructor or invocation is constant at compile time
/// which is a requirement in order to be able to use the source generator.
/// </summary>
/// <param name="argument">The argument to be analyzed.</param>
/// <returns><see langword="true"/> if the argument is constant; otherwise, <see langword="false"/>.</returns>
private static bool IsConstant(IArgumentOperation argument)
=> argument.Value.ConstantValue.HasValue;

/// <summary>
/// Ensures that the compilation can find the Regex and RegexAttribute types, and also validates that the
/// LangVersion of the project is >= 10.0 (which is the current requirement for the Regex source generator.
Expand All @@ -233,7 +230,7 @@ private static bool ProjectSupportsRegexSourceGenerator(Compilation compilation,
return false;
}

INamedTypeSymbol generatedRegexAttributeTypeSymbol = compilation.GetTypeByMetadataName(GeneratedRegexTypeName);
INamedTypeSymbol? generatedRegexAttributeTypeSymbol = compilation.GetTypeByMetadataName(GeneratedRegexTypeName);
if (generatedRegexAttributeTypeSymbol == null)
{
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ private static async Task<Document> ConvertToSourceGenerator(Document document,
{
operationArguments = invocationOperation.Arguments;
IEnumerable<SyntaxNode> arguments = operationArguments
.Where(arg => arg.Parameter.Name is not (UpgradeToGeneratedRegexAnalyzer.OptionsArgumentName or UpgradeToGeneratedRegexAnalyzer.PatternArgumentName))
.Where(arg => arg.Parameter?.Name is not (UpgradeToGeneratedRegexAnalyzer.OptionsArgumentName or UpgradeToGeneratedRegexAnalyzer.PatternArgumentName))
.Select(arg => arg.Syntax);

replacement = generator.InvocationExpression(generator.MemberAccessExpression(replacement, invocationOperation.TargetMethod.Name), arguments);
Expand Down Expand Up @@ -192,7 +192,7 @@ private static async Task<Document> ConvertToSourceGenerator(Document document,
// we also need to parse the pattern in case there are options that were specified inside the pattern via the `(?i)` switch.
SyntaxNode? cultureNameValue = null;
RegexOptions regexOptions = regexOptionsValue is not null ? GetRegexOptionsFromArgument(operationArguments) : RegexOptions.None;
string pattern = GetRegexPatternFromArgument(operationArguments);
string pattern = GetRegexPatternFromArgument(operationArguments)!;
regexOptions |= RegexParser.ParseOptionsInPattern(pattern, regexOptions);

// If the options include IgnoreCase and don't specify CultureInvariant then we will have to calculate the user's current culture in order to pass
Expand Down Expand Up @@ -239,9 +239,9 @@ static IEnumerable<ISymbol> GetAllMembers(ITypeSymbol? symbol)
}
}

static string GetRegexPatternFromArgument(ImmutableArray<IArgumentOperation> arguments)
static string? GetRegexPatternFromArgument(ImmutableArray<IArgumentOperation> arguments)
{
IArgumentOperation? patternArgument = arguments.SingleOrDefault(arg => arg.Parameter.Name == UpgradeToGeneratedRegexAnalyzer.PatternArgumentName);
IArgumentOperation? patternArgument = arguments.SingleOrDefault(arg => arg.Parameter?.Name == UpgradeToGeneratedRegexAnalyzer.PatternArgumentName);
if (patternArgument is null)
{
return null;
Expand All @@ -252,16 +252,17 @@ static string GetRegexPatternFromArgument(ImmutableArray<IArgumentOperation> arg

static RegexOptions GetRegexOptionsFromArgument(ImmutableArray<IArgumentOperation> arguments)
{
IArgumentOperation? optionsArgument = arguments.SingleOrDefault(arg => arg.Parameter.Name == UpgradeToGeneratedRegexAnalyzer.OptionsArgumentName);
IArgumentOperation? optionsArgument = arguments.SingleOrDefault(arg => arg.Parameter?.Name == UpgradeToGeneratedRegexAnalyzer.OptionsArgumentName);

return optionsArgument is null ? RegexOptions.None :
(RegexOptions)(int)optionsArgument.Value.ConstantValue.Value;
return optionsArgument is null || !optionsArgument.Value.ConstantValue.HasValue ?
RegexOptions.None :
(RegexOptions)(int)optionsArgument.Value.ConstantValue.Value!;
}

// Helper method that looks generates the node for pattern argument or options argument.
static SyntaxNode? GetNode(ImmutableArray<IArgumentOperation> arguments, SyntaxGenerator generator, string parameterName)
{
IArgumentOperation? argument = arguments.SingleOrDefault(arg => arg.Parameter.Name == parameterName);
IArgumentOperation? argument = arguments.SingleOrDefault(arg => arg.Parameter?.Name == parameterName);
if (argument is null)
{
return null;
Expand All @@ -270,7 +271,7 @@ static RegexOptions GetRegexOptionsFromArgument(ImmutableArray<IArgumentOperatio
Debug.Assert(parameterName is UpgradeToGeneratedRegexAnalyzer.OptionsArgumentName or UpgradeToGeneratedRegexAnalyzer.PatternArgumentName);
if (parameterName == UpgradeToGeneratedRegexAnalyzer.OptionsArgumentName)
{
string optionsLiteral = Literal(((RegexOptions)(int)argument.Value.ConstantValue.Value).ToString());
string optionsLiteral = Literal(((RegexOptions)(int)argument.Value.ConstantValue.Value!).ToString());
return SyntaxFactory.ParseExpression(optionsLiteral);
}
else if (argument.Value is ILiteralOperation literalOperation)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
<PropertyGroup>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<TargetFramework>$(NetCoreAppCurrent)</TargetFramework>
<DefineConstants>$(DefineConstants);SYSTEM_TEXT_REGULAREXPRESSIONS</DefineConstants>
</PropertyGroup>
<ItemGroup>
<Compile Include="System\Collections\HashtableExtensions.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -547,12 +547,13 @@ public static string ConvertOldStringsToClass(string set, string category)
strLength -= 2;
}

#if REGEXGENERATOR
return StringExtensions.Create
return
#if NETCOREAPP2_1_OR_GREATER
string
#else
return string.Create
StringExtensions
#endif
(strLength, (set, category, startsWithNulls), static (span, state) =>
.Create(strLength, (set, category, startsWithNulls), static (span, state) =>
{
int index;

Expand Down Expand Up @@ -981,7 +982,9 @@ public static bool ParticipatesInCaseConversion(ReadOnlySpan<char> s)
/// <summary>Gets whether the specified span contains only ASCII.</summary>
public static bool IsAscii(ReadOnlySpan<char> s)
{
#if REGEXGENERATOR
#if NET8_0_OR_GREATER
return Ascii.IsValid(s);
#else
foreach (char c in s)
{
if (c >= 128)
Expand All @@ -991,8 +994,6 @@ public static bool IsAscii(ReadOnlySpan<char> s)
}

return true;
#else
return Ascii.IsValid(s);
#endif
}

Expand Down Expand Up @@ -1250,11 +1251,12 @@ static bool InitializeValue(char ch, string set, ref uint[]? asciiLazyCache)
}

uint[]? cache = asciiLazyCache ?? Interlocked.CompareExchange(ref asciiLazyCache, new uint[CacheArrayLength], null) ?? asciiLazyCache;
#if REGEXGENERATOR
InterlockedExtensions.Or(ref cache[ch >> 4], bitsToSet);
#if NET5_0_OR_GREATER
Interlocked
#else
Interlocked.Or(ref cache[ch >> 4], bitsToSet);
InterlockedExtensions
#endif
.Or(ref cache[ch >> 4], bitsToSet);

// Return the computed value.
return isInClass;
Expand Down Expand Up @@ -1542,12 +1544,13 @@ internal static unsafe string CharsToStringClass(ReadOnlySpan<char> chars)
// Get the pointer/length of the span to be able to pass it into string.Create.
#pragma warning disable CS8500 // takes address of managed type
ReadOnlySpan<char> tmpChars = chars; // avoid address exposing the span and impacting the other code in the method that uses it
#if REGEXGENERATOR
return StringExtensions.Create(
return
#if NETCOREAPP2_1_OR_GREATER
string
#else
return string.Create(
StringExtensions
#endif
SetStartIndex + count, (IntPtr)(&tmpChars), static (span, charsPtr) =>
.Create(SetStartIndex + count, (IntPtr)(&tmpChars), static (span, charsPtr) =>
{
// Fill in the set string
span[FlagsIndex] = (char)0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@
namespace System.Text.RegularExpressions
{
[Flags]
#if REGEXGENERATOR
internal
#else
#if SYSTEM_TEXT_REGULAREXPRESSIONS
public
#else
internal
#endif
enum RegexOptions
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ namespace System.Text.RegularExpressions
/// <remarks>
/// This information is made available through <see cref="RegexParseException.Error"/>.
/// </remarks>
#if REGEXGENERATOR
internal
#else
#if SYSTEM_TEXT_REGULAREXPRESSIONS
public
#else
internal
#endif
enum RegexParseError
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ namespace System.Text.RegularExpressions
/// detailed information in the <see cref="Error"/> and <see cref="Offset"/> properties.
/// </summary>
[Serializable]
#if REGEXGENERATOR
internal
#else
#if SYSTEM_TEXT_REGULAREXPRESSIONS
public
#else
internal
#endif
sealed class RegexParseException : ArgumentException
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2076,7 +2076,7 @@ internal static int MapCaptureNumber(int capnum, Hashtable? caps) =>
// ' a b c d e f g h i j k l m n o p q r s t u v w x y z { | } ~
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, Q, S, 0, 0, 0};

#if NET7_0_OR_GREATER
#if NET8_0_OR_GREATER
private static readonly IndexOfAnyValues<char> s_metachars =
IndexOfAnyValues.Create("\t\n\f\r #$()*+.?[\\^{|");

Expand Down
Loading

0 comments on commit 11ab33a

Please sign in to comment.