Add extensible directive abstractions #869

NTaylorMullen · 2016-11-23T00:50:13Z

Based generic directive implementation off of descriptors.
Added parsing logic to consume descriptors and parse content that's expected.
Added parsing errors to automagically detect unexpected directive pieces.
Updated visitor implementations to understand the directive bits.
Added a builder abstraction to easily create descriptors. Had to maintain the ability to manually construct a descriptor to enable convenient serialization/deserialization.

Will add tests once design looks good.

NTaylorMullen · 2016-11-23T00:51:13Z

src/Microsoft.AspNetCore.Razor.Evolution/DefaultRazorIRLoweringPhase.cs

+                var directiveNode = (DirectiveIRNode)Builder.Pop();
+
+                var tokens = directiveNode.Children.OfType<DirectiveTokenIRNode>().ToList();
+                var exceptTokens = directiveNode.Children.Except(tokens).ToList();


This is pretty crappy but for the IR passes to come that will light up directives it'll be important to have easy access to the Tokens.

I get why you're doing this, but wouldn't it be better to just leave the Children as is and make Tokens do something like:

public IEnumerable<DirectiveTokenIRNode> Tokens => Children.OfType<DirectiveTokenIRNode>();

Hmm, sure we can do that 😄

NTaylorMullen · 2016-11-23T00:52:26Z

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

@@ -39,11 +39,16 @@ internal class CSharpCodeParser : TokenizerBackedParser<CSharpTokenizer, CSharpS
        private Dictionary<CSharpKeyword, Action<bool>> _keywordParsers = new Dictionary<CSharpKeyword, Action<bool>>();

        public CSharpCodeParser(ParserContext context)
+            : this (directiveDescriptors: Enumerable.Empty<DirectiveDescriptor>(), context: context)


Should talk about how we want to replace existing directives with the directive descriptors. Aka:

Added in the CSharpCodeParser as "additional" directives?

Added in the person who constructs CSharpCodeParser in a manner that enables users to remove default directives from the system.

I don't think there's any value in making the "built in" directives unparsable by removing them. Users have the ability to make "built in" directives invalid in the IR if they don't want them to be supported.

NTaylorMullen · 2016-11-23T00:52:51Z

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

+            {
+                MapDirectives(() => HandleDirective(directiveDescriptor), directiveDescriptor.Name);
+            }
+
            MapDirectives(TagHelperPrefixDirective, SyntaxConstants.CSharp.TagHelperPrefixKeyword);


These will be able to go away once they're implemented ontop of the directive descriptors. Issue..

NTaylorMullen · 2016-11-23T00:55:04Z

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

+
+        IDirectiveDescriptorBuilder AddString();
+
+        IDirectiveDescriptorBuilder AddLiteral(string literal, bool optional);


Don't love the "optional" part of this but this comes into play when doing @addTagHelper "..." where the quotes are optional. Also works for ending optional semicolons which exist on many of our directives.

If you can think of something better I'm all ears 😄

My two ideas

Semicolons are always optional in directives (you don't have a choice)

Don't build @addTagHelper on top of extensible directives unless we want the "optional" part to become popular

rynowak

The design of this looks right on the whole! Bring on the tests.

rynowak · 2016-11-23T16:27:13Z

src/Microsoft.AspNetCore.Razor.Evolution/DefaultRazorIRLoweringPhase.cs

+                var directiveNode = (DirectiveIRNode)Builder.Pop();
+
+                var tokens = directiveNode.Children.OfType<DirectiveTokenIRNode>().ToList();
+                var exceptTokens = directiveNode.Children.Except(tokens).ToList();


I get why you're doing this, but wouldn't it be better to just leave the Children as is and make Tokens do something like:

public IEnumerable<DirectiveTokenIRNode> Tokens => Children.OfType<DirectiveTokenIRNode>();

rynowak · 2016-11-23T16:27:50Z

src/Microsoft.AspNetCore.Razor.Evolution/DirectiveDescriptor.cs

+
+namespace Microsoft.AspNetCore.Razor.Evolution
+{
+    public class DirectiveDescriptor


Add a private constructor

That'd break deserialization

rynowak · 2016-11-23T16:28:37Z

src/Microsoft.AspNetCore.Razor.Evolution/DirectiveDescriptor.cs

+
+        public DirectiveDescriptorType Type { get; set; }
+
+        public IList<DirectiveTokenDescriptor> Tokens { get; set; }


Wait, what? Is this an implementation of the builder or a factory of the builder?

Then how about putting these factory methods on another type - static class DirectiveDescriptorBuilder

rynowak · 2016-11-23T16:29:21Z

src/Microsoft.AspNetCore.Razor.Evolution/DirectiveDescriptorType.cs

+
+namespace Microsoft.AspNetCore.Razor.Evolution
+{
+    public enum DirectiveDescriptorType


IMO we should suffix enums like this with Kind instead of Type. It's easy to overload the meaning of type.

rynowak · 2016-11-23T16:30:56Z

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

+
+        IDirectiveDescriptorBuilder AddString();
+
+        IDirectiveDescriptorBuilder AddLiteral(string literal, bool optional);


My two ideas

Semicolons are always optional in directives (you don't have a choice)

Don't build @addTagHelper on top of extensible directives unless we want the "optional" part to become popular

rynowak · 2016-11-23T16:32:33Z

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

@@ -39,11 +39,16 @@ internal class CSharpCodeParser : TokenizerBackedParser<CSharpTokenizer, CSharpS
        private Dictionary<CSharpKeyword, Action<bool>> _keywordParsers = new Dictionary<CSharpKeyword, Action<bool>>();

        public CSharpCodeParser(ParserContext context)
+            : this (directiveDescriptors: Enumerable.Empty<DirectiveDescriptor>(), context: context)


I don't think there's any value in making the "built in" directives unparsable by removing them. Users have the ability to make "built in" directives invalid in the IR if they don't want them to be supported.

NTaylorMullen · 2016-11-24T01:16:53Z

🆙 📅 added tests

rynowak

rynowak · 2016-11-24T01:32:32Z

src/Microsoft.AspNetCore.Razor.Evolution/DirectiveTokenDescriptorComparer.cs

+            }
+
+            return descriptorX != null &&
+                string.Equals(descriptorX.Value, descriptorY.Value, StringComparison.Ordinal) &&


This can nullref if x != null && y == null

Nope. The if above prevents that 😄

if (descriptorX == descriptorY)

The previous if is a short-circuit for the case where descriptorX and descriptorY are references to the same instance or both null. Need something like the following before this expression:

if (descriptorX == null || descriptorY == null) { // Not reachable if both descriptors are null. return false; }

If descriptorX != null then descriptorY != null because the == statement above would have short-circuited.

What you're suggesting is that we return false instead of true if both the descriptors being compared are null. Probably smart. We should do this for our other comparers because they fall into the same trap.

Huh?

If descriptorX != descriptorY and descriptorX != null, descriptorY could be anything except the same reference as descriptoryX -- including null. That is, the code currently excludes only a single non-null value for descriptorY. The code you checked in can easily null ref.

Second, two null references should compare equal. I was suggesting an addition:

if (descriptorX == descriptorY) { return true; } if (descriptorX == null || descriptorY == null) { return false; } return (string.Equals(descriptorX.Name, descriptorY.Name, StringComparison.Ordinal) && descriptorX.Kind == descriptorY.Kind && Enumerable.SequenceEqual( ... );

I had a look at the other comparers in Razor. Primarily the test ones are null-safe; they often Assert.NotNull() for both parameters.

What he said

And by that I mean, that I agree with my countryman 🇨🇦 🏒 🍁 🇨🇦

I be dumb 👍

- Based generic directive implementation off of descriptors. - Added parsing logic to consume descriptors and parse content that's expected. - Added parsing errors to automagically detect unexpected directive pieces. - Updated visitor implementations to understand the directive bits. - Added a builder abstraction to easily create descriptors. Had to maintain the ability to manually construct a descriptor to enable convenient serialization/deserialization. - Added tests/comparers to verify correctness of parsing. #853

NTaylorMullen assigned rynowak Nov 23, 2016

dnfclas added the cla-already-signed label Nov 23, 2016

NTaylorMullen commented Nov 23, 2016

View reviewed changes

NTaylorMullen mentioned this pull request Nov 23, 2016

Transition existing Razor directives to utilize DirectiveDescriptors. #870

Closed

rynowak reviewed Nov 23, 2016

View reviewed changes

NTaylorMullen force-pushed the nimullen/directives.853 branch from 30388a5 to b513709 Compare November 23, 2016 18:18

NTaylorMullen changed the title ~~[Design] Add extensible directive abstractions~~ Add extensible directive abstractions Nov 24, 2016

NTaylorMullen force-pushed the nimullen/directives.853 branch from 749866d to cc0ae6a Compare November 24, 2016 01:18

rynowak approved these changes Nov 24, 2016

View reviewed changes

NTaylorMullen force-pushed the nimullen/directives.853 branch from cc0ae6a to 518378f Compare November 24, 2016 03:56

NTaylorMullen merged commit 518378f into dev Nov 24, 2016

NTaylorMullen deleted the nimullen/directives.853 branch November 24, 2016 03:58

rynowak mentioned this pull request Nov 28, 2016

[WIP] ASP.NET Core Razor engine - do not merge NancyFx/Nancy#2573

Open

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add extensible directive abstractions #869

Add extensible directive abstractions #869

NTaylorMullen commented Nov 23, 2016

NTaylorMullen Nov 23, 2016

rynowak Nov 23, 2016

NTaylorMullen Nov 23, 2016

NTaylorMullen Nov 23, 2016

rynowak Nov 23, 2016

NTaylorMullen Nov 23, 2016 •

edited

Loading

NTaylorMullen Nov 23, 2016

rynowak Nov 23, 2016

rynowak left a comment

rynowak Nov 23, 2016

rynowak Nov 23, 2016

NTaylorMullen Nov 23, 2016

rynowak Nov 23, 2016

rynowak Nov 23, 2016

rynowak Nov 23, 2016

rynowak Nov 23, 2016

rynowak Nov 23, 2016

NTaylorMullen commented Nov 24, 2016

rynowak left a comment

rynowak Nov 24, 2016

NTaylorMullen Nov 24, 2016

dougbu Nov 24, 2016

NTaylorMullen Nov 28, 2016

dougbu Nov 28, 2016

rynowak Nov 28, 2016

rynowak Nov 28, 2016

NTaylorMullen Dec 5, 2016


		IDirectiveDescriptorBuilder AddString();

		IDirectiveDescriptorBuilder AddLiteral(string literal, bool optional);


		public DirectiveDescriptorType Type { get; set; }

		public IList<DirectiveTokenDescriptor> Tokens { get; set; }

Add extensible directive abstractions #869

Add extensible directive abstractions #869

Conversation

NTaylorMullen commented Nov 23, 2016

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

NTaylorMullen Nov 23, 2016 • edited Loading

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

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

Choose a reason for hiding this comment

NTaylorMullen commented Nov 24, 2016

rynowak left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTaylorMullen Nov 23, 2016 •

edited

Loading