From d36838ed883a84d9f85c8637c1f994c4e835086b Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Mon, 3 Jul 2017 14:57:23 -0700 Subject: [PATCH] Handle imported @page directives. - Prior to this imported `@page` directives would flow through the Razor system without error. This resulted in inconsistent behavior between MVC and Razor. Now, imported `@page` directives result in diagnostics on the page directive node. - Added two tests to verify that we still treat views with imported page directives as Razor pages BUT we also log a diagnostic on the page directive node. - Renamed the `ViewComponentDiagnosticFactory` class to `RazorExtensionsDiagnosticFactory` so it can be used for more than just view component diagnostics. This way we can ensure that our diagnostics don't end up overlapping. #1503 --- .../Properties/Resources.Designer.cs | 14 +++++++++ ...cs => RazorExtensionsDiagnosticFactory.cs} | 17 ++++++++++- .../RazorPageDocumentClassifierPass.cs | 29 +++++++++++++++++++ .../Resources.resx | 3 ++ ...ViewComponentTagHelperDescriptorFactory.cs | 12 ++++---- ...faultRazorIntermediateNodeLoweringPhase.cs | 3 +- .../IntermediateNodeExtensions.cs | 5 ++++ .../PageDirectiveTest.cs | 20 +++++++++++++ .../RazorPageDocumentClassifierPassTest.cs | 26 +++++++++++++++++ ...ComponentTagHelperDescriptorFactoryTest.cs | 12 ++++---- 10 files changed, 126 insertions(+), 15 deletions(-) rename src/Microsoft.AspNetCore.Mvc.Razor.Extensions/{ViewComponentDiagnosticFactory.cs => RazorExtensionsDiagnosticFactory.cs} (85%) diff --git a/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/Properties/Resources.Designer.cs b/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/Properties/Resources.Designer.cs index 97d9dc35b9..ccbf5f33b0 100644 --- a/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/Properties/Resources.Designer.cs @@ -94,6 +94,20 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions internal static string FormatMvcRazorParser_InvalidPropertyType(object p0, object p1, object p2) => string.Format(CultureInfo.CurrentCulture, GetString("MvcRazorParser_InvalidPropertyType"), p0, p1, p2); + /// + /// The '@{0}' directive specified in {1} file will not be imported. The directive must appear at the top of each Razor cshtml file. + /// + internal static string PageDirectiveCannotBeImported + { + get => GetString("PageDirectiveCannotBeImported"); + } + + /// + /// The '@{0}' directive specified in {1} file will not be imported. The directive must appear at the top of each Razor cshtml file. + /// + internal static string FormatPageDirectiveCannotBeImported(object p0, object p1) + => string.Format(CultureInfo.CurrentCulture, GetString("PageDirectiveCannotBeImported"), p0, p1); + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/ViewComponentDiagnosticFactory.cs b/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/RazorExtensionsDiagnosticFactory.cs similarity index 85% rename from src/Microsoft.AspNetCore.Mvc.Razor.Extensions/ViewComponentDiagnosticFactory.cs rename to src/Microsoft.AspNetCore.Mvc.Razor.Extensions/RazorExtensionsDiagnosticFactory.cs index 0584cd78d4..565d4caa0b 100644 --- a/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/ViewComponentDiagnosticFactory.cs +++ b/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/RazorExtensionsDiagnosticFactory.cs @@ -1,12 +1,13 @@ // 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.IO; using System.Threading.Tasks; using Microsoft.AspNetCore.Razor.Language; namespace Microsoft.AspNetCore.Mvc.Razor.Extensions { - internal class ViewComponentDiagnosticFactory + internal class RazorExtensionsDiagnosticFactory { private const string DiagnosticPrefix = "RZ"; @@ -98,5 +99,19 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions return diagnostic; } + + public static readonly RazorDiagnosticDescriptor PageDirective_CannotBeImported = + new RazorDiagnosticDescriptor( + $"{DiagnosticPrefix}3905", + () => Resources.PageDirectiveCannotBeImported, + RazorDiagnosticSeverity.Error); + + public static RazorDiagnostic CreatePageDirective_CannotBeImported(SourceSpan source) + { + var fileName = Path.GetFileName(source.FilePath); + var diagnostic = RazorDiagnostic.Create(PageDirective_CannotBeImported, source, PageDirective.Directive.Directive, fileName); + + return diagnostic; + } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/RazorPageDocumentClassifierPass.cs b/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/RazorPageDocumentClassifierPass.cs index de8c165e5b..90ea67329e 100644 --- a/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/RazorPageDocumentClassifierPass.cs +++ b/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/RazorPageDocumentClassifierPass.cs @@ -1,6 +1,7 @@ // 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.Language; using Microsoft.AspNetCore.Razor.Language.Intermediate; @@ -41,6 +42,34 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions method.Modifiers.Add("async"); method.Modifiers.Add("override"); method.ReturnType = $"global::{typeof(System.Threading.Tasks.Task).FullName}"; + + EnsureValidPageDirective(codeDocument); + } + + private void EnsureValidPageDirective(RazorCodeDocument codeDocument) + { + var document = codeDocument.GetDocumentIntermediateNode(); + var visitor = new Visitor(); + visitor.VisitDocument(document); + + if (visitor.DirectiveNode.IsImported()) + { + visitor.DirectiveNode.Diagnostics.Add( + RazorExtensionsDiagnosticFactory.CreatePageDirective_CannotBeImported(visitor.DirectiveNode.Source.Value)); + } + } + + private class Visitor : IntermediateNodeWalker + { + public DirectiveIntermediateNode DirectiveNode { get; private set; } + + public override void VisitDirective(DirectiveIntermediateNode node) + { + if (node.Descriptor == PageDirective.Directive) + { + DirectiveNode = node; + } + } } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/Resources.resx b/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/Resources.resx index a1c3afc652..6133d1dde0 100644 --- a/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/Resources.resx +++ b/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/Resources.resx @@ -135,4 +135,7 @@ Invalid tag helper property '{0}.{1}'. Dictionary values must not be of type '{2}'. + + The '@{0}' directive specified in {1} file will not be imported. The directive must appear at the top of each Razor cshtml file. + \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/ViewComponentTagHelperDescriptorFactory.cs b/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/ViewComponentTagHelperDescriptorFactory.cs index 48d6eed74b..cb64a4611a 100644 --- a/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/ViewComponentTagHelperDescriptorFactory.cs +++ b/src/Microsoft.AspNetCore.Mvc.Razor.Extensions/ViewComponentTagHelperDescriptorFactory.cs @@ -94,13 +94,13 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions if (methods.Length == 0) { - diagnostic = ViewComponentDiagnosticFactory.CreateViewComponent_CannotFindMethod(type.ToDisplayString(FullNameTypeDisplayFormat)); + diagnostic = RazorExtensionsDiagnosticFactory.CreateViewComponent_CannotFindMethod(type.ToDisplayString(FullNameTypeDisplayFormat)); method = null; return false; } else if (methods.Length > 1) { - diagnostic = ViewComponentDiagnosticFactory.CreateViewComponent_AmbiguousMethods(type.ToDisplayString(FullNameTypeDisplayFormat)); + diagnostic = RazorExtensionsDiagnosticFactory.CreateViewComponent_AmbiguousMethods(type.ToDisplayString(FullNameTypeDisplayFormat)); method = null; return false; } @@ -120,7 +120,7 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions } else { - diagnostic = ViewComponentDiagnosticFactory.CreateViewComponent_AsyncMethod_ShouldReturnTask(type.ToDisplayString(FullNameTypeDisplayFormat)); + diagnostic = RazorExtensionsDiagnosticFactory.CreateViewComponent_AsyncMethod_ShouldReturnTask(type.ToDisplayString(FullNameTypeDisplayFormat)); method = null; return false; } @@ -130,19 +130,19 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions // Will invoke synchronously. Method must not return void, Task or Task. if (returnType.SpecialType == SpecialType.System_Void) { - diagnostic = ViewComponentDiagnosticFactory.CreateViewComponent_SyncMethod_ShouldReturnValue(type.ToDisplayString(FullNameTypeDisplayFormat)); + diagnostic = RazorExtensionsDiagnosticFactory.CreateViewComponent_SyncMethod_ShouldReturnValue(type.ToDisplayString(FullNameTypeDisplayFormat)); method = null; return false; } else if (returnType == _taskSymbol) { - diagnostic = ViewComponentDiagnosticFactory.CreateViewComponent_SyncMethod_CannotReturnTask(type.ToDisplayString(FullNameTypeDisplayFormat)); + diagnostic = RazorExtensionsDiagnosticFactory.CreateViewComponent_SyncMethod_CannotReturnTask(type.ToDisplayString(FullNameTypeDisplayFormat)); method = null; return false; } else if (returnType.IsGenericType && returnType.ConstructedFrom == _genericTaskSymbol) { - diagnostic = ViewComponentDiagnosticFactory.CreateViewComponent_SyncMethod_CannotReturnTask(type.ToDisplayString(FullNameTypeDisplayFormat)); + diagnostic = RazorExtensionsDiagnosticFactory.CreateViewComponent_SyncMethod_CannotReturnTask(type.ToDisplayString(FullNameTypeDisplayFormat)); method = null; return false; } diff --git a/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorIntermediateNodeLoweringPhase.cs b/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorIntermediateNodeLoweringPhase.cs index 5fa40bfe73..670656ad8b 100644 --- a/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorIntermediateNodeLoweringPhase.cs +++ b/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorIntermediateNodeLoweringPhase.cs @@ -109,9 +109,8 @@ namespace Microsoft.AspNetCore.Razor.Language var directive = (DirectiveIntermediateNode)reference.Node; var descriptor = directive.Descriptor; var seenDirective = !seenDirectives.Add(descriptor); - var imported = ReferenceEquals(directive.Annotations[CommonAnnotations.Imported], CommonAnnotations.Imported); - if (!imported) + if (!directive.IsImported()) { continue; } diff --git a/src/Microsoft.AspNetCore.Razor.Language/Intermediate/IntermediateNodeExtensions.cs b/src/Microsoft.AspNetCore.Razor.Language/Intermediate/IntermediateNodeExtensions.cs index 708ce3d643..6af13aab2c 100644 --- a/src/Microsoft.AspNetCore.Razor.Language/Intermediate/IntermediateNodeExtensions.cs +++ b/src/Microsoft.AspNetCore.Razor.Language/Intermediate/IntermediateNodeExtensions.cs @@ -11,6 +11,11 @@ namespace Microsoft.AspNetCore.Razor.Language.Intermediate { private static readonly IReadOnlyList EmptyDiagnostics = Array.Empty(); + public static bool IsImported(this IntermediateNode node) + { + return ReferenceEquals(node.Annotations[CommonAnnotations.Imported], CommonAnnotations.Imported); + } + public static IReadOnlyList GetAllDiagnostics(this IntermediateNode node) { if (node == null) diff --git a/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/PageDirectiveTest.cs b/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/PageDirectiveTest.cs index 00ab029435..19c27dca79 100644 --- a/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/PageDirectiveTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/PageDirectiveTest.cs @@ -1,6 +1,7 @@ // 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.Language; using Microsoft.AspNetCore.Razor.Language.Intermediate; using Xunit; @@ -9,6 +10,25 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions { public class PageDirectiveTest { + [Fact] + public void TryGetPageDirective_ReturnsTrue_IfPageIsImported() + { + // Arrange + var content = "Hello world"; + var sourceDocument = RazorSourceDocument.Create(content, "file"); + var importDocument = RazorSourceDocument.Create("@page", "imports.cshtml"); + var codeDocument = RazorCodeDocument.Create(sourceDocument, new[] { importDocument }); + var engine = CreateEngine(); + var irDocument = CreateIRDocument(engine, codeDocument); + + // Act + var result = PageDirective.TryGetPageDirective(irDocument, out var pageDirective); + + // Assert + Assert.True(result); + Assert.Null(pageDirective.RouteTemplate); + } + [Fact] public void TryGetPageDirective_ReturnsFalse_IfPageDoesNotHaveDirective() { diff --git a/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/RazorPageDocumentClassifierPassTest.cs b/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/RazorPageDocumentClassifierPassTest.cs index f805e9571e..a8ec5c0b2b 100644 --- a/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/RazorPageDocumentClassifierPassTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/RazorPageDocumentClassifierPassTest.cs @@ -10,6 +10,32 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions { public class RazorPageDocumentClassifierPassTest { + [Fact] + public void RazorPageDocumentClassifierPass_LogsErrorForImportedPageDirectives() + { + // Arrange + var sourceSpan = new SourceSpan("import.cshtml", 0, 0, 0, 5); + var expectedDiagnostic = RazorExtensionsDiagnosticFactory.CreatePageDirective_CannotBeImported(sourceSpan); + var importDocument = RazorSourceDocument.Create("@page", "import.cshtml"); + var sourceDocument = RazorSourceDocument.Create("

Hello World

", "main.cshtml"); + var codeDocument = RazorCodeDocument.Create(sourceDocument, new[] { importDocument }); + var engine = CreateEngine(); + var irDocument = CreateIRDocument(engine, codeDocument); + var pass = new RazorPageDocumentClassifierPass + { + Engine = engine + }; + + // Act + pass.Execute(codeDocument, irDocument); + + // Assert + var pageDirectives = irDocument.FindDirectiveReferences(PageDirective.Directive); + var directive = Assert.Single(pageDirectives); + var diagnostic = Assert.Single(directive.Node.Diagnostics); + Assert.Equal(expectedDiagnostic, diagnostic); + } + [Fact] public void RazorPageDocumentClassifierPass_SetsDocumentKind() { diff --git a/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/ViewComponentTagHelperDescriptorFactoryTest.cs b/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/ViewComponentTagHelperDescriptorFactoryTest.cs index d545b62abe..aa0065c770 100644 --- a/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/ViewComponentTagHelperDescriptorFactoryTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/ViewComponentTagHelperDescriptorFactoryTest.cs @@ -159,7 +159,7 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions // Assert var diagnostic = Assert.Single(descriptor.GetAllDiagnostics()); - Assert.Equal(ViewComponentDiagnosticFactory.ViewComponent_CannotFindMethod.Id, diagnostic.Id); + Assert.Equal(RazorExtensionsDiagnosticFactory.ViewComponent_CannotFindMethod.Id, diagnostic.Id); } [Fact] @@ -208,7 +208,7 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions // Assert var diagnostic = Assert.Single(descriptor.GetAllDiagnostics()); - Assert.Equal(ViewComponentDiagnosticFactory.ViewComponent_AsyncMethod_ShouldReturnTask.Id, diagnostic.Id); + Assert.Equal(RazorExtensionsDiagnosticFactory.ViewComponent_AsyncMethod_ShouldReturnTask.Id, diagnostic.Id); } [Fact] @@ -225,7 +225,7 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions // Assert var diagnostic = Assert.Single(descriptor.GetAllDiagnostics()); - Assert.Equal(ViewComponentDiagnosticFactory.ViewComponent_AsyncMethod_ShouldReturnTask.Id, diagnostic.Id); + Assert.Equal(RazorExtensionsDiagnosticFactory.ViewComponent_AsyncMethod_ShouldReturnTask.Id, diagnostic.Id); } [Fact] @@ -242,7 +242,7 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions // Assert var diagnostic = Assert.Single(descriptor.GetAllDiagnostics()); - Assert.Equal(ViewComponentDiagnosticFactory.ViewComponent_SyncMethod_ShouldReturnValue.Id, diagnostic.Id); + Assert.Equal(RazorExtensionsDiagnosticFactory.ViewComponent_SyncMethod_ShouldReturnValue.Id, diagnostic.Id); } [Fact] @@ -259,7 +259,7 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions // Assert var diagnostic = Assert.Single(descriptor.GetAllDiagnostics()); - Assert.Equal(ViewComponentDiagnosticFactory.ViewComponent_SyncMethod_CannotReturnTask.Id, diagnostic.Id); + Assert.Equal(RazorExtensionsDiagnosticFactory.ViewComponent_SyncMethod_CannotReturnTask.Id, diagnostic.Id); } [Fact] @@ -276,7 +276,7 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions // Assert var diagnostic = Assert.Single(descriptor.GetAllDiagnostics()); - Assert.Equal(ViewComponentDiagnosticFactory.ViewComponent_SyncMethod_CannotReturnTask.Id, diagnostic.Id); + Assert.Equal(RazorExtensionsDiagnosticFactory.ViewComponent_SyncMethod_CannotReturnTask.Id, diagnostic.Id); } }