Make keyword state in parser immutable (#8234)

333fred · web-flow · commit 13534f70a521 · 2023-02-09T08:54:41.000-08:00
* Make keyword state in parser immutable

Splitting this simpler refactoring out from a larger immutability refactoring: there is no good reason for the keywords used in a given parse context to be mutable. They're used immutably in every real context. As part of this, I've added a reference to System.Collections.Immutable to the language dll, and we will make more use of the immutable collections in future refactorings.
diff --git a/src/Compiler/Directory.Packages.props b/src/Compiler/Directory.Packages.props
@@ -29,5 +29,6 @@
     <PackageVersion Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="$(Tooling_MicrosoftCodeAnalysisNetAnalyzersPackageVersion)" NoWarn="NU1608" />
     <PackageVersion Include="Microsoft.CodeAnalysis.BannedApiAnalyzers" Version="$(Tooling_MicrosoftCodeAnalysisBannedApiAnalyzersPackageVersion)" />
     <PackageVersion Include="Roslyn.Diagnostics.Analyzers" Version="$(Tooling_RoslynDiagnosticsAnalyzersPackageVersion)" />
+    <PackageVersion Include="System.Collections.Immutable" Version="$(SystemCollectionsImmutablePackageVersion)" />
   </ItemGroup>
 </Project>
diff --git a/src/Compiler/Microsoft.AspNetCore.Razor.Language/src/Legacy/CSharpCodeParser.cs b/src/Compiler/Microsoft.AspNetCore.Razor.Language/src/Legacy/CSharpCodeParser.cs
@@ -5,6 +5,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Collections.Immutable;
 using System.Diagnostics;
 using System.Linq;
 using Microsoft.AspNetCore.Razor.Language.Syntax.InternalSyntax;
@@ -48,8 +49,7 @@ internal class CSharpCodeParser : TokenizerBackedParser<CSharpTokenizer>
             builder.Description = Resources.TagHelperPrefixDirective_Description;
         });
 
-    internal static ISet<string> DefaultKeywords = new HashSet<string>()
-        {
+    internal static ImmutableHashSet<string> DefaultKeywords = ImmutableHashSet.Create(
             SyntaxConstants.CSharp.TagHelperPrefixKeyword,
             SyntaxConstants.CSharp.AddTagHelperKeyword,
             SyntaxConstants.CSharp.RemoveTagHelperKeyword,
@@ -65,12 +65,12 @@ internal class CSharpCodeParser : TokenizerBackedParser<CSharpTokenizer>
             "namespace",
             "class",
             "where"
-        };
+         );
 
-    private readonly ISet<string> CurrentKeywords = new HashSet<string>(DefaultKeywords);
+    private readonly ImmutableHashSet<string> CurrentKeywords;
 
-    private readonly Dictionary<CSharpKeyword, Action<SyntaxListBuilder<RazorSyntaxNode>, CSharpTransitionSyntax>> _keywordParserMap = new Dictionary<CSharpKeyword, Action<SyntaxListBuilder<RazorSyntaxNode>, CSharpTransitionSyntax>>();
-    private readonly Dictionary<string, Action<SyntaxListBuilder<RazorSyntaxNode>, CSharpTransitionSyntax>> _directiveParserMap = new Dictionary<string, Action<SyntaxListBuilder<RazorSyntaxNode>, CSharpTransitionSyntax>>(StringComparer.Ordinal);
+    private readonly ImmutableDictionary<CSharpKeyword, Action<SyntaxListBuilder<RazorSyntaxNode>, CSharpTransitionSyntax>> _keywordParserMap;
+    private readonly ImmutableDictionary<string, Action<SyntaxListBuilder<RazorSyntaxNode>, CSharpTransitionSyntax>> _directiveParserMap;
 
     public CSharpCodeParser(ParserContext context)
         : this(directives: Enumerable.Empty<DirectiveDescriptor>(), context: context)
@@ -90,15 +90,103 @@ public CSharpCodeParser(IEnumerable<DirectiveDescriptor> directives, ParserConte
             throw new ArgumentNullException(nameof(context));
         }
 
-        Keywords = new HashSet<string>();
+        var keywordsBuilder = ImmutableHashSet<string>.Empty.ToBuilder();
+        var keywordParserMapBuilder = ImmutableDictionary<CSharpKeyword, Action<SyntaxListBuilder<RazorSyntaxNode>, CSharpTransitionSyntax>>.Empty.ToBuilder();
+        var currentKeywordsBuilder = DefaultKeywords.ToBuilder();
+        var directiveParserMapBuilder = ImmutableDictionary.CreateBuilder<string, Action<SyntaxListBuilder<RazorSyntaxNode>, CSharpTransitionSyntax>>(StringComparer.Ordinal);
+
         SetupKeywordParsers();
         SetupExpressionParsers();
         SetupDirectiveParsers(directives);
+
+        Keywords = keywordsBuilder.ToImmutable();
+        CurrentKeywords = currentKeywordsBuilder.ToImmutable();
+        _keywordParserMap = keywordParserMapBuilder.ToImmutable();
+        _directiveParserMap = directiveParserMapBuilder.ToImmutable();
+
+        void SetupKeywordParsers()
+        {
+            MapKeywords(ParseConditionalBlock, topLevel: true, CSharpKeyword.For, CSharpKeyword.Foreach, CSharpKeyword.While, CSharpKeyword.Switch, CSharpKeyword.Lock);
+            MapKeywords(ParseCaseStatement, topLevel: false, CSharpKeyword.Case, CSharpKeyword.Default);
+            MapKeywords(ParseIfStatement, topLevel: true, CSharpKeyword.If);
+            MapKeywords(ParseTryStatement, topLevel: true, CSharpKeyword.Try);
+            MapKeywords(ParseDoStatement, topLevel: true, CSharpKeyword.Do);
+            MapKeywords(ParseUsingKeyword, topLevel: true, CSharpKeyword.Using);
+        }
+
+        void MapKeywords(
+            Action<SyntaxListBuilder<RazorSyntaxNode>, CSharpTransitionSyntax> handler,
+            bool topLevel,
+            params CSharpKeyword[] keywords)
+        {
+            foreach (var keyword in keywords)
+            {
+                keywordParserMapBuilder.Add(keyword, handler);
+                if (topLevel)
+                {
+                    keywordsBuilder.Add(CSharpLanguageCharacteristics.GetKeyword(keyword));
+                }
+            }
+        }
+
+        void SetupExpressionParsers()
+        {
+            keywordParserMapBuilder.Add(CSharpKeyword.Await, ParseAwaitExpression);
+        }
+
+        void SetupDirectiveParsers(IEnumerable<DirectiveDescriptor> directiveDescriptors)
+        {
+            foreach (var directiveDescriptor in directiveDescriptors)
+            {
+                currentKeywordsBuilder.Add(directiveDescriptor.Directive);
+                MapDirectives((builder, transition) => ParseExtensibleDirective(builder, transition, directiveDescriptor), directiveParserMapBuilder, keywordsBuilder, context, directiveDescriptor.Directive);
+            }
+
+            MapDirectives(ParseTagHelperPrefixDirective, directiveParserMapBuilder, keywordsBuilder, context, SyntaxConstants.CSharp.TagHelperPrefixKeyword);
+            MapDirectives(ParseAddTagHelperDirective, directiveParserMapBuilder, keywordsBuilder, context, SyntaxConstants.CSharp.AddTagHelperKeyword);
+            MapDirectives(ParseRemoveTagHelperDirective, directiveParserMapBuilder, keywordsBuilder, context, SyntaxConstants.CSharp.RemoveTagHelperKeyword);
+
+            // If there wasn't any extensible directives relating to the reserved directives then map them.
+            if (!directiveParserMapBuilder.ContainsKey("class"))
+            {
+                MapDirectives(ParseReservedDirective, directiveParserMapBuilder, keywordsBuilder, context, "class");
+            }
+
+            if (!directiveParserMapBuilder.ContainsKey("namespace"))
+            {
+                MapDirectives(ParseReservedDirective, directiveParserMapBuilder, keywordsBuilder, context, "namespace");
+            }
+        }
+
+        static void MapDirectives(
+            Action<SyntaxListBuilder<RazorSyntaxNode>, CSharpTransitionSyntax> handler,
+            ImmutableDictionary<string, Action<SyntaxListBuilder<RazorSyntaxNode>, CSharpTransitionSyntax>>.Builder directiveParserMap,
+            ImmutableHashSet<string>.Builder keywords,
+            ParserContext context,
+            params string[] directives)
+        {
+            foreach (var directive in directives)
+            {
+                if (directiveParserMap.ContainsKey(directive))
+                {
+                    // It is possible for the list to contain duplicates in cases when the project is misconfigured.
+                    // In those cases, we shouldn't register multiple handlers per keyword.
+                    continue;
+                }
+
+                directiveParserMap.Add(directive, (builder, transition) =>
+                {
+                    handler(builder, transition);
+                    context.SeenDirectives.Add(directive);
+                });
+                keywords.Add(directive);
+            }
+        }
     }
 
     public HtmlMarkupParser HtmlParser { get; set; }
 
-    protected internal ISet<string> Keywords { get; private set; }
+    protected internal ImmutableHashSet<string> Keywords { get; private set; }
 
     public bool IsNested { get; set; }
 
@@ -908,30 +996,6 @@ protected bool TryParseDirective(in SyntaxListBuilder<RazorSyntaxNode> builder,
         return false;
     }
 
-    private void SetupDirectiveParsers(IEnumerable<DirectiveDescriptor> directiveDescriptors)
-    {
-        foreach (var directiveDescriptor in directiveDescriptors)
-        {
-            CurrentKeywords.Add(directiveDescriptor.Directive);
-            MapDirectives((builder, transition) => ParseExtensibleDirective(builder, transition, directiveDescriptor), directiveDescriptor.Directive);
-        }
-
-        MapDirectives(ParseTagHelperPrefixDirective, SyntaxConstants.CSharp.TagHelperPrefixKeyword);
-        MapDirectives(ParseAddTagHelperDirective, SyntaxConstants.CSharp.AddTagHelperKeyword);
-        MapDirectives(ParseRemoveTagHelperDirective, SyntaxConstants.CSharp.RemoveTagHelperKeyword);
-
-        // If there wasn't any extensible directives relating to the reserved directives then map them.
-        if (!_directiveParserMap.ContainsKey("class"))
-        {
-            MapDirectives(ParseReservedDirective, "class");
-        }
-
-        if (!_directiveParserMap.ContainsKey("namespace"))
-        {
-            MapDirectives(ParseReservedDirective, "namespace");
-        }
-    }
-
     private void EnsureDirectiveIsAtStartOfLine()
     {
         // 1 is the offset of the @ transition for the directive.
@@ -955,28 +1019,6 @@ private void EnsureDirectiveIsAtStartOfLine()
         }
     }
 
-    // Internal for testing.
-    internal void MapDirectives(Action<SyntaxListBuilder<RazorSyntaxNode>, CSharpTransitionSyntax> handler, params string[] directives)
-    {
-        foreach (var directive in directives)
-        {
-            if (_directiveParserMap.ContainsKey(directive))
-            {
-                // It is possible for the list to contain duplicates in cases when the project is misconfigured.
-                // In those cases, we shouldn't register multiple handlers per keyword.
-                continue;
-            }
-
-            _directiveParserMap.Add(directive, (builder, transition) =>
-            {
-                handler(builder, transition);
-                Context.SeenDirectives.Add(directive);
-            });
-
-            Keywords.Add(directive);
-        }
-    }
-
     private void ParseTagHelperPrefixDirective(SyntaxListBuilder<RazorSyntaxNode> builder, CSharpTransitionSyntax transition)
     {
         RazorDiagnostic duplicateDiagnostic = null;
@@ -1791,51 +1833,6 @@ private bool AtBooleanLiteral()
         return result.HasValue && (result.Value == CSharpKeyword.True || result.Value == CSharpKeyword.False);
     }
 
-    private void SetupExpressionParsers()
-    {
-        MapExpressionKeyword(ParseAwaitExpression, CSharpKeyword.Await);
-    }
-
-    private void SetupKeywordParsers()
-    {
-        MapKeywords(
-            ParseConditionalBlock,
-            CSharpKeyword.For,
-            CSharpKeyword.Foreach,
-            CSharpKeyword.While,
-            CSharpKeyword.Switch,
-            CSharpKeyword.Lock);
-        MapKeywords(ParseCaseStatement, false, CSharpKeyword.Case, CSharpKeyword.Default);
-        MapKeywords(ParseIfStatement, CSharpKeyword.If);
-        MapKeywords(ParseTryStatement, CSharpKeyword.Try);
-        MapKeywords(ParseDoStatement, CSharpKeyword.Do);
-        MapKeywords(ParseUsingKeyword, CSharpKeyword.Using);
-    }
-
-    private void MapExpressionKeyword(Action<SyntaxListBuilder<RazorSyntaxNode>, CSharpTransitionSyntax> handler, CSharpKeyword keyword)
-    {
-        _keywordParserMap.Add(keyword, handler);
-
-        // Expression keywords don't belong in the regular keyword list
-    }
-
-    private void MapKeywords(Action<SyntaxListBuilder<RazorSyntaxNode>, CSharpTransitionSyntax> handler, params CSharpKeyword[] keywords)
-    {
-        MapKeywords(handler, topLevel: true, keywords: keywords);
-    }
-
-    private void MapKeywords(Action<SyntaxListBuilder<RazorSyntaxNode>, CSharpTransitionSyntax> handler, bool topLevel, params CSharpKeyword[] keywords)
-    {
-        foreach (var keyword in keywords)
-        {
-            _keywordParserMap.Add(keyword, handler);
-            if (topLevel)
-            {
-                Keywords.Add(CSharpLanguageCharacteristics.GetKeyword(keyword));
-            }
-        }
-    }
-
     private void ParseAwaitExpression(SyntaxListBuilder<RazorSyntaxNode> builder, CSharpTransitionSyntax transition)
     {
         // Ensure that we're on the await statement (only runs in debug)
diff --git a/src/Compiler/Microsoft.AspNetCore.Razor.Language/src/Legacy/ImplicitExpressionEditHandler.cs b/src/Compiler/Microsoft.AspNetCore.Razor.Language/src/Legacy/ImplicitExpressionEditHandler.cs
@@ -5,6 +5,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Collections.Immutable;
 using System.Diagnostics;
 using System.Globalization;
 using System.IO;
@@ -16,30 +17,16 @@ namespace Microsoft.AspNetCore.Razor.Language.Legacy;
 
 internal class ImplicitExpressionEditHandler : SpanEditHandler
 {
-    private readonly ISet<string> _keywords;
-    private readonly IReadOnlyCollection<string> _readOnlyKeywords;
-
-    public ImplicitExpressionEditHandler(Func<string, IEnumerable<Syntax.InternalSyntax.SyntaxToken>> tokenizer, ISet<string> keywords, bool acceptTrailingDot)
+    public ImplicitExpressionEditHandler(Func<string, IEnumerable<Syntax.InternalSyntax.SyntaxToken>> tokenizer, ImmutableHashSet<string> keywords, bool acceptTrailingDot)
         : base(tokenizer)
     {
-        _keywords = keywords ?? new HashSet<string>();
-
-        // HashSet<T> implements IReadOnlyCollection<T> as of 4.6, but does not for 4.5.1. If the runtime cast
-        // succeeds, avoid creating a new collection.
-        _readOnlyKeywords = (_keywords as IReadOnlyCollection<string>) ?? _keywords.ToArray();
-
+        Keywords = keywords;
         AcceptTrailingDot = acceptTrailingDot;
     }
 
     public bool AcceptTrailingDot { get; }
 
-    public IReadOnlyCollection<string> Keywords
-    {
-        get
-        {
-            return _readOnlyKeywords;
-        }
-    }
+    public ImmutableHashSet<string> Keywords { get; }
 
     public override string ToString()
     {
@@ -621,7 +608,7 @@ private bool StartsWithKeyword(string newContent)
     {
         using (var reader = new StringReader(newContent))
         {
-            return _keywords.Contains(reader.ReadWhile(ParserHelpers.IsIdentifierPart));
+            return Keywords.Contains(reader.ReadWhile(ParserHelpers.IsIdentifierPart));
         }
     }
 }
diff --git a/src/Compiler/Microsoft.AspNetCore.Razor.Language/src/Microsoft.AspNetCore.Razor.Language.csproj b/src/Compiler/Microsoft.AspNetCore.Razor.Language/src/Microsoft.AspNetCore.Razor.Language.csproj
@@ -10,6 +10,7 @@
 
   <ItemGroup>
     <ProjectReference Include="$(SharedSourceRoot)\Microsoft.AspNetCore.Razor.LanguageSupport\Microsoft.AspNetCore.Razor.LanguageSupport.csproj" />
+    <PackageReference Include="System.Collections.Immutable" />
   </ItemGroup>
 
 </Project>
diff --git a/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/Legacy/CSharpCodeParserTest.cs b/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/Legacy/CSharpCodeParserTest.cs
@@ -220,10 +220,12 @@ public void MapDirectives_HandlesDuplicates()
         var source = TestRazorSourceDocument.Create();
         var options = RazorParserOptions.CreateDefault();
         var context = new ParserContext(source, options);
-        var parser = new CSharpCodeParser(context);
 
         // Act & Assert (Does not throw)
-        parser.MapDirectives((b, t) => { }, "test");
-        parser.MapDirectives((b, t) => { }, "test");
+        var directiveDescriptors = new[] {
+            DirectiveDescriptor.CreateDirective("test", DirectiveKind.SingleLine),
+            DirectiveDescriptor.CreateDirective("test", DirectiveKind.SingleLine),
+        };
+        var parser = new CSharpCodeParser(directiveDescriptors, context);
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -5,6 +5,7 @@`
`5`	`5`
`6`	`6`	`using System;`
`7`	`7`	`using System.Collections.Generic;`
	`8`	`+using System.Collections.Immutable;`
`8`	`9`	`using System.Diagnostics;`
`9`	`10`	`using System.Globalization;`
`10`	`11`	`using System.IO;`
`@@ -16,30 +17,16 @@ namespace Microsoft.AspNetCore.Razor.Language.Legacy;`
`16`	`17`
`17`	`18`	`internal class ImplicitExpressionEditHandler : SpanEditHandler`
`18`	`19`	`{`
`19`		`- private readonly ISet<string> _keywords;`
`20`		`- private readonly IReadOnlyCollection<string> _readOnlyKeywords;`
`21`		`-`
`22`		`- public ImplicitExpressionEditHandler(Func<string, IEnumerable<Syntax.InternalSyntax.SyntaxToken>> tokenizer, ISet<string> keywords, bool acceptTrailingDot)`
	`20`	`+ public ImplicitExpressionEditHandler(Func<string, IEnumerable<Syntax.InternalSyntax.SyntaxToken>> tokenizer, ImmutableHashSet<string> keywords, bool acceptTrailingDot)`
`23`	`21`	`: base(tokenizer)`
`24`	`22`	`{`
`25`		`- _keywords = keywords ?? new HashSet<string>();`
`26`		`-`
`27`		`- // HashSet<T> implements IReadOnlyCollection<T> as of 4.6, but does not for 4.5.1. If the runtime cast`
`28`		`- // succeeds, avoid creating a new collection.`
`29`		`- _readOnlyKeywords = (_keywords as IReadOnlyCollection<string>) ?? _keywords.ToArray();`
`30`		`-`
	`23`	`+ Keywords = keywords;`
`31`	`24`	`AcceptTrailingDot = acceptTrailingDot;`
`32`	`25`	`}`
`33`	`26`
`34`	`27`	`public bool AcceptTrailingDot { get; }`
`35`	`28`
`36`		`- public IReadOnlyCollection<string> Keywords`
`37`		`- {`
`38`		`- get`
`39`		`- {`
`40`		`- return _readOnlyKeywords;`
`41`		`- }`
`42`		`- }`
	`29`	`+ public ImmutableHashSet<string> Keywords { get; }`
`43`	`30`
`44`	`31`	`public override string ToString()`
`45`	`32`	`{`
`@@ -621,7 +608,7 @@ private bool StartsWithKeyword(string newContent)`
`621`	`608`	`{`
`622`	`609`	`using (var reader = new StringReader(newContent))`
`623`	`610`	`{`
`624`		`- return _keywords.Contains(reader.ReadWhile(ParserHelpers.IsIdentifierPart));`
	`611`	`+ return Keywords.Contains(reader.ReadWhile(ParserHelpers.IsIdentifierPart));`
`625`	`612`	`}`
`626`	`613`	`}`
`627`	`614`	`}`