Razor parser rewrite #2590

ajaybhargavb · 2018-09-12T23:24:43Z

#2579, #2580, #2581, #2584, #2582, #2587, #2585, #2700, #2689

Rewrite CSharp parser
Basic rewrite of HTML parser (More improvements to follow)
Define and generate syntax nodes and boilerplate
Rewrite ClassifiedSpan and TagHelperSpan generation logic
Rewrite TagHelper phase
Rewrite Intermediate phase
Rewrite other miscellaneous features and bug fixes
Rewrite partial parsing
Added some syntax manipulation APIs
Removed unused legacy types
Update parser test infrastructure
Update tests
Regenerated baselines

NTaylorMullen · 2018-09-13T00:23:02Z

😲

ajaybhargavb · 2018-09-13T04:36:21Z

It just looks that way because I've duplicated the Html and CSharp parser code instead of editing it directly to keep things building. Reviewing this shouldn't be as painful as it seems but anyways don't bother reviewing this too hard just yet. I suggest skimming it just to look for red flags if any. It will be much easier to review later on when things are cleaner.

This is now ready for review

src/Microsoft.AspNetCore.Razor.Language/DefaultRazorIntermediateNodeLoweringPhase.cs

ajaybhargavb · 2018-11-13T23:15:25Z

src/Microsoft.AspNetCore.Razor.Language/DefaultRazorIntermediateNodeLoweringPhase.cs

                }
-                else
+                else if (context.ChunkGenerator is DirectiveTokenChunkGenerator tokenChunkGenerator)


Yes. Intermediate phase is still dependent on ChunkGenerators because the old code was heavily dependent on it. I plan to remove it in the future. #ByDesign

ajaybhargavb · 2018-11-13T23:18:36Z

src/Microsoft.AspNetCore.Razor.Language/DefaultRazorIntermediateNodeLoweringPhase.cs

+                {
+                    if (node.Value != null && node.Value.ChildNodes().All(c => c is MarkupLiteralAttributeValueSyntax))
+                    {
+                        // We need to do what ConditionalAttributeCollapser used to do.


Because we got rid of ConditionalAttributeCollapser, I need this logic to merge contiguous MarkupLiterals so we generate a single Intermediate node for all of it. I'll change the above comment to explain this. #ByDesign

ajaybhargavb · 2018-11-13T23:20:46Z

src/Microsoft.AspNetCore.Razor.Language/Legacy/AddImportChunkGenerator.cs

@@ -14,23 +14,6 @@ public AddImportChunkGenerator(string ns)

        public string Namespace { get; }

-        public override void Accept(ParserVisitor visitor, Span span)


ChunkGenerators don't really do anything anymore other than be a marker. #ByDesign

src/Microsoft.AspNetCore.Razor.Language/Legacy/EditResult.cs

ajaybhargavb · 2018-11-13T23:30:34Z

src/Microsoft.AspNetCore.Razor.Language/Legacy/TagHelperBlockRewriter.cs

        }

-        private static void ConfigureNonStringAttribute(SpanBuilder builder, bool isDuplicateAttribute)
+        private class AttributeValueRewriter : SyntaxRewriter


Whatever is in it is probably ugly. TagHelperBlockRewriter used to do a whole bunch of "special" rewriting of attribute values. Keeping the same behavior while operating on the new tree makes it really ugly. I plan to clean this up as much as possible as part of attribute value syntax improvements. #ByDesign

Awesomeness!

ajaybhargavb · 2018-11-13T23:32:09Z

src/Microsoft.AspNetCore.Razor.Language/Syntax/ChildSyntaxList.cs

+
+namespace Microsoft.AspNetCore.Razor.Language.Syntax
+{
+    internal readonly struct ChildSyntaxList : IEquatable<ChildSyntaxList>, IReadOnlyList<SyntaxNode>


From Roslyn #ByDesign

ajaybhargavb · 2018-11-14T00:02:12Z

src/Microsoft.AspNetCore.Razor.Language/Syntax/MarkupMinimizedTagHelperAttributeSyntax.cs

+    {
+        private static readonly string TagHelperAttributeInfoKey = typeof(TagHelperAttributeInfo).Name;
+
+        public TagHelperAttributeInfo TagHelperAttributeInfo


Added Metadata about the node as first class properties instead on an annotation. #ByDesign

ajaybhargavb · 2018-11-14T00:04:18Z

src/Microsoft.AspNetCore.Razor.Language/Syntax/SyntaxNode.Iterators.cs

+
+namespace Microsoft.AspNetCore.Razor.Language.Syntax
+{
+    internal abstract partial class SyntaxNode


From Roslyn #ByDesign

ajaybhargavb · 2018-11-14T00:05:14Z

src/Microsoft.AspNetCore.Razor.Language/Syntax/SyntaxNodeExtensions.cs

+            return content;
+        }
+
+        public static string GetTagName(this MarkupTagBlockSyntax tagBlock)


These are currently extension methods. But these will go away once the structure of a Tag Block is improved. #ByDesign

ajaybhargavb · 2018-11-14T00:05:43Z

src/Microsoft.AspNetCore.Razor.Language/Syntax/SyntaxReplacer.cs

+
+namespace Microsoft.AspNetCore.Razor.Language.Syntax
+{
+    internal static class SyntaxReplacer


From Roslyn #ByDesign

ajaybhargavb · 2018-11-14T00:06:07Z

src/Microsoft.AspNetCore.Razor.Language/Syntax/SyntaxRewriter.cs

+
+namespace Microsoft.AspNetCore.Razor.Language.Syntax
+{
+    internal abstract partial class SyntaxRewriter : SyntaxVisitor<SyntaxNode>


From Roslyn #ByDesign

ajaybhargavb · 2018-11-14T00:08:07Z

src/Microsoft.AspNetCore.Razor.Language/Syntax/InternalSyntax/SyntaxListPool.cs

+
+namespace Microsoft.AspNetCore.Razor.Language.Syntax.InternalSyntax
+{
+    internal class SyntaxListPool


From Roslyn #ByDesign

src/Microsoft.AspNetCore.Razor.Language/TagHelperSpanVisitor.cs

src/Microsoft.AspNetCore.Razor.Language/Legacy/IParentChunkGenerator.cs

rynowak · 2018-11-14T20:58:30Z

src/Microsoft.AspNetCore.Razor.Language/Legacy/SpanContext.cs

+
+namespace Microsoft.AspNetCore.Razor.Language.Legacy
+{
+    internal class SpanContext


SpanContext [](start = 19, length = 11)

I see that this is in the .Leygacy namespace which is OK. I'm hoping that this is somewhat of a temporary bridge between old code and new code.

Yes but they won't go away anytime soon because we don't have a plan for getting rid of EditHandlers

In reply to: 233617210 [](ancestors = 233617210)

src/Microsoft.AspNetCore.Razor.Language/Legacy/TagHelperAttributeInfo.cs

src/Microsoft.AspNetCore.Razor.Language/Syntax/LegacySyntaxNodeExtensions.cs

rynowak · 2018-11-14T21:05:07Z

src/Microsoft.AspNetCore.Razor.Language/Syntax/MarkupElementRewriter.cs

+
+namespace Microsoft.AspNetCore.Razor.Language.Syntax
+{
+    internal static class MarkupElementRewriter


MarkupElementRewriter [](start = 26, length = 21)

I'm assuming that this is also temporary (this is the code that groups elements/tags).

Yes but I didn't put it in Legacy because this operates completely on the new infrastructure and doesn't use any of our legacy components.

In reply to: 233619189 [](ancestors = 233619189)

src/Microsoft.AspNetCore.Razor.Language/Syntax/SyntaxNode.cs

rynowak · 2018-11-14T21:08:15Z

src/Microsoft.AspNetCore.Razor.Language/Syntax/SyntaxNodeExtensions.cs

+                if (source.Length == 0)
+                {
+                    // Just a marker symbol
+                    return new SourceLocation(source.FilePath, 0, 0, 0);


Is this really correct? Don't marker symbols still have a location? #ByDesign

The answer is yes for both questions. This is the case when the document is empty and the only thing we generate is a marker symbol.

In reply to: 233620145 [](ancestors = 233620145)

src/Microsoft.AspNetCore.Razor.Language/ClassifiedSpanVisitor.cs

ajaybhargavb · 2018-11-14T21:37:32Z

src/Microsoft.AspNetCore.Razor.Language/ClassifiedSpanVisitor.cs

+            return WriteBlock(node, BlockKindInternal.Markup, base.VisitMarkupBlock);
+        }
+
+        public override SyntaxNode VisitGenericBlock(GenericBlockSyntax node)


I already regret having a GenericBlock. This is currently only use in attribute values. I'm hoping I can get rid of this when I clean that up.

Yeah there shouldn't be anything generic. Everything should be specific.

NTaylorMullen

Still poking through, just publishing the comments I have so far.

src/Microsoft.AspNetCore.Razor.Language/ClassifiedSpanVisitor.cs

src/Microsoft.AspNetCore.Razor.Language/DefaultRazorIntermediateNodeLoweringPhase.cs

NTaylorMullen

More comments, still looking. Finished tokenizerbackedparser and csharp code parser O_O, they were masssssivveee. It took almost all day, but I was able to finish

src/Microsoft.AspNetCore.Razor.Language/Legacy/TokenizerBackedParser.cs

src/Microsoft.AspNetCore.Razor.Language/Legacy/CSharpCodeParser.cs

src/Microsoft.AspNetCore.Razor.Language/Legacy/TokenizerBackedParser.cs

src/Microsoft.AspNetCore.Razor.Language/Legacy/CSharpCodeParser.cs

src/Microsoft.AspNetCore.Razor.Language/Legacy/TokenizerBackedParser.cs

NTaylorMullen

More comments, starting to look at TagHelper goods. Loving all of the deletions.

src/Microsoft.AspNetCore.Razor.Language/Legacy/HtmlMarkupParser.cs

NTaylorMullen · 2018-11-16T01:32:23Z

src/Microsoft.AspNetCore.Razor.Language/Legacy/HtmlMarkupParser.cs

@@ -19,10 +19,6 @@ internal class HtmlMarkupParser : TokenizerBackedParser<HtmlTokenizer>
            SyntaxFactory.Token(SyntaxKind.Bang, "!"),


Not blocking the PR on this but this class definitely need to be refactored in ways like CSharpCodeParser was. CSharpCodeParser is beaaautiful in terms of its factorings and this could benefit from similar treatment.

Yup, that is the goal 🏁

src/Microsoft.AspNetCore.Razor.Language/Legacy/WhiteSpaceRewriter.cs

ajaybhargavb · 2018-11-16T04:11:55Z

🆙 📅

NTaylorMullen

Not many comments in regards to the TagHelper pieces, pushing through more.

src/Microsoft.AspNetCore.Razor.Language/Legacy/TagHelperBlockRewriter.cs

NTaylorMullen · 2018-11-16T19:27:56Z

src/Microsoft.AspNetCore.Razor.Language/Legacy/TagHelperBlockRewriter.cs

        }

-        private static void ConfigureNonStringAttribute(SpanBuilder builder, bool isDuplicateAttribute)
+        private class AttributeValueRewriter : SyntaxRewriter


Awesomeness!

src/Microsoft.AspNetCore.Razor.Language/Legacy/TagHelperInfo.cs

src/Microsoft.AspNetCore.Razor.Language/Legacy/TagHelperParseTreeRewriter.cs

NTaylorMullen · 2018-11-16T22:21:36Z

I Did it! That was super dope, nothing blocking

ajaybhargavb · 2018-11-17T00:13:36Z

🆙 📅

- Rewrite CSharp parser - Basic rewrite of HTML parser (More improvements to follow) - Define and generate syntax nodes and boilerplate - Rewrite ClassifiedSpan and TagHelperSpan generation logic - Rewrite TagHelper phase - Rewrite Intermediate phase - Rewrite other miscellaneous features and bug fixes - Rewrite partial parsing - Added some syntax manipulation APIs - Removed unused legacy types

- Update parser test infrastructure - Update tests - Regenerated baselines - Removed unused legacy types

ajaybhargavb · 2018-11-17T01:24:03Z

Thanks for the review @NTaylorMullen and @rynowak

ajaybhargavb force-pushed the feature/razor-parser branch from 66ec4b7 to eea6044 Compare September 12, 2018 23:26

ajaybhargavb force-pushed the feature/razor-parser branch 6 times, most recently from edbb012 to 88cca81 Compare September 25, 2018 22:21

ajaybhargavb force-pushed the feature/razor-parser branch 2 times, most recently from f6d1798 to 93ce3f1 Compare September 27, 2018 23:40

ajaybhargavb force-pushed the feature/razor-parser branch from 5148739 to 9a862c6 Compare November 2, 2018 18:30

ajaybhargavb force-pushed the feature/razor-parser branch 3 times, most recently from 64b594f to 85c71b2 Compare November 13, 2018 22:35

ajaybhargavb changed the title ~~[Design] Razor parser rewrite~~ Razor parser rewrite Nov 13, 2018

ajaybhargavb force-pushed the feature/razor-parser branch from 85c71b2 to 2467e0f Compare November 13, 2018 22:50

ajaybhargavb commented Nov 13, 2018

View reviewed changes

src/Microsoft.AspNetCore.Razor.Language/DefaultRazorIntermediateNodeLoweringPhase.cs Outdated Show resolved Hide resolved

ajaybhargavb commented Nov 13, 2018

View reviewed changes

src/Microsoft.AspNetCore.Razor.Language/Legacy/EditResult.cs Show resolved Hide resolved

ajaybhargavb commented Nov 13, 2018

View reviewed changes

ajaybhargavb commented Nov 14, 2018

View reviewed changes

ajaybhargavb assigned NTaylorMullen Nov 14, 2018