Refactored error handling mechanisms for parsing.

- This is needed to make passing error handling devices into more pieces of the parser easy.

#210

src/Microsoft.AspNet.Razor/Parser/ParserContext.cs

@@ -21,25 +21,14 @@ namespace Microsoft.AspNet.Razor.Parser
         private bool _terminated = false;
 
         private Stack<BlockBuilder> _blockStack = new Stack<BlockBuilder>();
+        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<RazorError>();
+            _errorSink = errorSink;
+        }
+
+        public IEnumerable<RazorError> Errors
+        {
+            get
+            {
+                return _errorSink.Errors;
+            }
         }
 
-        public IList<RazorError> 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")]

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
+{
+    /// <summary>
+    /// Used to manage <see cref="RazorError"/>s encountered during the Razor parsing phase.
+    /// </summary>
+    public class ParserErrorSink
+    {
+        private readonly List<RazorError> _errors;
+
+        /// <summary>
+        /// Instantiates a new instance of <see cref="ParserErrorSink"/>.
+        /// </summary>
+        public ParserErrorSink()
+        {
+            _errors = new List<RazorError>();
+        }
+
+        /// <summary>
+        /// <see cref="RazorError"/>s collected.
+        /// </summary>
+        public IEnumerable<RazorError> Errors
+        {
+            get
+            {
+                return _errors;
+            }
+        }
+
+        /// <summary>
+        /// Tracks the given <paramref name="error"/>.
+        /// </summary>
+        /// <param name="error">The <see cref="RazorError"/> to track.</param>        
+        public void OnError(RazorError error)
+        {
+            _errors.Add(error);
+        }
+
+        /// <summary>
+        /// Creates and tracks a new <see cref="RazorError"/>.
+        /// </summary>
+        /// <param name="location"><see cref="SourceLocation"/> of the error.</param>
+        /// <param name="message">A message describing the error.</param>        
+        public void OnError(SourceLocation location, string message)
+        {
+            _errors.Add(new RazorError(message, location));
+        }
+
+        /// <summary>
+        /// Creates and tracks a new <see cref="RazorError"/>.
+        /// </summary>
+        /// <param name="location"><see cref="SourceLocation"/> of the error.</param>
+        /// <param name="message">A message describing the error.</param>
+        /// <param name="length">The length of the error.</param>
+        public void OnError(SourceLocation location, string message, int length)
+        {
+            _errors.Add(new RazorError(message, location, length));
+        }
+    }
+}

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());
         }
 
         /// <summary>

src/Microsoft.AspNet.Razor/Parser/RewritingContext.cs

@@ -17,10 +17,12 @@ namespace Microsoft.AspNet.Razor.Parser
         /// <summary>
         /// Instantiates a new <see cref="RewritingContext"/>.
         /// </summary>
-        public RewritingContext(Block syntaxTree)
+        public RewritingContext(Block syntaxTree, ParserErrorSink errorSink)
         {
             _errors = new List<RazorError>();
             SyntaxTree = syntaxTree;
+
+            ErrorSink = errorSink;
         }
 
         /// <summary>
@@ -28,25 +30,6 @@ namespace Microsoft.AspNet.Razor.Parser
         /// </summary>
         public Block SyntaxTree { get; set; }
 
-        /// <summary>
+        public ParserErrorSink ErrorSink { get; }
-        /// <see cref="RazorError"/>s collected.
-        /// </summary>
-        public IEnumerable<RazorError> Errors
-        {
-            get
-            {
-                return _errors;
-            }
-        }
-
-        /// <summary>
-        /// Creates and tracks a new <see cref="RazorError"/>.
-        /// </summary>
-        /// <param name="location"><see cref="SourceLocation"/> of the error.</param>
-        /// <param name="message">A message describing the error.</param>        
-        public void OnError(SourceLocation location, string message)
-        {
-            _errors.Add(new RazorError(message, location));
-        }
     }
 }

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));

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);
             }

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) {
+        protected virtual ParserResults ParseDocument(string document, ParserErrorSink errorSink)
-            return RunParse(document, parser => parser.ParseDocument, designTimeParser, parserSelector: c => c.MarkupParser);
+        {
+            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<ParserBase, Action> parserActionSelector, bool designTimeParser, Func<ParserContext, ParserBase> parserSelector = null)
+        protected virtual ParserResults RunParse(string document, 
+                                                 Func<ParserBase, Action> parserActionSelector, 
+                                                 bool designTimeParser, 
+                                                 Func<ParserContext, ParserBase> 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;

test/Microsoft.AspNet.Razor.Test/Parser/Html/HtmlAttributeTest.cs

@@ -185,7 +185,7 @@ namespace Microsoft.AspNet.Razor.Test.Parser.Html
         {
             // Act
             var results = ParseDocument("<a href='~/Foo/Bar' />");
-            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;

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<ArgumentNullException>("source", () => new ParserContext(null, codeParser, new HtmlMarkupParser(), codeParser));
-        }
-
-        [Fact]
-        public void ConstructorRequiresNonNullCodeParser()
-        {
-            var codeParser = new CSharpCodeParser();
-            Assert.Throws<ArgumentNullException>("codeParser", () => new ParserContext(new SeekableTextReader(TextReader.Null), null, new HtmlMarkupParser(), codeParser));
-        }
-
-        [Fact]
-        public void ConstructorRequiresNonNullMarkupParser()
-        {
-            var codeParser = new CSharpCodeParser();
-            Assert.Throws<ArgumentNullException>("markupParser", () => new ParserContext(new SeekableTextReader(TextReader.Null), codeParser, null, codeParser));
-        }
-
-        [Fact]
-        public void ConstructorRequiresNonNullActiveParser()
-        {
-            Assert.Throws<ArgumentNullException>("activeParser", () => new ParserContext(new SeekableTextReader(TextReader.Null), new CSharpCodeParser(), new HtmlMarkupParser(), null));
-        }
-
         [Fact]
         public void ConstructorThrowsIfActiveParserIsNotCodeOrMarkupParser()
         {
             var parameterName = "activeParser";
-            var exception = Assert.Throws<ArgumentException>(parameterName, () => new ParserContext(new SeekableTextReader(TextReader.Null), new CSharpCodeParser(), new HtmlMarkupParser(), new CSharpCodeParser()));
+            var exception = Assert.Throws<ArgumentException>(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);
+            var errorSink = new ParserErrorSink();
-            new ParserContext(new SeekableTextReader(TextReader.Null), codeParser, markupParser, markupParser);
+            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<TextReader> positioningAction, ParserBase codeParser, ParserBase markupParser, ParserBase activeParser)
+        private ParserContext SetupTestContext(string document,
+                                               Action<TextReader> 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;
         }

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);

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<RazorError> expectedErrors)
         {
-            var results = ParseDocument(documentContent);
+            var errorSink = new ParserErrorSink();
-            var rewritingContext = new RewritingContext(results.Document);
+            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.
+            EvaluateRazorErrors(errorSink.Errors.ToList(), expectedErrors.ToList());
-            var errors = results.ParserErrors.Concat(rewritingContext.Errors).ToList();
-
-            EvaluateRazorErrors(errors, expectedErrors.ToList());
             EvaluateParseTree(rewritten, expectedOutput);
         }