Skip to content

Provide a tooling gesture that suggests values for required component parameters are not specified. #33384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 11, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Components/Components/src/DynamicComponent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ public DynamicComponent()
/// </summary>
[Parameter]
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
[EditorRequired]
public Type Type { get; set; } = default!;

/// <summary>
Expand Down
19 changes: 19 additions & 0 deletions src/Components/Components/src/EditorRequiredAttribute.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// 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;

namespace Microsoft.AspNetCore.Components
{
/// <summary>
/// Specifies that the component parameter is required to be provided by the user when authoring it in the editor.
/// <para>
/// If a value for this parameter is not provided, editors or build tools may provide warnings indicating the user to
/// specify a value. This attribute is only valid on properties marked with <see cref="ParameterAttribute"/>.
/// </para>
/// </summary>
[AttributeUsage(AttributeTargets.Property, AllowMultiple = false)]
public sealed class EditorRequiredAttribute : Attribute
{
}
}
2 changes: 2 additions & 0 deletions src/Components/Components/src/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ Microsoft.AspNetCore.Components.ComponentApplicationState.PersistAsJson<TValue>(
Microsoft.AspNetCore.Components.ComponentApplicationState.PersistState(string! key, byte[]! value) -> void
Microsoft.AspNetCore.Components.ComponentApplicationState.TryTakeAsJson<TValue>(string! key, out TValue? instance) -> bool
Microsoft.AspNetCore.Components.ComponentApplicationState.TryTakePersistedState(string! key, out byte[]? value) -> bool
Microsoft.AspNetCore.Components.EditorRequiredAttribute
Microsoft.AspNetCore.Components.EditorRequiredAttribute.EditorRequiredAttribute() -> void
Microsoft.AspNetCore.Components.ErrorBoundaryBase
Microsoft.AspNetCore.Components.ErrorBoundaryBase.ChildContent.get -> Microsoft.AspNetCore.Components.RenderFragment?
Microsoft.AspNetCore.Components.ErrorBoundaryBase.ChildContent.set -> void
Expand Down
1 change: 1 addition & 0 deletions src/Components/Components/src/RouteView.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ public class RouteView : IComponent
/// displayed and the parameter values that will be supplied to the page.
/// </summary>
[Parameter]
[EditorRequired]
public RouteData RouteData { get; set; }

/// <summary>
Expand Down
12 changes: 9 additions & 3 deletions src/Components/Components/src/Routing/Router.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ static readonly IReadOnlyDictionary<string, object> _emptyParametersDictionary
/// <summary>
/// Gets or sets the assembly that should be searched for components matching the URI.
/// </summary>
[Parameter] public Assembly AppAssembly { get; set; }
[Parameter]
[EditorRequired]
public Assembly AppAssembly { get; set; }

/// <summary>
/// Gets or sets a collection of additional assemblies that should be searched for components
Expand All @@ -58,12 +60,16 @@ static readonly IReadOnlyDictionary<string, object> _emptyParametersDictionary
/// <summary>
/// Gets or sets the content to display when no match is found for the requested route.
/// </summary>
[Parameter] public RenderFragment NotFound { get; set; }
[Parameter]
[EditorRequired]
public RenderFragment NotFound { get; set; }

/// <summary>
/// Gets or sets the content to display when a match is found for the requested route.
/// </summary>
[Parameter] public RenderFragment<RouteData> Found { get; set; }
[Parameter]
[EditorRequired]
public RenderFragment<RouteData> Found { get; set; }

/// <summary>
/// Get or sets the content to display when asynchronous navigation is in progress.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ protected BoundAttributeDescriptor(string kind)

public bool IsBooleanProperty { get; protected set; }

internal bool IsEditorRequired { get; set; }

public string Name { get; protected set; }

public string IndexerNamePrefix { get; protected set; }
Expand Down Expand Up @@ -81,4 +83,4 @@ public override int GetHashCode()
return BoundAttributeDescriptorComparer.Default.GetHashCode(this);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// 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;
Expand Down Expand Up @@ -28,6 +28,8 @@ public abstract class BoundAttributeDescriptorBuilder

public abstract RazorDiagnosticCollection Diagnostics { get; }

internal bool IsEditorRequired { get; set; }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conflicted with this concept being on the BoundAttributeDescriptor vs. the RequiredAttributeDescriptor. I know the RequiredAttributeDescriptor has a slightly different implementation detail (aka effects if the TagHelper binds) but I still wonder if we're maybe crossing concerns. Did you happen to think more about the two options?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SteveSandersonMS brought this up yesterday.

Right now, a tag helper attribute that is required but is missing causes the binding to fail. As a result, you lose colorization, don't get completion / documentation / tooltips, and you get the warning that says this tag looks like a component but did not bind.

If there's a way to improve this experience (keep colorization and completion and provide a better warning), we could use the current implementation.


public virtual IReadOnlyList<BoundAttributeParameterDescriptorBuilder> BoundAttributeParameters { get; }

public virtual void BindAttributeParameter(Action<BoundAttributeParameterDescriptorBuilder> configure)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ protected override void ExecuteCore(RazorCodeDocument codeDocument, DocumentInte
}
}

private ComponentIntermediateNode RewriteAsComponent(TagHelperIntermediateNode node, TagHelperDescriptor tagHelper)
private static ComponentIntermediateNode RewriteAsComponent(TagHelperIntermediateNode node, TagHelperDescriptor tagHelper)
{
var component = new ComponentIntermediateNode()
{
Expand All @@ -89,13 +89,54 @@ private ComponentIntermediateNode RewriteAsComponent(TagHelperIntermediateNode n
// because we see the nodes in the wrong order.
foreach (var childContent in component.ChildContents)
{
childContent.ParameterName = childContent.ParameterName ?? component.ChildContentParameterName ?? ComponentMetadata.ChildContent.DefaultParameterName;
childContent.ParameterName ??= component.ChildContentParameterName ?? ComponentMetadata.ChildContent.DefaultParameterName;
}

ValidateRequiredAttributes(node, tagHelper, component);

return component;
}

private MarkupElementIntermediateNode RewriteAsElement(TagHelperIntermediateNode node)
private static void ValidateRequiredAttributes(TagHelperIntermediateNode node, TagHelperDescriptor tagHelper, ComponentIntermediateNode intermediateNode)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I'm not sure that this (the IR pass) is the right location to log diagnostics for this. Would it be possibel to log these diagnostics on the TagHelperElement syntax nodes? It'd enable us to have a better association with what element was the "problem" element and would enable tooling to react appropriately (aka generate the required attributes)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any pointers on where that would be? This seemed like the earliest place where things like child components were resolved.

{
if (intermediateNode.Children.Any(c => c is TagHelperDirectiveAttributeIntermediateNode node && (node.TagHelper?.IsSplatTagHelper() ?? false)))
{
// If there are any splat attributes, assume the user may have provided all values.
// This pass runs earlier than ComponentSplatLoweringPass, so we cannot rely on the presence of SplatIntermediateNode to make this check.
return;
}

foreach (var requiredAttribute in tagHelper.EditorRequiredAttributes)
{
if (!IsPresentAsAttribute(requiredAttribute.Name, intermediateNode))
{
intermediateNode.Diagnostics.Add(
RazorDiagnosticFactory.CreateComponent_EditorRequiredParameterNotSpecified(
node.Source ?? SourceSpan.Undefined,
intermediateNode.TagName,
requiredAttribute.Name));
}
}

static bool IsPresentAsAttribute(string attributeName, ComponentIntermediateNode intermediateNode)
{
foreach (var child in intermediateNode.Children)
{
if (child is ComponentAttributeIntermediateNode attributeNode && attributeName == attributeNode.AttributeName)
{
return true;
}
else if (child is ComponentChildContentIntermediateNode childContent && attributeName == childContent.AttributeName)
{
return true;
}
}

return false;
}
}

private static MarkupElementIntermediateNode RewriteAsElement(TagHelperIntermediateNode node)
{
var result = new MarkupElementIntermediateNode()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,10 @@ public BoundAttributeDescriptor Build()
CaseSensitive,
parameters,
new Dictionary<string, string>(Metadata),
diagnostics.ToArray());
diagnostics.ToArray())
{
IsEditorRequired = IsEditorRequired,
};

return descriptor;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using Microsoft.AspNetCore.Razor.Language.Legacy;

namespace Microsoft.AspNetCore.Razor.Language
Expand Down Expand Up @@ -588,6 +589,17 @@ public static RazorDiagnostic CreateTagHelper_InconsistentTagStructure(SourceSpa
return RazorDiagnostic.Create(TagHelper_InconsistentTagStructure, location, firstDescriptor, secondDescriptor, tagName, nameof(TagMatchingRuleDescriptor.TagStructure));
}

internal static readonly RazorDiagnosticDescriptor Component_EditorRequiredParameterNotSpecified =
new RazorDiagnosticDescriptor(
$"{DiagnosticPrefix}2012",
() => Resources.Component_EditorRequiredParameterNotSpecified,
RazorDiagnosticSeverity.Warning);

public static RazorDiagnostic CreateComponent_EditorRequiredParameterNotSpecified(SourceSpan location, string tagName, string parameterName)
{
return RazorDiagnostic.Create(Component_EditorRequiredParameterNotSpecified, location, tagName, parameterName);
}

#endregion

#region TagHelper Errors
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<?xml version="1.0" encoding="utf-8"?>
<root>
<!--
Microsoft ResX Schema
Expand Down Expand Up @@ -571,4 +571,7 @@
<data name="ParseError_Unexpected_Identifier_At_Position" xml:space="preserve">
<value>'{0}' is not valid in this position. Valid options are '{1}'</value>
</data>
</root>
<data name="Component_EditorRequiredParameterNotSpecified" xml:space="preserve">
<value>Component '{0}' expects a value for the parameter '{1}', but a value may not have been provided.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ namespace Microsoft.AspNetCore.Razor.Language
public abstract class TagHelperDescriptor : IEquatable<TagHelperDescriptor>
{
private IEnumerable<RazorDiagnostic> _allDiagnostics;
private BoundAttributeDescriptor[] _editorRequiredAttributes;

protected TagHelperDescriptor(string kind)
{
Expand Down Expand Up @@ -45,6 +46,14 @@ protected TagHelperDescriptor(string kind)
internal bool? IsComponentFullyQualifiedNameMatchCache { get; set; }
internal bool? IsChildContentTagHelperCache { get; set; }
internal ParsedTypeInformation? ParsedTypeInfo { get; set; }
internal BoundAttributeDescriptor[] EditorRequiredAttributes
{
get
{
_editorRequiredAttributes ??= GetEditorRequiredAttributes(BoundAttributes);
return _editorRequiredAttributes;
}
}

public bool HasErrors
{
Expand Down Expand Up @@ -96,6 +105,23 @@ public override int GetHashCode()
return _hashCode.Value;
}

private static BoundAttributeDescriptor[] GetEditorRequiredAttributes(IReadOnlyList<BoundAttributeDescriptor> boundAttributeDescriptors)
{
List<BoundAttributeDescriptor> editorRequiredAttributes = null;
var count = boundAttributeDescriptors.Count;
for (var i = 0; i < count; i++)
{
var attribute = boundAttributeDescriptors[i];
if (attribute.IsEditorRequired)
{
editorRequiredAttributes ??= new();
editorRequiredAttributes.Add(attribute);
}
}

return editorRequiredAttributes?.ToArray() ?? Array.Empty<BoundAttributeDescriptor>();
}

internal readonly struct ParsedTypeInformation
{
public ParsedTypeInformation(bool success, StringSegment @namespace, StringSegment typeName)
Expand Down
Loading