From 74974d371cc4903851f0aba748a728fcb719a0b1 Mon Sep 17 00:00:00 2001 From: NTaylorMullen Date: Sat, 4 Oct 2014 23:03:23 -0700 Subject: [PATCH] Add error mechanism for TagHelperParseTreeRewriter. - Replaced customer facing Debug.Assert with a new error mechanism to surface errors to GenerateCode callers. - The new mechanism is a general purpose way for ISyntaxTreeRewriters to add errors to the parsing phase. #174 --- .../Parser/ISyntaxTreeRewriter.cs | 12 +- .../Parser/MarkupRewriter.cs | 6 +- .../Parser/RazorParser.cs | 18 ++- .../Parser/RewritingContext.cs | 52 ++++++++ .../Parser/TagHelpers/TagHelperBlock.cs | 13 ++ .../TagHelpers/TagHelperBlockBuilder.cs | 9 ++ .../TagHelpers/TagHelperParseTreeRewriter.cs | 39 +++++- .../Properties/RazorResources.Designer.cs | 16 +++ .../RazorResources.resx | 3 + .../Parser/Html/HtmlAttributeTest.cs | 12 +- .../Parser/WhitespaceRewriterTest.cs | 5 +- .../TagHelperParseTreeRewriterTest.cs | 117 +++++++++++++++++- 12 files changed, 270 insertions(+), 32 deletions(-) create mode 100644 src/Microsoft.AspNet.Razor/Parser/RewritingContext.cs diff --git a/src/Microsoft.AspNet.Razor/Parser/ISyntaxTreeRewriter.cs b/src/Microsoft.AspNet.Razor/Parser/ISyntaxTreeRewriter.cs index b080ca237f..55adf8c735 100644 --- a/src/Microsoft.AspNet.Razor/Parser/ISyntaxTreeRewriter.cs +++ b/src/Microsoft.AspNet.Razor/Parser/ISyntaxTreeRewriter.cs @@ -6,19 +6,17 @@ using Microsoft.AspNet.Razor.Parser.SyntaxTree; namespace Microsoft.AspNet.Razor.Parser { /// - /// Defines the contract for rewriting a syntax tree. + /// Contract for rewriting a syntax tree. /// public interface ISyntaxTreeRewriter { /// - /// Rewrites the provided syntax tree. + /// Rewrites the provided s . /// - /// The current syntax tree. - /// The syntax tree or a syntax tree to be used instead of the - /// tree. + /// Contains information on the rewriting of the syntax tree. /// - /// If you choose not to modify the syntax tree you can always return . + /// To modify the syntax tree replace the s . /// - Block Rewrite(Block input); + void Rewrite(RewritingContext context); } } diff --git a/src/Microsoft.AspNet.Razor/Parser/MarkupRewriter.cs b/src/Microsoft.AspNet.Razor/Parser/MarkupRewriter.cs index 0866aadd05..7136860e42 100644 --- a/src/Microsoft.AspNet.Razor/Parser/MarkupRewriter.cs +++ b/src/Microsoft.AspNet.Razor/Parser/MarkupRewriter.cs @@ -28,11 +28,11 @@ namespace Microsoft.AspNet.Razor.Parser get { return _blocks.Count > 0 ? _blocks.Peek() : null; } } - public virtual Block Rewrite(Block input) + public virtual void Rewrite(RewritingContext context) { - input.Accept(this); + context.SyntaxTree.Accept(this); Debug.Assert(_blocks.Count == 1); - return _blocks.Pop().Build(); + context.SyntaxTree = _blocks.Pop().Build(); } public override void VisitBlock(Block block) diff --git a/src/Microsoft.AspNet.Razor/Parser/RazorParser.cs b/src/Microsoft.AspNet.Razor/Parser/RazorParser.cs index 1bf6b83698..da5d8692a1 100644 --- a/src/Microsoft.AspNet.Razor/Parser/RazorParser.cs +++ b/src/Microsoft.AspNet.Razor/Parser/RazorParser.cs @@ -136,25 +136,27 @@ namespace Microsoft.AspNet.Razor.Parser ParserResults results = context.CompleteParse(); // Rewrite whitespace if supported - Block current = results.Document; + var rewritingContext = new RewritingContext(results.Document); foreach (ISyntaxTreeRewriter rewriter in Optimizers) { - current = rewriter.Rewrite(current); + rewriter.Rewrite(rewritingContext); } if (_tagHelperDescriptorResolver != null) { var tagHelperRegistrationVisitor = new TagHelperRegistrationVisitor(_tagHelperDescriptorResolver); - var tagHelperProvider = tagHelperRegistrationVisitor.CreateProvider(current); + var tagHelperProvider = tagHelperRegistrationVisitor.CreateProvider(rewritingContext.SyntaxTree); var tagHelperParseTreeRewriter = new TagHelperParseTreeRewriter(tagHelperProvider); // Rewrite the document to utilize tag helpers - current = tagHelperParseTreeRewriter.Rewrite(current); + tagHelperParseTreeRewriter.Rewrite(rewritingContext); } + var syntaxTree = rewritingContext.SyntaxTree; + // Link the leaf nodes into a chain Span prev = null; - foreach (Span node in current.Flatten()) + foreach (Span node in syntaxTree.Flatten()) { node.Previous = prev; if (prev != null) @@ -164,8 +166,12 @@ namespace Microsoft.AspNet.Razor.Parser prev = node; } + // We want to surface both the parsing and rewriting errors as one unified list of errors because + // both parsing and rewriting errors affect the end users Razor page. + var errors = results.ParserErrors.Concat(rewritingContext.Errors).ToList(); + // Return the new result - return new ParserResults(current, results.ParserErrors); + return new ParserResults(syntaxTree, errors); } } } diff --git a/src/Microsoft.AspNet.Razor/Parser/RewritingContext.cs b/src/Microsoft.AspNet.Razor/Parser/RewritingContext.cs new file mode 100644 index 0000000000..68bcc7b55f --- /dev/null +++ b/src/Microsoft.AspNet.Razor/Parser/RewritingContext.cs @@ -0,0 +1,52 @@ +// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Generic; +using Microsoft.AspNet.Razor.Parser.SyntaxTree; +using Microsoft.AspNet.Razor.Text; + +namespace Microsoft.AspNet.Razor.Parser +{ + /// + /// Informational class for rewriting a syntax tree. + /// + public class RewritingContext + { + private readonly List _errors; + + /// + /// Instantiates a new . + /// + public RewritingContext(Block syntaxTree) + { + _errors = new List(); + SyntaxTree = syntaxTree; + } + + /// + /// The documents syntax tree. + /// + public Block SyntaxTree { get; set; } + + /// + /// s collected. + /// + public IEnumerable Errors + { + get + { + return _errors; + } + } + + /// + /// Creates and tracks a new . + /// + /// of the error. + /// A message describing the error. + public void OnError(SourceLocation location, string message) + { + _errors.Add(new RazorError(message, location)); + } + } +} \ No newline at end of file diff --git a/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperBlock.cs b/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperBlock.cs index 66cd083982..7067b24cf9 100644 --- a/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperBlock.cs +++ b/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperBlock.cs @@ -7,6 +7,7 @@ using System.Diagnostics; using System.Globalization; using System.Linq; using Microsoft.AspNet.Razor.Parser.SyntaxTree; +using Microsoft.AspNet.Razor.Text; using Microsoft.Internal.Web.Utils; namespace Microsoft.AspNet.Razor.Parser.TagHelpers @@ -16,6 +17,8 @@ namespace Microsoft.AspNet.Razor.Parser.TagHelpers /// public class TagHelperBlock : Block, IEquatable { + private readonly SourceLocation _start; + /// /// Instantiates a new instance of a . /// @@ -26,6 +29,7 @@ namespace Microsoft.AspNet.Razor.Parser.TagHelpers { TagName = source.TagName; Attributes = new Dictionary(source.Attributes); + _start = source.Start; source.Reset(); @@ -40,6 +44,15 @@ namespace Microsoft.AspNet.Razor.Parser.TagHelpers /// public IDictionary Attributes { get; private set; } + /// + public override SourceLocation Start + { + get + { + return _start; + } + } + /// /// The HTML tag name. /// diff --git a/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperBlockBuilder.cs b/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperBlockBuilder.cs index 3fc4fc7dc3..8308c01d22 100644 --- a/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperBlockBuilder.cs +++ b/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperBlockBuilder.cs @@ -7,6 +7,7 @@ using System.Linq; using Microsoft.AspNet.Razor.Generator; using Microsoft.AspNet.Razor.Parser.SyntaxTree; using Microsoft.AspNet.Razor.TagHelpers; +using Microsoft.AspNet.Razor.Text; using Microsoft.AspNet.Razor.Tokenizer.Symbols; namespace Microsoft.AspNet.Razor.Parser.TagHelpers @@ -44,6 +45,9 @@ namespace Microsoft.AspNet.Razor.Parser.TagHelpers CodeGenerator = new TagHelperCodeGenerator(descriptors); Type = startTag.Type; Attributes = GetTagAttributes(startTag); + + // There will always be at least one child for the '<'. + Start = startTag.Children.First().Start; } // Internal for testing @@ -98,6 +102,11 @@ namespace Microsoft.AspNet.Razor.Parser.TagHelpers base.Reset(); } + /// + /// The starting of the tag helper. + /// + public SourceLocation Start { get; private set; } + private static IDictionary GetTagAttributes(Block tagBlock) { var attributes = new Dictionary(StringComparer.OrdinalIgnoreCase); diff --git a/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperParseTreeRewriter.cs b/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperParseTreeRewriter.cs index 01330aaf96..4a08a901d2 100644 --- a/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperParseTreeRewriter.cs +++ b/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperParseTreeRewriter.cs @@ -25,13 +25,13 @@ namespace Microsoft.AspNet.Razor.Parser.TagHelpers.Internal _blockStack = new Stack(); } - public Block Rewrite(Block input) + public void Rewrite(RewritingContext context) { - RewriteTags(input); + RewriteTags(context.SyntaxTree); - Debug.Assert(_blockStack.Count == 0); + ValidateRewrittenSyntaxTree(context); - return _currentBlock.Build(); + context.SyntaxTree = _currentBlock.Build(); } private void RewriteTags(Block input) @@ -122,6 +122,37 @@ namespace Microsoft.AspNet.Razor.Parser.TagHelpers.Internal BuildCurrentlyTrackedBlock(); } + private void ValidateRewrittenSyntaxTree(RewritingContext context) + { + // If the blockStack still has elements in it that means there's at least one malformed TagHelper block in + // the document, that's the only way we can have a non-zero _blockStack.Count. + if (_blockStack.Count != 0) + { + // We reverse the children so we can search from the back to the front for the TagHelper that is + // malformed. + var candidateChildren = _currentBlock.Children.Reverse(); + var malformedTagHelper = candidateChildren.OfType().FirstOrDefault(); + + // If the malformed tag helper is null that means something other than a TagHelper caused the + // unbalancing of the syntax tree (should never happen). + Debug.Assert(malformedTagHelper != null); + + // We only create a single error because we can't reasonably determine other invalid tag helpers in the + // document; having one malformed tag helper puts the document into an invalid state. + context.OnError( + malformedTagHelper.Start, + RazorResources.FormatTagHelpersParseTreeRewriter_FoundMalformedTagHelper( + malformedTagHelper.TagName)); + + // We need to build the remaining blocks in the stack to ensure we don't return an invalid syntax tree. + do + { + BuildCurrentlyTrackedTagHelperBlock(); + } + while (_blockStack.Count != 0); + } + } + private void BuildCurrentlyTrackedBlock() { // Going to remove the current BlockBuilder from the stack because it's complete. diff --git a/src/Microsoft.AspNet.Razor/Properties/RazorResources.Designer.cs b/src/Microsoft.AspNet.Razor/Properties/RazorResources.Designer.cs index 4d965eec63..cd468b4d71 100644 --- a/src/Microsoft.AspNet.Razor/Properties/RazorResources.Designer.cs +++ b/src/Microsoft.AspNet.Razor/Properties/RazorResources.Designer.cs @@ -1542,6 +1542,22 @@ namespace Microsoft.AspNet.Razor return string.Format(CultureInfo.CurrentCulture, GetString("TagHelpers_CannotUseDirectiveWithNoTagHelperDescriptorResolver"), p0, p1, p2); } + /// + /// Found a malformed '{0}' tag helper. Tag helpers must have a start and end tag or be self closing. + /// + internal static string TagHelpersParseTreeRewriter_FoundMalformedTagHelper + { + get { return GetString("TagHelpersParseTreeRewriter_FoundMalformedTagHelper"); } + } + + /// + /// Found a malformed '{0}' tag helper. Tag helpers must have a start and end tag or be self closing. + /// + internal static string FormatTagHelpersParseTreeRewriter_FoundMalformedTagHelper(object p0) + { + return string.Format(CultureInfo.CurrentCulture, GetString("TagHelpersParseTreeRewriter_FoundMalformedTagHelper"), p0); + } + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Microsoft.AspNet.Razor/RazorResources.resx b/src/Microsoft.AspNet.Razor/RazorResources.resx index b258cdc9c3..476f47f4ce 100644 --- a/src/Microsoft.AspNet.Razor/RazorResources.resx +++ b/src/Microsoft.AspNet.Razor/RazorResources.resx @@ -424,4 +424,7 @@ Instead, wrap the contents of the block in "{{}}": Cannot use directive '{0}' when a {1} has not been provided to the {2}. + + Found a malformed '{0}' tag helper. Tag helpers must have a start and end tag or be self closing. + \ No newline at end of file diff --git a/test/Microsoft.AspNet.Razor.Test/Parser/Html/HtmlAttributeTest.cs b/test/Microsoft.AspNet.Razor.Test/Parser/Html/HtmlAttributeTest.cs index 89034a460d..0c9c2a74b0 100644 --- a/test/Microsoft.AspNet.Razor.Test/Parser/Html/HtmlAttributeTest.cs +++ b/test/Microsoft.AspNet.Razor.Test/Parser/Html/HtmlAttributeTest.cs @@ -185,8 +185,10 @@ namespace Microsoft.AspNet.Razor.Test.Parser.Html { // Act ParserResults results = ParseDocument(""); - Block rewritten = new ConditionalAttributeCollapser(new HtmlMarkupParser().BuildSpan).Rewrite(results.Document); - rewritten = new MarkupCollapser(new HtmlMarkupParser().BuildSpan).Rewrite(rewritten); + var rewritingContext = new RewritingContext(results.Document); + new ConditionalAttributeCollapser(new HtmlMarkupParser().BuildSpan).Rewrite(rewritingContext); + new MarkupCollapser(new HtmlMarkupParser().BuildSpan).Rewrite(rewritingContext); + var rewritten = rewritingContext.SyntaxTree; // Assert Assert.Equal(0, results.ParserErrors.Count); @@ -270,8 +272,10 @@ namespace Microsoft.AspNet.Razor.Test.Parser.Html // Act ParserResults results = ParseDocument(code); - Block rewritten = new ConditionalAttributeCollapser(new HtmlMarkupParser().BuildSpan).Rewrite(results.Document); - rewritten = new MarkupCollapser(new HtmlMarkupParser().BuildSpan).Rewrite(rewritten); + var rewritingContext = new RewritingContext(results.Document); + new ConditionalAttributeCollapser(new HtmlMarkupParser().BuildSpan).Rewrite(rewritingContext); + new MarkupCollapser(new HtmlMarkupParser().BuildSpan).Rewrite(rewritingContext); + var rewritten = rewritingContext.SyntaxTree; // Assert Assert.Equal(0, results.ParserErrors.Count); diff --git a/test/Microsoft.AspNet.Razor.Test/Parser/WhitespaceRewriterTest.cs b/test/Microsoft.AspNet.Razor.Test/Parser/WhitespaceRewriterTest.cs index b068775a0c..bf82039514 100644 --- a/test/Microsoft.AspNet.Razor.Test/Parser/WhitespaceRewriterTest.cs +++ b/test/Microsoft.AspNet.Razor.Test/Parser/WhitespaceRewriterTest.cs @@ -32,14 +32,15 @@ namespace Microsoft.AspNet.Razor.Test.Parser factory.Markup("test") ); WhiteSpaceRewriter rewriter = new WhiteSpaceRewriter(new HtmlMarkupParser().BuildSpan); + var rewritingContext = new RewritingContext(start); // Act - Block actual = rewriter.Rewrite(start); + rewriter.Rewrite(rewritingContext); factory.Reset(); // Assert - ParserTestBase.EvaluateParseTree(actual, new MarkupBlock( + ParserTestBase.EvaluateParseTree(rewritingContext.SyntaxTree, new MarkupBlock( factory.Markup("test"), factory.Markup(" "), new ExpressionBlock( diff --git a/test/Microsoft.AspNet.Razor.Test/TagHelpers/TagHelperParseTreeRewriterTest.cs b/test/Microsoft.AspNet.Razor.Test/TagHelpers/TagHelperParseTreeRewriterTest.cs index 4d60856852..4896a5936d 100644 --- a/test/Microsoft.AspNet.Razor.Test/TagHelpers/TagHelperParseTreeRewriterTest.cs +++ b/test/Microsoft.AspNet.Razor.Test/TagHelpers/TagHelperParseTreeRewriterTest.cs @@ -2,6 +2,8 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Collections.Generic; +using System.Globalization; +using System.Linq; using Microsoft.AspNet.Razor.Generator; using Microsoft.AspNet.Razor.Parser; using Microsoft.AspNet.Razor.Parser.SyntaxTree; @@ -16,6 +18,90 @@ namespace Microsoft.AspNet.Razor.Test.TagHelpers { public class TagHelperParseTreeRewriterTest : CsHtmlMarkupParserTestBase { + public static IEnumerable IncompleteHelperBlockData + { + get + { + var factory = CreateDefaultSpanFactory(); + var blockFactory = new BlockFactory(factory); + var errorFormat = "Found a malformed '{0}' tag helper. Tag helpers must have a start and end tag or " + + "be self closing."; + var dateTimeNow = new MarkupBlock( + new ExpressionBlock( + factory.CodeTransition(), + factory.Code("DateTime.Now") + .AsImplicitExpression(CSharpCodeParser.DefaultKeywords) + .Accepts(AcceptedCharacters.NonWhiteSpace))); + + yield return new object[] { + "

", + new MarkupBlock( + new MarkupTagHelperBlock("p", + new Dictionary + { + { "class", new MarkupBlock(factory.Markup("foo")) }, + { "dynamic", new MarkupBlock(dateTimeNow) }, + { "style", new MarkupBlock(factory.Markup("color:red;")) } + }, + new MarkupTagHelperBlock("strong", + blockFactory.MarkupTagBlock("

")))), + new RazorError(string.Format(CultureInfo.InvariantCulture, errorFormat, "p"), + SourceLocation.Zero) + }; + yield return new object[] { + "

Hello World

", + new MarkupBlock( + blockFactory.MarkupTagBlock("
"), + new MarkupTagHelperBlock("p", + factory.Markup("Hello "), + new MarkupTagHelperBlock("strong", + factory.Markup("World")), + blockFactory.MarkupTagBlock("
"))), + new RazorError(string.Format(CultureInfo.InvariantCulture, errorFormat, "p"), + absoluteIndex: 5, lineIndex: 0, columnIndex: 5) + }; + yield return new object[] { + "

Hello World

", + new MarkupBlock( + blockFactory.MarkupTagBlock("
"), + new MarkupTagHelperBlock("p", + factory.Markup("Hello "), + new MarkupTagHelperBlock("strong", + factory.Markup("World"), + blockFactory.MarkupTagBlock("
")))), + new RazorError(string.Format(CultureInfo.InvariantCulture, errorFormat, "strong"), + absoluteIndex: 14, lineIndex: 0, columnIndex: 14) + }; + yield return new object[] { + "

Hello

World

", + new MarkupBlock( + new MarkupTagHelperBlock("p", + new Dictionary + { + { "class", new MarkupBlock(factory.Markup("foo")) } + }, + factory.Markup("Hello "), + new MarkupTagHelperBlock("p", + new Dictionary + { + { "style", new MarkupBlock(factory.Markup("color:red;")) } + }, + factory.Markup("World")))), + new RazorError(string.Format(CultureInfo.InvariantCulture, errorFormat, "p"), + SourceLocation.Zero) + }; + } + } + [Theory] + [MemberData(nameof(IncompleteHelperBlockData))] + public void TagHelperParseTreeRewriter_CreatesErrorForIncompleteTagHelper( + string documentContent, + MarkupBlock expectedOutput, + RazorError expectedError) + { + RunParseTreeRewriterTest(documentContent, expectedOutput, new[] { expectedError }, "strong", "p"); + } + public static IEnumerable TextTagsBlockData { get @@ -938,14 +1024,25 @@ namespace Microsoft.AspNet.Razor.Test.TagHelpers } private void RunParseTreeRewriterTest(string documentContent, - MarkupBlock expectedOutput, - params string[] tagNames) + MarkupBlock expectedOutput, + params string[] tagNames) + { + RunParseTreeRewriterTest(documentContent, + expectedOutput, + errors: Enumerable.Empty(), + tagNames: tagNames); + } + + private void RunParseTreeRewriterTest(string documentContent, + MarkupBlock expectedOutput, + IEnumerable errors, + params string[] tagNames) { // Arrange var providerContext = BuildProviderContext(tagNames); // Act & Assert - EvaluateData(providerContext, documentContent, expectedOutput); + EvaluateData(providerContext, documentContent, expectedOutput, errors); } private TagHelperDescriptorProvider BuildProviderContext(params string[] tagNames) @@ -961,12 +1058,20 @@ namespace Microsoft.AspNet.Razor.Test.TagHelpers return new TagHelperDescriptorProvider(descriptors); } - private void EvaluateData(TagHelperDescriptorProvider provider, string documentContent, MarkupBlock expectedOutput) + private void EvaluateData(TagHelperDescriptorProvider provider, + string documentContent, + MarkupBlock expectedOutput, + IEnumerable expectedErrors) { var results = ParseDocument(documentContent); - var rewritten = new TagHelperParseTreeRewriter(provider).Rewrite(results.Document); + var rewritingContext = new RewritingContext(results.Document); + new TagHelperParseTreeRewriter(provider).Rewrite(rewritingContext); + var rewritten = rewritingContext.SyntaxTree; - Assert.Empty(results.ParserErrors); + // Combine the parser errors and the rewriter errors. Normally the RazorParser does this. + var errors = results.ParserErrors.Concat(rewritingContext.Errors).ToList(); + + EvaluateRazorErrors(errors, expectedErrors.ToList()); EvaluateParseTree(rewritten, expectedOutput); }