Added MarkupElementRewriter #2624

ajaybhargavb · 2018-10-01T20:04:51Z

This is going to be run before and after the tag helper phase. I am currently not adding any errors at this level. All errors will be added during the tag helper phase. Sending this as a separate PR just to make sure everyone is on the same page.
Added tests

ajaybhargavb · 2018-10-01T20:05:14Z

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

+{
+    internal class MarkupElementRewriter
+    {
+        public static RazorSyntaxTree Rewrite(RazorSyntaxTree syntaxTree)


This is called before tag helper phase.

Names are wacky here because there are two rewritiers. Suggest something like AddElementNodes RemoveElementNodes

ajaybhargavb · 2018-10-01T20:05:26Z

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

+            return newSyntaxTree;
+        }
+
+        public static RazorSyntaxTree RemoveMarkupElement(RazorSyntaxTree syntaxTree)


This is called after tag helper phase.

ajaybhargavb · 2018-10-01T20:06:45Z

test/Microsoft.AspNetCore.Razor.Language.Test/Legacy/MarkupElementRewriterTest.cs

+            BaselineTest(rewritten);
+
+            var unrewritten = MarkupElementRewriter.RemoveMarkupElement(rewritten);
+            Assert.Equal(syntaxTree.Root.SerializedValue, unrewritten.Root.SerializedValue);


Each test will also run the removal phase and make sure the output matches the original syntax tree. It didn't make sense to add separate tests for the removal phase.

ajaybhargavb · 2018-10-01T20:08:19Z

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

+                        continue;
+                    }
+
+                    var tagName = tagBlock.GetTagName();


GetTagName() is an extension method to MarkupTagBlockSyntax. This will change in the future when tag block have a first class TagName child.

pranavkm · 2018-10-01T20:25:43Z

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

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


static class?

pranavkm · 2018-10-01T20:32:50Z

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

+
+            private bool IsVoidElement(MarkupTagBlockSyntax tagBlock)
+            {
+                return VoidElements.Contains(tagBlock.GetTagName(), StringComparer.OrdinalIgnoreCase);


This is doing Enumerable.Contains. Did you mean to do VoidElements.Contains(tagBlock.GetTagName()) since the hashset is already using an OrdinalIgnoreCase comparer?

pranavkm · 2018-10-01T20:35:03Z

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

+                return VoidElements.Contains(tagBlock.GetTagName(), StringComparer.OrdinalIgnoreCase);
+            }
+
+            private bool IsSelfClosing(MarkupTagBlockSyntax tagBlock)


I'm guessing this is is a end tag like </> but don't see a corresponding test. Should this be unit tested?

There is a corresponding test. https://github.com/aspnet/Razor/pull/2624/files#diff-8dfd8aeca4f15b18bb45a38795fd1c02R94

Wouldn't either of those have one or more child nodes?

They can't have any child nodes.

pranavkm · 2018-10-01T20:36:13Z

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

+            {
+                var lastChild = tagBlock.ChildNodes().LastOrDefault();
+
+                return lastChild?.GetContent().EndsWith("/>", StringComparison.Ordinal) ?? false;


Out of curiosity, why does IsEndTag use literal tokens and this use GetContent()?

IsEndTag was a copy-paste-modify. I was wondering the same thing as why is it doing all the weird index stuff. But I left it as is for now.

pranavkm · 2018-10-01T20:38:34Z

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

+                return false;
+            }
+
+            private SyntaxNode RewriteMalformedTags(SyntaxNode parent, int malformedTagCount)


This method is invoked when malformedTagCount != _startTagTracker.Count, but reading the code it looks like _startTagTracker.Count must be > malformedTagCount (since it's popped that many times). Does the if need to be changed?

Sure. It doesn't functionally change anything but I'll change the if.

pranavkm · 2018-10-01T20:44:52Z

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

+                        node = RewriteNodeCore(node, startTag: tagBlock, tagChildren: Enumerable.Empty<RazorSyntaxNode>(), endTag: null);
+
+                        // Since we rewrote 'node', it's children are different. Update our collection.
+                        children = node.ChildNodes();


How does this work? Should this restart from the beginning since the children are different or continue from the next index because this node is already observed?

It won't restart. It just continues from where it left off. I still have to update the collection because rewriting node changes the children.

What if instead you created a new list of children and then rewrote it once.

So like:

A B C D

Iterate through the children of A - (B, C, D) and rewrite just those nodes as needed so you end up with (B, C!, D) (assume C was rewritten). Then rewrite A once to replace its children. That's how I would expect something like this to work.

The problem is that when I rewrite (B, C, D) => (B, C!, D), it actually becomes (B, C!, D!) because everything after a rewritten node is different because red nodes are created on demand

Can you explain this in more detail? I'm not sure that I get it.

Sorry I missed this part "created a new list of children". What I said doesn't apply in that case. I'll give this a shot

pranavkm · 2018-10-01T20:45:01Z

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

+
+            public Rewriter()
+            {
+                _startTagTracker = new Stack<TagBlockTracker>();


pranavkm · 2018-10-01T20:46:07Z

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

+                MarkupTagBlockSyntax endTag)
+            {
+                var body = new SyntaxList<RazorSyntaxNode>(tagChildren);
+                var markupElement = SyntaxFactory.MarkupElement((MarkupTagBlockSyntax)startTag, body, endTag);


Can this be initialized inline (in the lambda)?

pranavkm · 2018-10-01T20:46:31Z

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

+                }
+
+                // Replace nodes
+                var rewrittenElement = parent.ReplaceNodes(originalNodes, (original, rewritten) => {


Formatting. Also local functions for 💯 points

pranavkm · 2018-10-01T20:48:02Z

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

+
+                var currentIndex = 0;
+                var children = node.ChildNodes();
+                while (currentIndex < children.Count)


for (var i= 0; i < children.Count; i++) works really well here.

ajaybhargavb · 2018-10-02T16:59:03Z

Ping @NTaylorMullen @rynowak @pranavkm

rynowak · 2018-10-02T17:41:15Z

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

+            return newSyntaxTree;
+        }
+
+        private class Rewriter : SyntaxRewriter


Is there precendent for any of this? Like is this based on something or did you write it fresh?

https://github.com/aspnet/Blazor/blob/master/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/ComponentDocumentRewritePass.cs FYI

Is there precendent for any of this? Like is this based on something or did you write it fresh?

This is mostly fresh. Some of the malformed tag handling logic is very similar to https://github.com/aspnet/Razor/blob/master/src/Microsoft.AspNetCore.Razor.Language/Legacy/TagHelperParseTreeRewriter.cs#L730

rynowak · 2018-10-02T17:41:31Z

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

+                if (node != null)
+                {
+                    node = RewriteNode(node);
+                }


Can a node ever be null?

Yes. Visit() is called on every child and Visit() calls node.Accept(this) if the node is not null.

rynowak · 2018-10-02T17:44:10Z

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

+                    node = RewriteNode(node);
+                }
+
+                return base.Visit(node);


Does it make sense for this to be a preorder traversal instead? https://github.com/aspnet/Blazor/blob/master/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/ComponentDocumentRewritePass.cs#L55

I'll take a look

That works as well

rynowak · 2018-10-02T17:51:06Z

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

+                {
+                    // Nothing to replace
+                    return parent;
+                }


Does this preserve order correctly? What about a case like:

<div>Hi</div> @Something! <div>bye</div>

I see, you did the below.

I think this would make a little more sense if you mutated originalNodes in place and preserved ordering while doing so.

rynowak · 2018-10-02T17:54:07Z

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

+                    return true;
+                }
+
+                // Could not recover tag. Aka we found an end tag without a corresponding start tag.


Make sure you have a test for recovery when the cause is a malformed void element. We had a bug for this recently in Blazor.

<input ....></input>

ajaybhargavb · 2018-10-02T22:50:01Z

🆙 📅

pranavkm

Looks good to me

pranavkm · 2018-10-02T22:59:12Z

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

+                    TrackChild(rewritten, rewrittenChildren);
+                }
+
+                tagChildren.ForEach(c => TrackChild(null, rewrittenChildren));


Can we not use lambdas with side effects? Just write the foreach, it's more readable

It'd also let you pass in Array.Empty<RazorSyntaxNode>() instead of new List<>

NTaylorMullen

Overall things are good. The only beef I have with the PR is that the two core methods BuildMarkupElement and TrackChild are supppper confusing to follow.

NTaylorMullen · 2018-10-03T18:42:23Z

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

+{
+    internal static class MarkupElementRewriter
+    {
+        public static RazorSyntaxTree AddMarkupElement(RazorSyntaxTree syntaxTree)


Should be plural, AddMarkupElements Ditto with the remove

NTaylorMullen · 2018-10-03T18:53:08Z

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

+                    {
+                        // Don't want to track incomplete, void or self-closing tags.
+                        // Simply wrap it in a block with no body or start/end tag.
+                        if (IsEndTag(tagBlock))


lol this is super odd. This case handles:

</ >

</foo />

Should update the comment.

NTaylorMullen · 2018-10-03T18:56:50Z

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

+                return node;
+            }
+
+            private void BuildMarkupElement(List<SyntaxNode> rewrittenChildren, MarkupTagBlockSyntax startTag, List<RazorSyntaxNode> tagChildren, MarkupTagBlockSyntax endTag)


IReadOnlyList<RazorSyntaxNode> tagChildren, don't want to give the perception it's edited.

NTaylorMullen · 2018-10-03T18:59:51Z

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

+                // The call to SyntaxNode.ReplaceNodes() later will take care removing the nodes whose replacement is null.
+
+                var body = tagChildren.Where(t => t != null).ToList();
+                var rewritten = SyntaxFactory.MarkupElement(startTag, new SyntaxList<RazorSyntaxNode>(body), endTag: endTag);


no need for endTag:

NTaylorMullen · 2018-10-03T19:03:28Z

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

+                    return node;
+                }
+
+                _startTagTracker.Clear();


In practice is this ever not already clear?

It should be. Just being extra defensive.

NTaylorMullen · 2018-10-03T19:12:54Z

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

+                }
+            }
+
+            private void TrackChild(SyntaxNode child, List<SyntaxNode> rewrittenChildren)


Confusing af

Added this to https://github.com/aspnet/Razor/issues/2619.

ajaybhargavb · 2018-10-04T22:10:46Z

I've addressed most of the feedback here. I've added the one I didn't address to this list https://github.com/aspnet/Razor/issues/2619 to be addressed later. Merging this now.

ajaybhargavb assigned rynowak and NTaylorMullen Oct 1, 2018

ajaybhargavb requested review from rynowak and NTaylorMullen October 1, 2018 20:04

ajaybhargavb commented Oct 1, 2018

View reviewed changes

ajaybhargavb force-pushed the ajbaaska/markup-rewrite branch from 9d014f6 to 4ac4e54 Compare October 1, 2018 20:17

pranavkm reviewed Oct 1, 2018

View reviewed changes

rynowak reviewed Oct 2, 2018

View reviewed changes

ajaybhargavb force-pushed the ajbaaska/markup-rewrite branch from 5df1ece to f1492ef Compare October 2, 2018 22:47

pranavkm reviewed Oct 2, 2018

View reviewed changes

NTaylorMullen reviewed Oct 3, 2018

View reviewed changes

ajaybhargavb force-pushed the ajbaaska/markup-rewrite branch from a7846a1 to e7706ab Compare October 4, 2018 22:08

Added MarkupElementRewriter

e0f3c3d

ajaybhargavb force-pushed the ajbaaska/markup-rewrite branch from e7706ab to e0f3c3d Compare October 4, 2018 22:15

ajaybhargavb merged commit e0f3c3d into ajbaaska/taghelper-parser Oct 4, 2018

ajaybhargavb deleted the ajbaaska/markup-rewrite branch October 4, 2018 22:16

ajaybhargavb mentioned this pull request Dec 14, 2018

[Tracking] Parser improvements feedback dotnet/aspnetcore#5093

Closed

14 tasks

Added MarkupElementRewriter #2624

Added MarkupElementRewriter #2624

Uh oh!

Conversation

ajaybhargavb commented Oct 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaybhargavb commented Oct 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!