From 0835de17ecde4fe0560ad649918e47b6e0a3e8da Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Wed, 28 Dec 2016 21:25:36 -0800 Subject: [PATCH] Adds stronger verification for spans This change ensures that spans are contiguous and that all source is part of a span. This means that a character can't be 'lost' and not a member of any span. And guess what? We have a bug like that. So now a few tests are skipped due to that bug. Also made some changes to tests that construct invalid spans or spans without correct locations as their expected input. This allows us to add the above verification to all parser tests. --- .../Legacy/CSharpBlockTest.cs | 28 +++-- .../Legacy/CSharpRazorCommentsTest.cs | 2 +- .../Legacy/CSharpSectionTest.cs | 2 + .../Legacy/CSharpStatementTest.cs | 6 + .../Legacy/CSharpTemplateTest.cs | 2 + .../Legacy/CSharpToMarkupSwitchTest.cs | 6 +- .../Legacy/HtmlBlockTest.cs | 2 + .../Legacy/HtmlDocumentTest.cs | 2 + .../Legacy/ParserTestBase.cs | 103 ++++++++++++++---- .../SyntaxTreeVerifier.cs | 42 +++++++ 10 files changed, 161 insertions(+), 34 deletions(-) create mode 100644 test/Microsoft.AspNetCore.Razor.Evolution.Test/SyntaxTreeVerifier.cs diff --git a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpBlockTest.cs b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpBlockTest.cs index fdc6ca3d72..96ac830efd 100644 --- a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpBlockTest.cs +++ b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpBlockTest.cs @@ -611,6 +611,7 @@ catch(bar) { baz(); }", BlockType.Statement, SpanKind.Code); prefix: "try {", markup: "

Foo

", suffix: "}", + expectedStart: new SourceLocation(5, 0, 5), expectedMarkup: new MarkupBlock( Factory.Markup(" "), BlockFactory.MarkupTagBlock("

", AcceptedCharacters.None), @@ -632,6 +633,7 @@ catch(bar) { baz(); }", BlockType.Statement, SpanKind.Code); prefix: "try { var foo = new { } } catch(Foo Bar Baz) {", markup: "

Foo

", suffix: "}", + expectedStart: new SourceLocation(46, 0, 46), expectedMarkup: new MarkupBlock( Factory.Markup(" "), BlockFactory.MarkupTagBlock("

", AcceptedCharacters.None), @@ -664,6 +666,7 @@ catch(bar) { baz(); }", BlockType.Statement, SpanKind.Code); "{ var foo = new { } } catch(Foo Bar Baz) {", markup: "

Foo

", suffix: "}", + expectedStart: new SourceLocation(128, 0, 128), expectedMarkup: new MarkupBlock( Factory.Markup(" "), BlockFactory.MarkupTagBlock("

", AcceptedCharacters.None), @@ -685,6 +688,7 @@ catch(bar) { baz(); }", BlockType.Statement, SpanKind.Code); prefix: "try { var foo = new { } } finally {", markup: "

Foo

", suffix: "}", + expectedStart: new SourceLocation(35, 0, 35), expectedMarkup: new MarkupBlock( Factory.Markup(" "), BlockFactory.MarkupTagBlock("

", AcceptedCharacters.None), @@ -1115,6 +1119,8 @@ catch(bar) { baz(); }", BlockType.Statement, SpanKind.Code); [MemberData(nameof(BlockWithEscapedTransitionData))] public void ParseBlock_WithDoubleTransition_DoesNotThrow(string input, object expected) { + FixupSpans = true; + // Act & Assert ParseBlockTest(input, (Block)expected); } @@ -1217,14 +1223,22 @@ catch(bar) { baz(); }", BlockType.Statement, SpanKind.Code); Factory.Code(postComment).AsStatement().Accepts(acceptedCharacters))); } - private void RunSimpleWrappedMarkupTest(string prefix, string markup, string suffix, MarkupBlock expectedMarkup, AcceptedCharacters acceptedCharacters = AcceptedCharacters.Any) + private void RunSimpleWrappedMarkupTest(string prefix, string markup, string suffix, MarkupBlock expectedMarkup, SourceLocation expectedStart, AcceptedCharacters acceptedCharacters = AcceptedCharacters.Any) { - ParseBlockTest(prefix + markup + suffix, - new StatementBlock( - Factory.Code(prefix).AsStatement(), - expectedMarkup, - Factory.Code(suffix).AsStatement().Accepts(acceptedCharacters) - )); + var expected = new StatementBlock( + Factory.Code(prefix).AsStatement(), + expectedMarkup, + Factory.Code(suffix).AsStatement().Accepts(acceptedCharacters)); + + // Since we're building the 'expected' input out of order we need to do some trickery + // to get the locations right. + SpancestryCorrector.Correct(expected); + expected.FindFirstDescendentSpan().ChangeStart(SourceLocation.Zero); + + // We make the caller pass a start location so we can verify that nothing has gone awry. + Assert.Equal(expectedStart, expectedMarkup.Start); + + ParseBlockTest(prefix + markup + suffix, expected); } private void NamespaceImportTest(string content, string expectedNS, AcceptedCharacters acceptedCharacters = AcceptedCharacters.None, string errorMessage = null, SourceLocation? location = null) diff --git a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpRazorCommentsTest.cs b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpRazorCommentsTest.cs index 7262353f34..9053035e25 100644 --- a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpRazorCommentsTest.cs +++ b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpRazorCommentsTest.cs @@ -114,7 +114,7 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy length: 1)); } - [Fact] + [Fact(Skip = "Fails due to https://github.com/aspnet/Razor/issues/897")] public void RazorCommentInVerbatimBlock() { ParseDocumentTest("@{" + Environment.NewLine diff --git a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpSectionTest.cs b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpSectionTest.cs index fd1ad152b0..32104c88ec 100644 --- a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpSectionTest.cs +++ b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpSectionTest.cs @@ -696,6 +696,8 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy [MemberData(nameof(SectionWithEscapedTransitionData))] public void ParseSectionBlock_WithDoubleTransition_DoesNotThrow(string input, object expected) { + FixupSpans = true; + ParseDocumentTest(input, (Block)expected); } } diff --git a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpStatementTest.cs b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpStatementTest.cs index 997439e4f9..b8f0284942 100644 --- a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpStatementTest.cs +++ b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpStatementTest.cs @@ -237,6 +237,8 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy [MemberData(nameof(ExceptionFilterData))] public void ExceptionFilters(string document, object expectedStatement) { + FixupSpans = true; + // Act & Assert ParseBlockTest(document, (StatementBlock)expectedStatement); } @@ -292,6 +294,8 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy object expectedStatement, object expectedErrors) { + FixupSpans = true; + // Act & Assert ParseBlockTest(document, (StatementBlock)expectedStatement, (RazorError[])expectedErrors); } @@ -351,6 +355,8 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy [MemberData(nameof(StaticUsingData))] public void StaticUsingImport(string document, object expectedResult) { + FixupSpans = true; + // Act & Assert ParseBlockTest(document, (DirectiveBlock)expectedResult); } diff --git a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpTemplateTest.cs b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpTemplateTest.cs index 42ae3b76f5..b2097b365a 100644 --- a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpTemplateTest.cs +++ b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpTemplateTest.cs @@ -270,6 +270,8 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy [Fact] public void ParseBlock_WithDoubleTransition_DoesNotThrow() { + FixupSpans = true; + // Arrange var testTemplateWithDoubleTransitionCode = " @

Foo #@item

"; var testTemplateWithDoubleTransition = new TemplateBlock( diff --git a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpToMarkupSwitchTest.cs b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpToMarkupSwitchTest.cs index fd61aeee13..580eaf7e9a 100644 --- a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpToMarkupSwitchTest.cs +++ b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/CSharpToMarkupSwitchTest.cs @@ -605,7 +605,7 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy Factory.MetaCode("}").Accepts(AcceptedCharacters.None))); } - [Fact] + [Fact(Skip = "Fails due to https://github.com/aspnet/Razor/issues/897")] public void ParseBlockCorrectlyReturnsFromMarkupBlockWithPseudoTag() { ParseBlockTest("if (i > 0) { ; }", @@ -620,7 +620,7 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy Factory.Code(" }").AsStatement())); } - [Fact] + [Fact(Skip = "Fails due to https://github.com/aspnet/Razor/issues/897")] public void ParseBlockCorrectlyReturnsFromMarkupBlockWithPseudoTagInCodeBlock() { ParseBlockTest("{ if (i > 0) { ; } }", @@ -639,7 +639,7 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy Factory.MetaCode("}").Accepts(AcceptedCharacters.None))); } - [Fact] + [Fact(Skip = "Fails due to https://github.com/aspnet/Razor/issues/897")] public void ParseBlockSupportsAllKindsOfImplicitMarkupInCodeBlock() { ParseBlockTest("{" + Environment.NewLine diff --git a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/HtmlBlockTest.cs b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/HtmlBlockTest.cs index e1392fcca6..00fc85e305 100644 --- a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/HtmlBlockTest.cs +++ b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/HtmlBlockTest.cs @@ -390,6 +390,8 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy [MemberData(nameof(HtmlCommentSupportsMultipleDashesData))] public void HtmlCommentSupportsMultipleDashes(string documentContent, object expectedOutput) { + FixupSpans = true; + ParseBlockTest(documentContent, (MarkupBlock)expectedOutput); } diff --git a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/HtmlDocumentTest.cs b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/HtmlDocumentTest.cs index c397aecd94..b2348605e3 100644 --- a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/HtmlDocumentTest.cs +++ b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/HtmlDocumentTest.cs @@ -757,6 +757,8 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy [MemberData(nameof(BlockWithEscapedTransitionData))] public void ParseBlock_WithDoubleTransition_DoesNotThrow(string input, object expected) { + FixupSpans = true; + // Act & Assert ParseDocumentTest(input, (Block)expected); } diff --git a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/ParserTestBase.cs b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/ParserTestBase.cs index 98ce68d4de..84712fe8f5 100644 --- a/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/ParserTestBase.cs +++ b/test/Microsoft.AspNetCore.Razor.Evolution.Test/Legacy/ParserTestBase.cs @@ -14,15 +14,22 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy { internal static Block IgnoreOutput = new IgnoreOutputBlock(); - internal SpanFactory Factory { get; private set; } - internal BlockFactory BlockFactory { get; private set; } - internal ParserTestBase() { Factory = CreateSpanFactory(); BlockFactory = CreateBlockFactory(); } + /// + /// Set to true to autocorrect the locations of spans to appear in document order with no gaps. + /// Use this when spans were not created in document order. + /// + protected bool FixupSpans { get; set; } + + internal SpanFactory Factory { get; private set; } + + internal BlockFactory BlockFactory { get; private set; } + internal abstract RazorSyntaxTree ParseBlock(string document, bool designTime); internal virtual RazorSyntaxTree ParseDocument(string document, bool designTime = false) @@ -149,6 +156,17 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy { var result = ParseBlock(document, designTime); + if (FixupSpans) + { + SpancestryCorrector.Correct(expected); + + var span = expected.FindFirstDescendentSpan(); + span.ChangeStart(SourceLocation.Zero); + } + + SyntaxTreeVerifier.Verify(result); + SyntaxTreeVerifier.Verify(expected); + if (!ReferenceEquals(expected, IgnoreOutput)) { EvaluateResults(result, expected, expectedErrors); @@ -188,6 +206,17 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy builder.Type = blockType; var expected = ConfigureAndAddSpanToBlock(builder, Factory.Span(spanType, spanContent, spanType == SpanKind.Markup).Accepts(acceptedCharacters)); + if (FixupSpans) + { + SpancestryCorrector.Correct(expected); + + var span = expected.FindFirstDescendentSpan(); + span.ChangeStart(SourceLocation.Zero); + } + + SyntaxTreeVerifier.Verify(result); + SyntaxTreeVerifier.Verify(expected); + if (!ReferenceEquals(expected, IgnoreOutput)) { EvaluateResults(result, expected, expectedErrors); @@ -219,33 +248,25 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy ParseDocumentTest(document, expectedRoot, designTime, null); } - internal virtual void ParseDocumentTest(string document, Block expectedRoot, bool designTime, params RazorError[] expectedErrors) + internal virtual void ParseDocumentTest(string document, Block expected, bool designTime, params RazorError[] expectedErrors) { var result = ParseDocument(document, designTime); - if (!ReferenceEquals(expectedRoot, IgnoreOutput)) + if (FixupSpans) { - EvaluateResults(result, expectedRoot, expectedErrors); + SpancestryCorrector.Correct(expected); + + var span = expected.FindFirstDescendentSpan(); + span.ChangeStart(SourceLocation.Zero); } - } - internal virtual RazorSyntaxTree RunParse( - string document, - Func parserActionSelector, - bool designTimeParser, - Func parserSelector = null, - ErrorSink errorSink = null) - { - throw null; - } + SyntaxTreeVerifier.Verify(result); + SyntaxTreeVerifier.Verify(expected); - internal virtual void RunParseTest( - string document, - Func parserActionSelector, - Block expectedRoot, IList expectedErrors, - bool designTimeParser, Func parserSelector = null) - { - throw null; + if (!ReferenceEquals(expected, IgnoreOutput)) + { + EvaluateResults(result, expected, expectedErrors); + } } [Conditional("PARSER_TRACE")] @@ -547,5 +568,41 @@ namespace Microsoft.AspNetCore.Razor.Evolution.Legacy { public IgnoreOutputBlock() : base(BlockType.Template, new SyntaxTreeNode[0], null) { } } + + // Corrects the parents and previous/next information for spans + internal class SpancestryCorrector : ParserVisitor + { + private SpancestryCorrector() + { + } + + protected Block CurrentBlock { get; set; } + + protected Span LastSpan { get; set; } + + public static void Correct(Block block) + { + new SpancestryCorrector().VisitBlock(block); + } + + public override void VisitBlock(Block block) + { + CurrentBlock = block; + base.VisitBlock(block); + } + + public override void VisitSpan(Span span) + { + span.Parent = CurrentBlock; + + span.Previous = LastSpan; + if (LastSpan != null) + { + LastSpan.Next = span; + } + + LastSpan = span; + } + } } } \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Razor.Evolution.Test/SyntaxTreeVerifier.cs b/test/Microsoft.AspNetCore.Razor.Evolution.Test/SyntaxTreeVerifier.cs new file mode 100644 index 0000000000..08a0da0db1 --- /dev/null +++ b/test/Microsoft.AspNetCore.Razor.Evolution.Test/SyntaxTreeVerifier.cs @@ -0,0 +1,42 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using Microsoft.AspNetCore.Razor.Evolution.Legacy; + +namespace Microsoft.AspNetCore.Razor.Evolution +{ + // Verifies recursively that a syntax tree has no gaps in terms of position/location. + internal class SyntaxTreeVerifier : ParserVisitor + { + private readonly SourceLocationTracker _tracker = new SourceLocationTracker(SourceLocation.Zero); + + private SyntaxTreeVerifier() + { + } + + public static void Verify(RazorSyntaxTree syntaxTree) + { + Verify(syntaxTree.Root); + } + + public static void Verify(Block block) + { + new SyntaxTreeVerifier().VisitBlock(block); + } + + public override void VisitSpan(Span span) + { + var start = span.Start; + if (!start.Equals(_tracker.CurrentLocation)) + { + throw new InvalidOperationException($"Span starting at {span.Start} should start at {_tracker.CurrentLocation} - {span} "); + } + + for (var i = 0; i < span.Symbols.Count; i++) + { + _tracker.UpdateLocation(span.Symbols[i].Content); + } + } + } +}