From 15348b04681965f2ac7ed66968985f3bb16190fa Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Fri, 14 Nov 2014 15:10:02 -0800 Subject: [PATCH] Refactored error handling mechanisms for parsing. - This is needed to make passing error handling devices into more pieces of the parser easy. #210 --- .../Parser/ParserContext.cs | 51 +++++++------- .../Parser/ParserErrorSink.cs | 66 +++++++++++++++++++ .../Parser/RazorParser.cs | 11 ++-- .../Parser/RewritingContext.cs | 25 ++----- .../TagHelpers/TagHelperParseTreeRewriter.cs | 2 +- .../Parser/TokenizerBackedParser.Helpers.cs | 2 +- .../Framework/ParserTestBase.cs | 38 +++++++++-- .../Parser/Html/HtmlAttributeTest.cs | 4 +- .../Parser/ParserContextTest.cs | 62 ++++++++--------- .../Parser/WhitespaceRewriterTest.cs | 2 +- .../TagHelperParseTreeRewriterTest.cs | 18 +++-- 11 files changed, 180 insertions(+), 101 deletions(-) create mode 100644 src/Microsoft.AspNet.Razor/Parser/ParserErrorSink.cs diff --git a/src/Microsoft.AspNet.Razor/Parser/ParserContext.cs b/src/Microsoft.AspNet.Razor/Parser/ParserContext.cs index b2d5664e52..cd5d0ce808 100644 --- a/src/Microsoft.AspNet.Razor/Parser/ParserContext.cs +++ b/src/Microsoft.AspNet.Razor/Parser/ParserContext.cs @@ -21,25 +21,14 @@ namespace Microsoft.AspNet.Razor.Parser private bool _terminated = false; private Stack _blockStack = new Stack(); + private readonly ParserErrorSink _errorSink; - public ParserContext(ITextDocument source, ParserBase codeParser, ParserBase markupParser, ParserBase activeParser) + public ParserContext([NotNull] ITextDocument source, + [NotNull] ParserBase codeParser, + [NotNull] ParserBase markupParser, + [NotNull] ParserBase activeParser, + [NotNull] ParserErrorSink errorSink) { - if (source == null) - { - throw new ArgumentNullException("source"); - } - if (codeParser == null) - { - throw new ArgumentNullException("codeParser"); - } - if (markupParser == null) - { - throw new ArgumentNullException("markupParser"); - } - if (activeParser == null) - { - throw new ArgumentNullException("activeParser"); - } if (activeParser != codeParser && activeParser != markupParser) { throw new ArgumentException(RazorResources.ActiveParser_Must_Be_Code_Or_Markup_Parser, "activeParser"); @@ -51,10 +40,17 @@ namespace Microsoft.AspNet.Razor.Parser CodeParser = codeParser; MarkupParser = markupParser; ActiveParser = activeParser; - Errors = new List(); + _errorSink = errorSink; + } + + public IEnumerable Errors + { + get + { + return _errorSink.Errors; + } } - public IList Errors { get; private set; } public TextDocumentReader Source { get; set; } public ParserBase CodeParser { get; private set; } public ParserBase MarkupParser { get; private set; } @@ -194,18 +190,28 @@ namespace Microsoft.AspNet.Razor.Parser } } + public void OnError(RazorError error) + { + EnusreNotTerminated(); + AssertOnOwnerTask(); + + _errorSink.OnError(error); + } + public void OnError(SourceLocation location, string message) { EnusreNotTerminated(); AssertOnOwnerTask(); - Errors.Add(new RazorError(message, location)); + + _errorSink.OnError(location, message); } public void OnError(SourceLocation location, string message, params object[] args) { EnusreNotTerminated(); AssertOnOwnerTask(); - OnError(location, String.Format(CultureInfo.CurrentCulture, message, args)); + + OnError(location, string.Format(CultureInfo.CurrentCulture, message, args)); } public ParserResults CompleteParse() @@ -218,7 +224,8 @@ namespace Microsoft.AspNet.Razor.Parser { throw new InvalidOperationException(RazorResources.ParserContext_CannotCompleteTree_OutstandingBlocks); } - return new ParserResults(_blockStack.Pop().Build(), Errors); + + return new ParserResults(_blockStack.Pop().Build(), _errorSink.Errors.ToList()); } [Conditional("DEBUG")] diff --git a/src/Microsoft.AspNet.Razor/Parser/ParserErrorSink.cs b/src/Microsoft.AspNet.Razor/Parser/ParserErrorSink.cs new file mode 100644 index 0000000000..8e326d1e36 --- /dev/null +++ b/src/Microsoft.AspNet.Razor/Parser/ParserErrorSink.cs @@ -0,0 +1,66 @@ +// 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 +{ + /// + /// Used to manage s encountered during the Razor parsing phase. + /// + public class ParserErrorSink + { + private readonly List _errors; + + /// + /// Instantiates a new instance of . + /// + public ParserErrorSink() + { + _errors = new List(); + } + + /// + /// s collected. + /// + public IEnumerable Errors + { + get + { + return _errors; + } + } + + /// + /// Tracks the given . + /// + /// The to track. + public void OnError(RazorError error) + { + _errors.Add(error); + } + + /// + /// 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)); + } + + /// + /// Creates and tracks a new . + /// + /// of the error. + /// A message describing the error. + /// The length of the error. + public void OnError(SourceLocation location, string message, int length) + { + _errors.Add(new RazorError(message, location, length)); + } + } +} \ No newline at end of file diff --git a/src/Microsoft.AspNet.Razor/Parser/RazorParser.cs b/src/Microsoft.AspNet.Razor/Parser/RazorParser.cs index 619d2da07f..920350020c 100644 --- a/src/Microsoft.AspNet.Razor/Parser/RazorParser.cs +++ b/src/Microsoft.AspNet.Razor/Parser/RazorParser.cs @@ -149,7 +149,8 @@ namespace Microsoft.AspNet.Razor.Parser private ParserResults ParseCore(ITextDocument input) { // Setup the parser context - var context = new ParserContext(input, CodeParser, MarkupParser, MarkupParser) + var errorSink = new ParserErrorSink(); + var context = new ParserContext(input, CodeParser, MarkupParser, MarkupParser, errorSink) { DesignTimeMode = DesignTimeMode }; @@ -164,7 +165,7 @@ namespace Microsoft.AspNet.Razor.Parser var results = context.CompleteParse(); // Rewrite whitespace if supported - var rewritingContext = new RewritingContext(results.Document); + var rewritingContext = new RewritingContext(results.Document, errorSink); foreach (ISyntaxTreeRewriter rewriter in Optimizers) { rewriter.Rewrite(rewritingContext); @@ -194,12 +195,8 @@ 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(syntaxTree, errors); + return new ParserResults(syntaxTree, errorSink.Errors.ToList()); } /// diff --git a/src/Microsoft.AspNet.Razor/Parser/RewritingContext.cs b/src/Microsoft.AspNet.Razor/Parser/RewritingContext.cs index 68bcc7b55f..cff54037e6 100644 --- a/src/Microsoft.AspNet.Razor/Parser/RewritingContext.cs +++ b/src/Microsoft.AspNet.Razor/Parser/RewritingContext.cs @@ -17,10 +17,12 @@ namespace Microsoft.AspNet.Razor.Parser /// /// Instantiates a new . /// - public RewritingContext(Block syntaxTree) + public RewritingContext(Block syntaxTree, ParserErrorSink errorSink) { _errors = new List(); SyntaxTree = syntaxTree; + + ErrorSink = errorSink; } /// @@ -28,25 +30,6 @@ namespace Microsoft.AspNet.Razor.Parser /// 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)); - } + public ParserErrorSink ErrorSink { get; } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperParseTreeRewriter.cs b/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperParseTreeRewriter.cs index 7611f97cad..995ad9aa80 100644 --- a/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperParseTreeRewriter.cs +++ b/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperParseTreeRewriter.cs @@ -141,7 +141,7 @@ namespace Microsoft.AspNet.Razor.Parser.TagHelpers.Internal // 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( + context.ErrorSink.OnError( malformedTagHelper.Start, RazorResources.FormatTagHelpersParseTreeRewriter_FoundMalformedTagHelper( malformedTagHelper.TagName)); diff --git a/src/Microsoft.AspNet.Razor/Parser/TokenizerBackedParser.Helpers.cs b/src/Microsoft.AspNet.Razor/Parser/TokenizerBackedParser.Helpers.cs index 9f01de7dba..1b53027335 100644 --- a/src/Microsoft.AspNet.Razor/Parser/TokenizerBackedParser.Helpers.cs +++ b/src/Microsoft.AspNet.Razor/Parser/TokenizerBackedParser.Helpers.cs @@ -208,7 +208,7 @@ namespace Microsoft.AspNet.Razor.Parser { foreach (RazorError error in symbol.Errors) { - Context.Errors.Add(error); + Context.OnError(error); } Span.Accept(symbol); } diff --git a/test/Microsoft.AspNet.Razor.Test/Framework/ParserTestBase.cs b/test/Microsoft.AspNet.Razor.Test/Framework/ParserTestBase.cs index 428b161abe..62e66f1e24 100644 --- a/test/Microsoft.AspNet.Razor.Test/Framework/ParserTestBase.cs +++ b/test/Microsoft.AspNet.Razor.Test/Framework/ParserTestBase.cs @@ -35,9 +35,16 @@ namespace Microsoft.AspNet.Razor.Test.Framework protected abstract ParserBase SelectActiveParser(ParserBase codeParser, ParserBase markupParser); - public virtual ParserContext CreateParserContext(ITextDocument input, ParserBase codeParser, ParserBase markupParser) + public virtual ParserContext CreateParserContext(ITextDocument input, + ParserBase codeParser, + ParserBase markupParser, + ParserErrorSink errorSink) { - return new ParserContext(input, codeParser, markupParser, SelectActiveParser(codeParser, markupParser)); + return new ParserContext(input, + codeParser, + markupParser, + SelectActiveParser(codeParser, markupParser), + errorSink); } protected abstract SpanFactory CreateSpanFactory(); @@ -146,11 +153,23 @@ namespace Microsoft.AspNet.Razor.Test.Framework } protected virtual ParserResults ParseDocument(string document) { - return ParseDocument(document, designTimeParser: false); + return ParseDocument(document, designTimeParser: false, errorSink: null); } - protected virtual ParserResults ParseDocument(string document, bool designTimeParser) { - return RunParse(document, parser => parser.ParseDocument, designTimeParser, parserSelector: c => c.MarkupParser); + protected virtual ParserResults ParseDocument(string document, ParserErrorSink errorSink) + { + return ParseDocument(document, designTimeParser: false, errorSink: errorSink); + } + + protected virtual ParserResults ParseDocument(string document, + bool designTimeParser, + ParserErrorSink errorSink) + { + return RunParse(document, + parser => parser.ParseDocument, + designTimeParser, + parserSelector: c => c.MarkupParser, + errorSink: errorSink); } protected virtual ParserResults ParseBlock(string document) { @@ -161,9 +180,14 @@ namespace Microsoft.AspNet.Razor.Test.Framework return RunParse(document, parser => parser.ParseBlock, designTimeParser); } - protected virtual ParserResults RunParse(string document, Func parserActionSelector, bool designTimeParser, Func parserSelector = null) + protected virtual ParserResults RunParse(string document, + Func parserActionSelector, + bool designTimeParser, + Func parserSelector = null, + ParserErrorSink errorSink = null) { parserSelector = parserSelector ?? (c => c.ActiveParser); + errorSink = errorSink ?? new ParserErrorSink(); // Create the source ParserResults results = null; @@ -173,7 +197,7 @@ namespace Microsoft.AspNet.Razor.Test.Framework { var codeParser = CreateCodeParser(); var markupParser = CreateMarkupParser(); - var context = CreateParserContext(reader, codeParser, markupParser); + var context = CreateParserContext(reader, codeParser, markupParser, errorSink); context.DesignTimeMode = designTimeParser; codeParser.Context = context; diff --git a/test/Microsoft.AspNet.Razor.Test/Parser/Html/HtmlAttributeTest.cs b/test/Microsoft.AspNet.Razor.Test/Parser/Html/HtmlAttributeTest.cs index 6b208adfa7..683714f526 100644 --- a/test/Microsoft.AspNet.Razor.Test/Parser/Html/HtmlAttributeTest.cs +++ b/test/Microsoft.AspNet.Razor.Test/Parser/Html/HtmlAttributeTest.cs @@ -185,7 +185,7 @@ namespace Microsoft.AspNet.Razor.Test.Parser.Html { // Act var results = ParseDocument(""); - var rewritingContext = new RewritingContext(results.Document); + var rewritingContext = new RewritingContext(results.Document, new ParserErrorSink()); new ConditionalAttributeCollapser(new HtmlMarkupParser().BuildSpan).Rewrite(rewritingContext); new MarkupCollapser(new HtmlMarkupParser().BuildSpan).Rewrite(rewritingContext); var rewritten = rewritingContext.SyntaxTree; @@ -272,7 +272,7 @@ namespace Microsoft.AspNet.Razor.Test.Parser.Html // Act var results = ParseDocument(code); - var rewritingContext = new RewritingContext(results.Document); + var rewritingContext = new RewritingContext(results.Document, new ParserErrorSink()); new ConditionalAttributeCollapser(new HtmlMarkupParser().BuildSpan).Rewrite(rewritingContext); new MarkupCollapser(new HtmlMarkupParser().BuildSpan).Rewrite(rewritingContext); var rewritten = rewritingContext.SyntaxTree; diff --git a/test/Microsoft.AspNet.Razor.Test/Parser/ParserContextTest.cs b/test/Microsoft.AspNet.Razor.Test/Parser/ParserContextTest.cs index afb6ad4206..0fe69e71d2 100644 --- a/test/Microsoft.AspNet.Razor.Test/Parser/ParserContextTest.cs +++ b/test/Microsoft.AspNet.Razor.Test/Parser/ParserContextTest.cs @@ -17,38 +17,17 @@ namespace Microsoft.AspNet.Razor.Test.Parser { public class ParserContextTest { - [Fact] - public void ConstructorRequiresNonNullSource() - { - var codeParser = new CSharpCodeParser(); - Assert.Throws("source", () => new ParserContext(null, codeParser, new HtmlMarkupParser(), codeParser)); - } - - [Fact] - public void ConstructorRequiresNonNullCodeParser() - { - var codeParser = new CSharpCodeParser(); - Assert.Throws("codeParser", () => new ParserContext(new SeekableTextReader(TextReader.Null), null, new HtmlMarkupParser(), codeParser)); - } - - [Fact] - public void ConstructorRequiresNonNullMarkupParser() - { - var codeParser = new CSharpCodeParser(); - Assert.Throws("markupParser", () => new ParserContext(new SeekableTextReader(TextReader.Null), codeParser, null, codeParser)); - } - - [Fact] - public void ConstructorRequiresNonNullActiveParser() - { - Assert.Throws("activeParser", () => new ParserContext(new SeekableTextReader(TextReader.Null), new CSharpCodeParser(), new HtmlMarkupParser(), null)); - } - [Fact] public void ConstructorThrowsIfActiveParserIsNotCodeOrMarkupParser() { var parameterName = "activeParser"; - var exception = Assert.Throws(parameterName, () => new ParserContext(new SeekableTextReader(TextReader.Null), new CSharpCodeParser(), new HtmlMarkupParser(), new CSharpCodeParser())); + var exception = Assert.Throws(parameterName, + () => new ParserContext( + source: new SeekableTextReader(TextReader.Null), + codeParser: new CSharpCodeParser(), + markupParser: new HtmlMarkupParser(), + activeParser: new CSharpCodeParser(), + errorSink: new ParserErrorSink())); ExceptionHelpers.ValidateArgumentException(parameterName, RazorResources.ActiveParser_Must_Be_Code_Or_Markup_Parser, exception); } @@ -57,8 +36,11 @@ namespace Microsoft.AspNet.Razor.Test.Parser { var codeParser = new CSharpCodeParser(); var markupParser = new HtmlMarkupParser(); - new ParserContext(new SeekableTextReader(TextReader.Null), codeParser, markupParser, codeParser); - new ParserContext(new SeekableTextReader(TextReader.Null), codeParser, markupParser, markupParser); + var errorSink = new ParserErrorSink(); + new ParserContext( + new SeekableTextReader(TextReader.Null), codeParser, markupParser, codeParser, errorSink); + new ParserContext( + new SeekableTextReader(TextReader.Null), codeParser, markupParser, markupParser, errorSink); } [Fact] @@ -70,7 +52,11 @@ namespace Microsoft.AspNet.Razor.Test.Parser var expectedMarkupParser = new HtmlMarkupParser(); // Act - var context = new ParserContext(expectedBuffer, expectedCodeParser, expectedMarkupParser, expectedCodeParser); + var context = new ParserContext(expectedBuffer, + expectedCodeParser, + expectedMarkupParser, + expectedCodeParser, + new ParserErrorSink()); // Assert Assert.NotNull(context.Source); @@ -236,9 +222,19 @@ namespace Microsoft.AspNet.Razor.Test.Parser return SetupTestContext(document, positioningAction, codeParser, markupParser, codeParser); } - private ParserContext SetupTestContext(string document, Action positioningAction, ParserBase codeParser, ParserBase markupParser, ParserBase activeParser) + private ParserContext SetupTestContext(string document, + Action positioningAction, + ParserBase codeParser, + ParserBase markupParser, + ParserBase activeParser) { - var context = new ParserContext(new SeekableTextReader(new StringReader(document)), codeParser, markupParser, activeParser); + var context = new ParserContext( + new SeekableTextReader(new StringReader(document)), + codeParser, + markupParser, + activeParser, + new ParserErrorSink()); + positioningAction(context.Source); return context; } diff --git a/test/Microsoft.AspNet.Razor.Test/Parser/WhitespaceRewriterTest.cs b/test/Microsoft.AspNet.Razor.Test/Parser/WhitespaceRewriterTest.cs index c1d912b0e0..2e8fa54a3a 100644 --- a/test/Microsoft.AspNet.Razor.Test/Parser/WhitespaceRewriterTest.cs +++ b/test/Microsoft.AspNet.Razor.Test/Parser/WhitespaceRewriterTest.cs @@ -32,7 +32,7 @@ namespace Microsoft.AspNet.Razor.Test.Parser factory.Markup("test") ); var rewriter = new WhiteSpaceRewriter(new HtmlMarkupParser().BuildSpan); - var rewritingContext = new RewritingContext(start); + var rewritingContext = new RewritingContext(start, new ParserErrorSink()); // Act rewriter.Rewrite(rewritingContext); diff --git a/test/Microsoft.AspNet.Razor.Test/TagHelpers/TagHelperParseTreeRewriterTest.cs b/test/Microsoft.AspNet.Razor.Test/TagHelpers/TagHelperParseTreeRewriterTest.cs index 3c4ccebbee..33ef7c5139 100644 --- a/test/Microsoft.AspNet.Razor.Test/TagHelpers/TagHelperParseTreeRewriterTest.cs +++ b/test/Microsoft.AspNet.Razor.Test/TagHelpers/TagHelperParseTreeRewriterTest.cs @@ -1181,20 +1181,26 @@ namespace Microsoft.AspNet.Razor.Test.TagHelpers return new TagHelperDescriptorProvider(descriptors); } + public override ParserContext CreateParserContext(ITextDocument input, + ParserBase codeParser, + ParserBase markupParser, + ParserErrorSink errorSink) + { + return base.CreateParserContext(input, codeParser, markupParser, errorSink); + } + private void EvaluateData(TagHelperDescriptorProvider provider, string documentContent, MarkupBlock expectedOutput, IEnumerable expectedErrors) { - var results = ParseDocument(documentContent); - var rewritingContext = new RewritingContext(results.Document); + var errorSink = new ParserErrorSink(); + var results = ParseDocument(documentContent, errorSink); + var rewritingContext = new RewritingContext(results.Document, errorSink); new TagHelperParseTreeRewriter(provider).Rewrite(rewritingContext); var rewritten = rewritingContext.SyntaxTree; - // 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()); + EvaluateRazorErrors(errorSink.Errors.ToList(), expectedErrors.ToList()); EvaluateParseTree(rewritten, expectedOutput); }