Add file scoped extensible directives. #1457

NTaylorMullen · 2017-06-22T00:19:52Z

What's in the PR

Introduced a new DirectiveUsage enum that enables directive authors to specify if they want their directives be have a specific usage. The default (what exists today) is DirectiveUsage.Unrestricted. The new one that was introduced in this PR (file scoped) is labeled as SinglePreContent. It's a semi-verbose name but I think it's important that when a user sees the enum they have a clear indication as to what it entails; this of course is further elaborated in doc comments.

For a SinglePreContent directive there is the following restrictions:

Must exist prior to any statements or expressions
Must exist prior to any HTML (including TagHelpers)
Must not have any duplicates

With these restrictions in mind these are the high level pieces that this enables

Directives sitting next to other directives
Directives sitting indiscriminately next to any number of whitespace
Directives being decorated with comments

Inheriting Directives

This PR does not contain the code to automatically inherit directives that are labeled as SinglePreContent. I thought about making this automatic but felt that it could go in a follow up if deemed important. The primary reason for it not being in this PR is due to it requiring a change in design on how we inherit directives. Today we append imported IR to the bottom of our IR document; this isn't sufficient if you have a default importing story. If we start specifying how directives are inherited we then need to start marking DirectiveIntermediateNodes (in a way that stops overridden directives from flowing into extensibility) as inherited or not inherited and enable ways on how to change that.

Added DirectiveUsage to enable extensible directive authors to indicate how their directives should be used. Currently support Unrestricted (how section directives have always worked and a single pre-content based directive.
Added directive parsing tests.
Removed no longer used BlockKindInternal items.
Update built-in directives to use DirectiveUsage.
Updated integration code gen and IR bits to reflect new directive usage.
Updated existing unit tests that happened to test directives in code blocks to now test what happens when they exist at the document level. Being inside of a code block is now invalid and we have separate tests for that scenario.

#1376

/cc @rynowak @ajaybhargavb

NTaylorMullen · 2017-06-22T00:20:53Z

src/Microsoft.AspNetCore.Razor.Language/Legacy/CSharpCodeParser.cs

+                    return;
+                }
+
+                var root = Context.Builder.ActiveBlocks.Last();


@ajaybhargavb this is the magic that determines if a directive exists prior to code, html etc.

NTaylorMullen · 2017-06-22T00:22:09Z

test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/ModelDirectiveTest.cs

@@ -14,7 +14,7 @@ namespace Microsoft.AspNetCore.Mvc.Razor.Extensions
    public class ModelDirectiveTest
    {
        [Fact]
-        public void ModelDirective_GetModelType_GetsTypeFromLastWellFormedDirective()
+        public void ModelDirective_GetModelType_GetsTypeFromFirstWellFormedDirective()


This means that @model existing multiple times in a single document results in the first valid @model being used. If you have a valid model in an import file that carries over and properly overrides proper @model directives due to how our ModelDirective code passes over the IR.

I don't understand what you're saying

NTaylorMullen · 2017-06-22T00:22:25Z

...estFiles/IntegrationTests/CodeGenerationIntegrationTest/MultipleModels_DesignTime.codegen.cs

@@ -39,6 +39,6 @@ public class TestFiles_IntegrationTests_CodeGenerationIntegrationTest_MultipleMo
        [global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
        public global::Microsoft.AspNetCore.Mvc.Rendering.IJsonHelper Json { get; private set; }
        [global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
-        public global::Microsoft.AspNetCore.Mvc.Rendering.IHtmlHelper<System.Collections.IEnumerable> Html { get; private set; }
+        public global::Microsoft.AspNetCore.Mvc.Rendering.IHtmlHelper<ThisShouldBeGenerated> Html { get; private set; }


Should have been this way to start with.

NTaylorMullen · 2017-06-22T00:23:31Z

test/Microsoft.AspNetCore.Razor.Language.Test/Legacy/CSharpDirectivesTest.cs

@@ -1062,57 +1351,53 @@ public void AddTagHelperDirective_EndQuoteRequiresDoubleQuotesAroundValue()
        }

        [Fact]
-        public void ParseBlock_InheritsDirective()


No longer valid. Have other tests.

rynowak · 2017-06-22T01:30:16Z

I like the naming File Scoped rather than pre-content. I call it file scoped, because the unit of operation is the file, and it is only allowed at the top level of scoping.

I think we have the following categories:

File Scoped - Singly Occurring
File Scoped - Multiple Occurring
Block Scoped

I defined File Scoped in the same way that you define PreContent as far as lexically how they are in Razor code. I think that part is perfect in your description.

Additionally they should have the following semantics:

File Scoped - Single

Imported directives can be overridden - from the furthest to the closest. An overridden directive does not appear in the IR. It is invisible to user code.

If you don't want this, don't do this. File Scoped - Multiple will allow you to see multiple uses of the directive.

Using a File Scoped - Single directive multiple times in any single file is an error.

Using a File Scoped - Single directive outside of the 'top' of the file is an error.

Does not support blocks.

Examples: model, inherits, page

MultipleFileScoped

Imported directives are all added to the IR document - from the furthest to the closest.

Using a File Scoped - Multiple directive outside of the 'top' of the file is an error.

Does not support blocks.

Examples: inject, using

Block

Imported directives are all added to the IR document - from the furthest to the closest.

Can appear anywhere in the document.

Examples: section

rynowak · 2017-06-22T01:30:53Z

For what I'm calling a Block directive, I'm not sure if we should make it possible to make a directive importable/not-importable.

rynowak · 2017-06-22T01:31:22Z

My guess btw is that the code for this is mostly right, I feel like my concept of this is about 10% off from yours

rynowak · 2017-06-22T01:32:32Z

Also - notice how 'Block' is pretty much how directives behave today. The File Scoped variations just remove the need to extensiblity authors to do error checking.

rynowak · 2017-06-22T01:34:44Z

src/Microsoft.AspNetCore.Mvc.Razor.Extensions/ModelDirective.cs

+            builder =>
+            {
+                builder.AddTypeToken();
+                builder.Usage = DirectiveUsage.SinglePreContent;


Should usage just be a required argument to CreateDirective? I don't have a major gripe with this, I just want to understand how you see the tradeoffs.

Thought about that approach but didn't feel like it deserved its own argument since having an unrestricted directive was consistent with what we've always had in the past. Aka, users previous expectations of directives is the default experience.

OK cool, I think that's fine.

rynowak · 2017-06-22T01:35:28Z

src/Microsoft.AspNetCore.Razor.Language/IDirectiveDescriptorBuilder.cs

@@ -31,6 +31,11 @@ public interface IDirectiveDescriptorBuilder
        DirectiveKind Kind { get; }

        /// <summary>
+        /// Gets the directive usage. The usage determines how many, and where directives can exist per document.


rynowak · 2017-06-22T01:36:52Z

src/Microsoft.AspNetCore.Razor.Language/Properties/Resources.Designer.cs

@@ -514,6 +514,34 @@ internal static string IntermediateNodeReference_CollectionIsReadOnly
        internal static string FormatIntermediateNodeReference_CollectionIsReadOnly(object p0)
            => string.Format(CultureInfo.CurrentCulture, GetString("IntermediateNodeReference_CollectionIsReadOnly"), p0);

+        /// <summary>
+        /// The '{0}' directive may only occurr once per document.


rynowak · 2017-06-22T01:39:00Z

test/Microsoft.AspNetCore.Mvc.Razor.Extensions.Test/ModelDirectiveTest.cs

@@ -31,7 +31,7 @@ @model Type2
            var result = ModelDirective.GetModelType(irDocument);

            // Assert
-            Assert.Equal("Type2", result);


I think this is fine.

rynowak · 2017-06-22T01:39:12Z

...tegrationTests/CodeGenerationIntegrationTest/IncompleteDirectives_DesignTime.diagnostics.txt

 TestFiles/IntegrationTests/CodeGenerationIntegrationTest/IncompleteDirectives.cshtml(14,11): Error RZ9999: The 'namespace' directive expects a namespace name.
+TestFiles/IntegrationTests/CodeGenerationIntegrationTest/IncompleteDirectives.cshtml(15,1): Error RZ9999: The 'namespace' directive may only occurr once per document.


occurrrrrrr

rynowak

This needs to have an effect on how imports work. let's discuss tomorrow if you don't like my ideas 😆

NTaylorMullen · 2017-06-22T18:26:13Z

Running an MVC build with these bits to ensure I didn't break anything and then will get this in (assuming all is well). I'll then do a follow up PR to implement _ViewImports support by default for FileScopedSinglyOccurring directive usages.

- Added `DirectiveUsage` to enable extensible directive authors to indicate how their directives should be used. Currently support `Unrestricted` (how section directives have always worked) and a file scoped singly occurring directive. - Added directive parsing tests. - Removed no longer used `BlockKindInternal` items. #1376

- Updated integration code gen and IR bits to reflect new directive usage. - Updated existing unit tests that happened to test directives in code blocks to now test what happens when they exist at the document level. Being inside of a code block is now invalid and we have separate tests for that scenario. #1376

NTaylorMullen requested review from rynowak and ajaybhargavb June 22, 2017 00:19

dnfclas added the cla-already-signed label Jun 22, 2017

NTaylorMullen commented Jun 22, 2017

View reviewed changes

rynowak reviewed Jun 22, 2017

View reviewed changes

rynowak suggested changes Jun 22, 2017

View reviewed changes

NTaylorMullen force-pushed the nimullen/1376 branch from 74df4a7 to a6117b6 Compare June 22, 2017 18:24

NTaylorMullen added 2 commits June 22, 2017 11:29

NTaylorMullen force-pushed the nimullen/1376 branch from a6117b6 to 1dd4feb Compare June 22, 2017 18:30

NTaylorMullen merged commit 7a04e35 into dev Jun 22, 2017

NTaylorMullen deleted the nimullen/1376 branch June 22, 2017 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add file scoped extensible directives. #1457

Add file scoped extensible directives. #1457

NTaylorMullen commented Jun 22, 2017

NTaylorMullen Jun 22, 2017

NTaylorMullen Jun 22, 2017

rynowak Jun 22, 2017

NTaylorMullen Jun 22, 2017

NTaylorMullen Jun 22, 2017

rynowak commented Jun 22, 2017 •

edited

Loading

rynowak commented Jun 22, 2017

rynowak commented Jun 22, 2017

rynowak commented Jun 22, 2017

rynowak Jun 22, 2017 •

edited

Loading

NTaylorMullen Jun 22, 2017

rynowak Jun 22, 2017

rynowak Jun 22, 2017

rynowak Jun 22, 2017

rynowak Jun 22, 2017

rynowak Jun 22, 2017

rynowak left a comment

NTaylorMullen commented Jun 22, 2017

		TestFiles/IntegrationTests/CodeGenerationIntegrationTest/IncompleteDirectives.cshtml(14,11): Error RZ9999: The 'namespace' directive expects a namespace name.
		TestFiles/IntegrationTests/CodeGenerationIntegrationTest/IncompleteDirectives.cshtml(15,1): Error RZ9999: The 'namespace' directive may only occurr once per document.

Add file scoped extensible directives. #1457

Add file scoped extensible directives. #1457

Conversation

NTaylorMullen commented Jun 22, 2017

What's in the PR

Inheriting Directives

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented Jun 22, 2017 • edited Loading

rynowak commented Jun 22, 2017

rynowak commented Jun 22, 2017

rynowak commented Jun 22, 2017

rynowak Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak left a comment

Choose a reason for hiding this comment

NTaylorMullen commented Jun 22, 2017

rynowak commented Jun 22, 2017 •

edited

Loading

rynowak Jun 22, 2017 •

edited

Loading