From 344862fbc349260d13fb9f927b9d29aa59a854e4 Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Thu, 20 Apr 2017 12:38:48 -0700 Subject: [PATCH] Change string token parsing to not flow errored tokens. - Found that our extensible directive string parsing system wasn't consistent with the rest of the extensible directive tokens. Basically, if there were malformed string tokens we'd consume them and pass them along to extensible directive passes. This was a big no-no because it means extensible directive passes weren't able to rely on tokens being passed to them being well-formed. - Fixed up existing extensible directive tests that relied on output of string tokens. #1247 --- .../PageDirective.cs | 12 +++---- .../Legacy/CSharpCodeParser.cs | 9 +++-- ...lformedPageDirective_DesignTime.codegen.cs | 4 --- .../MalformedPageDirective_DesignTime.ir.txt | 5 ++- .../MalformedPageDirective_Runtime.codegen.cs | 4 +-- .../MalformedPageDirective_Runtime.ir.txt | 6 ++-- .../DirectiveRemovalIROptimizationPassTest.cs | 4 +-- .../Legacy/CSharpDirectivesTest.cs | 33 +++++-------------- 8 files changed, 25 insertions(+), 52 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/PageDirective.cs b/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/PageDirective.cs index 014587d2c1..b46fb85612 100644 --- a/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/PageDirective.cs +++ b/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/PageDirective.cs @@ -67,15 +67,11 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions private static string TrimQuotes(string content) { - if (content.Length >= 2 && - content.StartsWith("\"", StringComparison.Ordinal) && - content.EndsWith("\"", StringComparison.Ordinal)) - { - return content.Substring(1, content.Length - 2); - } + Debug.Assert(content.Length >= 2); + Debug.Assert(content.StartsWith("\"", StringComparison.Ordinal)); + Debug.Assert(content.EndsWith("\"", StringComparison.Ordinal)); - // The extensible directive system handed us invalid content. There's already an error logged. - return null; + return content.Substring(1, content.Length - 2); } private class Visitor : RazorIRNodeWalker diff --git a/src/Microsoft.AspNetCore.Razor.Language/Legacy/CSharpCodeParser.cs b/src/Microsoft.AspNetCore.Razor.Language/Legacy/CSharpCodeParser.cs index ca206343a5..67c72f9712 100644 --- a/src/Microsoft.AspNetCore.Razor.Language/Legacy/CSharpCodeParser.cs +++ b/src/Microsoft.AspNetCore.Razor.Language/Legacy/CSharpCodeParser.cs @@ -1614,18 +1614,17 @@ namespace Microsoft.AspNetCore.Razor.Language.Legacy break; case DirectiveTokenKind.String: - if (At(CSharpSymbolType.StringLiteral)) + if (At(CSharpSymbolType.StringLiteral) && CurrentSymbol.Errors.Count == 0) { AcceptAndMoveNext(); } else { - var startLocation = CurrentStart; - AcceptUntil(CSharpSymbolType.WhiteSpace, CSharpSymbolType.NewLine); Context.ErrorSink.OnError( - startLocation, + CurrentStart, LegacyResources.FormatDirectiveExpectsQuotedStringLiteral(descriptor.Name), - Span.End.AbsoluteIndex - Span.Start.AbsoluteIndex); + CurrentSymbol.Content.Length); + return; } break; } diff --git a/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_DesignTime.codegen.cs b/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_DesignTime.codegen.cs index 26ffa461cd..e7dac6c06f 100644 --- a/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_DesignTime.codegen.cs +++ b/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_DesignTime.codegen.cs @@ -13,10 +13,6 @@ namespace AspNetCore { #pragma warning disable 219 private void __RazorDirectiveTokenHelpers__() { - ((System.Action)(() => { -global::System.Object __typeHelper = "foo; - } - ))(); } #pragma warning restore 219 private static System.Object __o = null; diff --git a/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_DesignTime.ir.txt b/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_DesignTime.ir.txt index a4788b3a3c..56f9d162c4 100644 --- a/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_DesignTime.ir.txt +++ b/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_DesignTime.ir.txt @@ -24,12 +24,11 @@ Document - DirectiveToken - (586:11,14 [96] ) - Microsoft.AspNetCore.Mvc.Razor.TagHelpers.UrlResolutionTagHelper, Microsoft.AspNetCore.Mvc.Razor DirectiveToken - (698:12,14 [87] ) - Microsoft.AspNetCore.Mvc.Razor.TagHelpers.HeadTagHelper, Microsoft.AspNetCore.Mvc.Razor DirectiveToken - (801:13,14 [87] ) - Microsoft.AspNetCore.Mvc.Razor.TagHelpers.BodyTagHelper, Microsoft.AspNetCore.Mvc.Razor - DirectiveToken - (6:0,6 [4] MalformedPageDirective.cshtml) - "foo CSharpStatement - RazorIRToken - - CSharp - private static System.Object __o = null; RazorMethodDeclaration - - public - async, override - global::System.Threading.Tasks.Task - ExecuteAsync - HtmlContent - (12:1,0 [43] MalformedPageDirective.cshtml) - RazorIRToken - (12:1,0 [2] MalformedPageDirective.cshtml) - Html - \n + HtmlContent - (6:0,6 [49] MalformedPageDirective.cshtml) + RazorIRToken - (6:0,6 [8] MalformedPageDirective.cshtml) - Html - "foo\n\n RazorIRToken - (14:2,0 [4] MalformedPageDirective.cshtml) - Html -

RazorIRToken - (18:2,4 [8] MalformedPageDirective.cshtml) - Html - About Us RazorIRToken - (26:2,12 [5] MalformedPageDirective.cshtml) - Html -

diff --git a/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_Runtime.codegen.cs b/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_Runtime.codegen.cs index 77f9668a85..c1d44cdda6 100644 --- a/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_Runtime.codegen.cs +++ b/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_Runtime.codegen.cs @@ -14,8 +14,8 @@ namespace AspNetCore #pragma warning disable 1998 public async override global::System.Threading.Tasks.Task ExecuteAsync() { - BeginContext(12, 43, true); - WriteLiteral("\r\n

About Us

\r\n

We are awesome.

"); + BeginContext(6, 49, true); + WriteLiteral("\"foo\r\n\r\n

About Us

\r\n

We are awesome.

"); EndContext(); } #pragma warning restore 1998 diff --git a/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_Runtime.ir.txt b/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_Runtime.ir.txt index 4553832fcd..5718255912 100644 --- a/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_Runtime.ir.txt +++ b/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_Runtime.ir.txt @@ -11,9 +11,9 @@ Document - ClassDeclaration - - public - TestFiles_IntegrationTests_CodeGenerationIntegrationTest_MalformedPageDirective_cshtml - global::Microsoft.AspNetCore.Mvc.RazorPages.Page - RazorMethodDeclaration - - public - async, override - global::System.Threading.Tasks.Task - ExecuteAsync CSharpStatement - - RazorIRToken - - CSharp - BeginContext(12, 43, true); - HtmlContent - (12:1,0 [43] MalformedPageDirective.cshtml) - RazorIRToken - (12:1,0 [2] MalformedPageDirective.cshtml) - Html - \n + RazorIRToken - - CSharp - BeginContext(6, 49, true); + HtmlContent - (6:0,6 [49] MalformedPageDirective.cshtml) + RazorIRToken - (6:0,6 [8] MalformedPageDirective.cshtml) - Html - "foo\n\n RazorIRToken - (14:2,0 [4] MalformedPageDirective.cshtml) - Html -

RazorIRToken - (18:2,4 [8] MalformedPageDirective.cshtml) - Html - About Us RazorIRToken - (26:2,12 [5] MalformedPageDirective.cshtml) - Html -

diff --git a/test/Microsoft.AspNetCore.Razor.Language.Test/DirectiveRemovalIROptimizationPassTest.cs b/test/Microsoft.AspNetCore.Razor.Language.Test/DirectiveRemovalIROptimizationPassTest.cs index 37982be75f..2aeb98d8d5 100644 --- a/test/Microsoft.AspNetCore.Razor.Language.Test/DirectiveRemovalIROptimizationPassTest.cs +++ b/test/Microsoft.AspNetCore.Razor.Language.Test/DirectiveRemovalIROptimizationPassTest.cs @@ -14,7 +14,7 @@ namespace Microsoft.AspNetCore.Razor.Language public void Execute_Custom_RemovesDirectiveIRNodeFromIRDocument() { // Arrange - var content = "@custom Hello"; + var content = "@custom \"Hello\""; var sourceDocument = TestRazorSourceDocument.Create(content); var codeDocument = RazorCodeDocument.Create(sourceDocument); var defaultEngine = RazorEngine.Create(b => @@ -49,7 +49,7 @@ namespace Microsoft.AspNetCore.Razor.Language public void Execute_MultipleCustomDirectives_RemovesDirectiveIRNodesFromIRDocument() { // Arrange - var content = "@custom Hello" + Environment.NewLine + "@custom World"; + var content = "@custom \"Hello\"" + Environment.NewLine + "@custom \"World\""; var sourceDocument = TestRazorSourceDocument.Create(content); var codeDocument = RazorCodeDocument.Create(sourceDocument); var defaultEngine = RazorEngine.Create(b => diff --git a/test/Microsoft.AspNetCore.Razor.Language.Test/Legacy/CSharpDirectivesTest.cs b/test/Microsoft.AspNetCore.Razor.Language.Test/Legacy/CSharpDirectivesTest.cs index e56b58b482..b909ce8eac 100644 --- a/test/Microsoft.AspNetCore.Razor.Language.Test/Legacy/CSharpDirectivesTest.cs +++ b/test/Microsoft.AspNetCore.Razor.Language.Test/Legacy/CSharpDirectivesTest.cs @@ -128,10 +128,7 @@ namespace Microsoft.AspNetCore.Razor.Language.Legacy new DirectiveChunkGenerator(descriptor), Factory.CodeTransition(), Factory.MetaCode("custom").Accepts(AcceptedCharacters.None), - Factory.Span(SpanKind.Markup, " ", markup: false).Accepts(AcceptedCharacters.WhiteSpace), - Factory.Span(SpanKind.Markup, "AString", markup: false) - .With(new DirectiveTokenChunkGenerator(descriptor.Tokens[0])) - .Accepts(AcceptedCharacters.NonWhiteSpace)), expectedError); + Factory.Span(SpanKind.Markup, " ", markup: false).Accepts(AcceptedCharacters.WhiteSpace)), expectedError); } [Fact] @@ -142,7 +139,7 @@ namespace Microsoft.AspNetCore.Razor.Language.Legacy var expectedError = new RazorError( LegacyResources.FormatDirectiveExpectsQuotedStringLiteral("custom"), new SourceLocation(8, 0, 8), - length: 6); + length: 1); // Act & Assert ParseCodeBlockTest( @@ -152,10 +149,7 @@ namespace Microsoft.AspNetCore.Razor.Language.Legacy new DirectiveChunkGenerator(descriptor), Factory.CodeTransition(), Factory.MetaCode("custom").Accepts(AcceptedCharacters.None), - Factory.Span(SpanKind.Markup, " ", markup: false).Accepts(AcceptedCharacters.WhiteSpace), - Factory.Span(SpanKind.Markup, "{foo?}", markup: false) - .With(new DirectiveTokenChunkGenerator(descriptor.Tokens[0])) - .Accepts(AcceptedCharacters.NonWhiteSpace)), expectedError); + Factory.Span(SpanKind.Markup, " ", markup: false).Accepts(AcceptedCharacters.WhiteSpace)), expectedError); } [Fact] @@ -176,10 +170,7 @@ namespace Microsoft.AspNetCore.Razor.Language.Legacy new DirectiveChunkGenerator(descriptor), Factory.CodeTransition(), Factory.MetaCode("custom").Accepts(AcceptedCharacters.None), - Factory.Span(SpanKind.Markup, " ", markup: false).Accepts(AcceptedCharacters.WhiteSpace), - Factory.Span(SpanKind.Markup, "'AString'", markup: false) - .With(new DirectiveTokenChunkGenerator(descriptor.Tokens[0])) - .Accepts(AcceptedCharacters.NonWhiteSpace)), expectedError); + Factory.Span(SpanKind.Markup, " ", markup: false).Accepts(AcceptedCharacters.WhiteSpace)), expectedError); } [Fact] @@ -187,14 +178,10 @@ namespace Microsoft.AspNetCore.Razor.Language.Legacy { // Arrange var descriptor = DirectiveDescriptorBuilder.Create("custom").AddString().Build(); - var expectedError1 = new RazorError( - LegacyResources.ParseError_Unterminated_String_Literal, - new SourceLocation(15, 0, 15), - length: 1); - var expectedError2 = new RazorError( + var expectedError = new RazorError( LegacyResources.FormatDirectiveExpectsQuotedStringLiteral("custom"), new SourceLocation(8, 0, 8), - length: 8); + length: 7); // Act & Assert ParseCodeBlockTest( @@ -204,12 +191,8 @@ namespace Microsoft.AspNetCore.Razor.Language.Legacy new DirectiveChunkGenerator(descriptor), Factory.CodeTransition(), Factory.MetaCode("custom").Accepts(AcceptedCharacters.None), - Factory.Span(SpanKind.Markup, " ", markup: false).Accepts(AcceptedCharacters.WhiteSpace), - Factory.Span(SpanKind.Markup, "AString\"", markup: false) - .With(new DirectiveTokenChunkGenerator(descriptor.Tokens[0])) - .Accepts(AcceptedCharacters.NonWhiteSpace)), - expectedError1, - expectedError2); + Factory.Span(SpanKind.Markup, " ", markup: false).Accepts(AcceptedCharacters.WhiteSpace)), + expectedError); } [Fact]