Reacted to descriptor changes in product code. #1054

NTaylorMullen · 2017-03-07T19:20:10Z

@ajaybhargavb and I are getting PRs out now for the descriptor changes so they can be looked in consumable pieces over as we finish the test reaction work.

Added a new RazorDiagnosticFactory abstraction to handle RazorDiagnostics and their corresponding errors/ids etc. This new API should allow for easy addition of new RazorDiagnostic errors.
Updated the DefaultTagHelperDescriptorFactory to construct TagHelperDescriptors using the new builder APIs and in the new descriptor format (1 descriptor per type).
Updated ViewComponentTagHelperDescriptorFactory to construct TagHelperDescriptors with the builder API.
With both factory implementations code was duplicated because the ViewComponent work will be moving outside of Razor once we have the proper hooks.
Updated TagHelper binding bits to capture a binding result in order to query which rules appy to a given tag name.
Still need to handle deserialization of TagHelperDescriptor types within the Razor extension.

#973

rynowak · 2017-03-07T20:46:20Z

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

            }

-            var visitor = new MainSourceVisitor(document, builder, namespaces)
+            var tagHelperPrefix = codeDocument.GetTagHelperPrefix();


rynowak · 2017-03-07T20:48:29Z

src/Microsoft.AspNetCore.Razor.Evolution/Intermediate/SetPreallocatedTagHelperPropertyIRNode.cs

        public string PropertyName { get; set; }

-        public TagHelperAttributeDescriptor Descriptor { get; set; }
+        public BoundAttributeDescriptor Descriptor { get; set; }


It would be really nice if this had a backreference to the taghelperdescriptor as well. This is pretty cumbersome when working with the IR.

The IR node I mean

Spoke offline. Going to add a reference to TagHelperBinding aka TagBindingResult

rynowak · 2017-03-07T20:49:23Z

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

+
+namespace Microsoft.AspNetCore.Razor.Evolution.Legacy
+{
+    internal sealed class TagBindingResult


TagHelperBinding?

rynowak · 2017-03-07T20:51:42Z

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

+        /// <param name="tagHelperPrefix">The tag helper prefix being used by the document.</param>
        /// <param name="descriptors">The descriptors that the <see cref="TagHelperDescriptorProvider"/> will pull from.</param>
-        public TagHelperDescriptorProvider(IEnumerable<TagHelperDescriptor> descriptors)
+        public TagHelperDescriptorProvider(string tagHelperPrefix, IEnumerable<TagHelperDescriptor> descriptors)


We'll want to clean this up as well in the future as part of the extensibility for discovery.

OK I'm avoiding commenting here after reading further. This needs a lot of simplification.

rynowak · 2017-03-07T20:53:12Z

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

            }

-            var descriptors = Enumerable.Empty<TagHelperDescriptor>();
+            TagBindingResult tagHelperBinding;


See you already knew what to call it.

rynowak · 2017-03-07T21:05:31Z

src/Microsoft.CodeAnalysis.Razor/DefaultTagHelperDescriptorFactory.cs

+                    {
+                        _index++;
+                    }
                }


It's really nice overall to see the changes to this class, this is much cleaner than before.

rynowak · 2017-03-07T21:06:16Z

src/Microsoft.CodeAnalysis.Razor/TagHelpers.cs

-                var descriptors = factory.CreateDescriptors(type, errors);
-                results.AddRange(descriptors);
+                var descriptor = factory.CreateDescriptor(type);
+                results.Add(descriptor);


What if the factory returns null?

rynowak · 2017-03-07T21:07:29Z

src/Microsoft.VisualStudio.LanguageServices.Razor/DefaultTagHelperResolver.cs

-                var symbol = compilation.GetTypeByMetadataName(tagHelper.TypeName);
+                if (tagHelper.Documentation != null)
+                {
+                    goto AddDocumentedTagHelper;


No. you haven't earned this.

rynowak · 2017-03-07T21:08:14Z

src/Microsoft.VisualStudio.LanguageServices.Razor/DefaultTagHelperResolver.cs

-                                AddXmlDocumentation(compilation, result.Descriptors);
-                                return result;
+                                var documentedTagHelpers = GetDocumentedTagHelpers(compilation, result.Descriptors);
+                                var documentedResult = new TagHelperResolutionResult(documentedTagHelpers, result.Diagnostics);


Why change this data flow? Just because of immutability?

I'd suggest just writing it in a sane, albeit inefficient way.

Get the descriptors

Create the objects

Loop over each one that's missing docs and add them

return

Alternatively you could loop over the builders in stage 3.

NVM I am a sillypants

rynowak · 2017-03-07T21:09:18Z

src/Microsoft.VisualStudio.LanguageServices.Razor/DefaultTagHelperResolver.cs

+            return documentedTagHelpers;
+        }
+
+        private ITagHelperBoundAttributeDescriptorBuilder ShallowCopy(string tagHelperTypeName, BoundAttributeDescriptor attribute)


Think about making this an internal method on the abstract class. Then each descriptor type can 'copy' itself to a builder.

Thought about doing that and then realized this was the only part of the API that'd use it. Decided against it until we need it.

Sounds good.

- Added a new `RazorDiagnosticFactory` abstraction to handle `RazorDiagnostic`s and their corresponding errors/ids etc. This new API should allow for easy addition of new `RazorDiagnostic` errors. - Updated the `DefaultTagHelperDescriptorFactory` to construct `TagHelperDescriptor`s using the new builder APIs and in the new descriptor format (1 descriptor per type). - Updated `ViewComponentTagHelperDescriptorFactory` to construct `TagHelperDescriptor`s with the builder API. - With both factory implementations code was duplicated because the ViewComponent work will be moving outside of Razor once we have the proper hooks. - Updated `TagHelper` binding bits to capture a binding result in order to query which rules appy to a given tag name. - Still need to handle deserialization of `TagHelperDescriptor` types within the Razor extension. #973

- Renamed TagBindingResult to TagHelperBinding - Added reference to TagHelperBinding in SetTagHelperPropertyIRNode - Cleanup

ajaybhargavb · 2017-03-14T00:59:10Z

🆙 📅

- Finished migrating the `Microsoft.CodeAnalysis.Razor.Test` project. Major work here was getting the various factory tests constructing `TagHelperDescriptor`s correctly in a 1 -> 1 format with types. - Migrated integration tests for confidence in our product code changes. - Deleted several tests that raised themselves as being duplicates. - Updated case sensitive comparers to understand new `TagHelperDescriptor` API. - Removed design time comparers. #973

- Cleanup Validate() in builders - Remove case sensitive comparers and some cleanup

- Fixed legacy tests - Added TagHelperDescriptorJsonConverter and added serialization tests

rynowak

rynowak · 2017-03-15T02:31:27Z

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


-                AddTagHelperCreation(tagHelperBlock.Descriptors);
-                AddTagHelperAttributes(tagHelperBlock.Attributes, tagHelperBlock.Descriptors);
+                AddTagHelperCreation(tagHelperBlock.BindingResult);


This should just be .Binding

rynowak · 2017-03-15T02:33:36Z

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

+
+namespace Microsoft.AspNetCore.Razor.Evolution.Legacy
+{
+    internal sealed class TagHelperBinding


This should be public, maybe with an internal constructor? IR Visitors will want to be able to see this

rynowak · 2017-03-15T02:33:57Z

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

+
+        public TagHelperBinding(IReadOnlyDictionary<TagHelperDescriptor, IEnumerable<TagMatchingRule>> applicableDescriptorMappings)
+        {
+            _applicableDescriptorMappings = applicableDescriptorMappings;


how about just mappings?

rynowak · 2017-03-15T03:02:36Z

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

+     * REVIEWERS: This needs to be public or IVT for tooling to consume until we flesh
+     * out a proper API to decouple tooling from Razors intrinsics. They use these to
+     * determine if their HTML elements match a descriptor or not to provide IntelliSense.
+    */


Don't leave this comment here

rynowak · 2017-03-15T03:15:22Z

src/Microsoft.CodeAnalysis.Razor/RequiredAttributeParser.cs

+namespace Microsoft.CodeAnalysis.Razor
+{
+    // Internal for testing
+    internal static class RequiredAttributeParser


This whole thing should be independent of the builder... The builder should call into this class, not the other way around.

That'd put the logic of CSS based required attributes in the builder and not in the DefaultTagHelperDescriptorFactory. That sounds a little backwards compared to how all other declarative information on TagHelper's is consumed.

Filed an issue to track this. https://github.com/aspnet/Razor/issues/1082

NTaylorMullen requested review from pranavkm and rynowak March 7, 2017 19:20

dnfclas added the cla-already-signed label Mar 7, 2017

NTaylorMullen changed the title ~~[Design] Reacted to descriptor changes in product code.~~ Reacted to descriptor changes in product code. Mar 7, 2017

rynowak suggested changes Mar 7, 2017

View reviewed changes

NTaylorMullen mentioned this pull request Mar 8, 2017

Instead of tracking @tagHelperPrefix at the code document level, find another way #1060

Closed

ajaybhargavb mentioned this pull request Mar 10, 2017

Decouple errors for TagHelpers from type name #1072

Closed

ajaybhargavb force-pushed the nimullen/thdapi.973.descriptors branch from 26f75a6 to 4306def Compare March 13, 2017 23:09

N. Taylor Mullen and others added 2 commits March 13, 2017 16:57

Addressed feedback

62fad09

- Renamed TagBindingResult to TagHelperBinding - Added reference to TagHelperBinding in SetTagHelperPropertyIRNode - Cleanup

ajaybhargavb force-pushed the nimullen/thdapi.973.product branch from 9ce4e8e to 62fad09 Compare March 14, 2017 00:58

N. Taylor Mullen and others added 7 commits March 13, 2017 19:36

Evolution tests WIP.

4de0835

Finished fixing Razor.Evolution tests.

8834b09

Addressed feedback

ff3a79e

- Cleanup Validate() in builders - Remove case sensitive comparers and some cleanup

Finish porting Razor.Evolution tests

37b7b56

- Fixed legacy tests - Added TagHelperDescriptorJsonConverter and added serialization tests

Addressed feedback

779e260

Bring back DefaultDirectiveIRPassTest from the dead

ddb770d

rynowak approved these changes Mar 15, 2017

View reviewed changes

ajaybhargavb added 2 commits March 15, 2017 20:18

Addressed final feedback

f535e15

Make things build

f453186

ajaybhargavb merged commit f453186 into nimullen/thdapi.973.descriptors Mar 16, 2017

ajaybhargavb deleted the nimullen/thdapi.973.product branch March 17, 2017 17:51

ajaybhargavb mentioned this pull request Dec 14, 2018

Refactor RequiredAttributeParser and decouple it from the builder dotnet/aspnetcore#4998

Closed

Reacted to descriptor changes in product code. #1054

Reacted to descriptor changes in product code. #1054

Uh oh!

Conversation

NTaylorMullen commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaybhargavb commented Mar 14, 2017

Uh oh!

rynowak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NTaylorMullen commented Mar 7, 2017 •

edited

Loading