Skip to content

Commit

Permalink
Clarify [EventHandler] tag helper discovery logic and avoid exception (
Browse files Browse the repository at this point in the history
…#10828)

This fixes an issue that @tmat pointed out to me over email. In a recent
change (#10720), I added a call to `Assumed.Unreachable()` when
`[EventHandler]` tag helper discovery encounters an attribute with
invalid constructor attributes. However, throwing an exception during
tag helper discovery is usually the wrong approach. Normally, if Roslyn
symbols aren't in the proper shape during tag helper discovery, Razor
will simply not produce a tag helper. (We _do_ support diagnostics for
tag helpers, but those are usually reserved for warnings and errors that
are related to a tag helper's data that would make it unusable, such as
a name containing whitespace.)

It turns out that the "unreachable" condition wasn't actually all that
unreachable and @tmat was hitting it while working on hot reload tests.
So, I've changed the code to make the success conditions clearer, i.e.,
the attribute data must match one of the two constructor calls. And, I
changed the logic to simply skip `[EventHandler]` attributes that don't
meet the success conditions.
  • Loading branch information
DustinCampbell authored Sep 3, 2024
2 parents 8bddfe9 + 277c780 commit 277663c
Show file tree
Hide file tree
Showing 2 changed files with 290 additions and 43 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Collections.Immutable;
using Microsoft.AspNetCore.Razor;
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.AspNetCore.Razor.Language.Components;
Expand Down Expand Up @@ -54,14 +55,14 @@ protected override void Collect(ISymbol symbol, ICollection<TagHelperDescriptor>
{
if (SymbolEqualityComparer.Default.Equals(attribute.AttributeClass, _eventHandlerAttribute))
{
if (!AttributeArgs.TryGet(attribute, out var values))
if (!AttributeArgs.TryGet(attribute, out var args))
{
// If this occurs, the EventHandlerAttribute type has been broken.
Assumed.Unreachable();
// If this occurs, the [EventHandler] was defined incorrectly, so we can't create a tag helper.
continue;
}

var (typeName, namespaceName) = displayNames.GetNames();
results.Add(CreateTagHelper(typeName, namespaceName, type.Name, values));
results.Add(CreateTagHelper(typeName, namespaceName, type.Name, args));
}
}
}
Expand All @@ -70,8 +71,8 @@ protected override void Collect(ISymbol symbol, ICollection<TagHelperDescriptor>
private readonly record struct AttributeArgs(
string Attribute,
INamedTypeSymbol EventArgsType,
bool EnableStopPropagation,
bool EnablePreventDefault)
bool EnableStopPropagation = false,
bool EnablePreventDefault = false)
{
public static bool TryGet(AttributeData attribute, out AttributeArgs args)
{
Expand All @@ -80,34 +81,49 @@ public static bool TryGet(AttributeData attribute, out AttributeArgs args)
// - EventHandlerAttribute(string attributeName, Type eventArgsType);
// - EventHandlerAttribute(string attributeName, Type eventArgsType, bool enableStopPropagation, bool enablePreventDefault);

args = default;

var arguments = attribute.ConstructorArguments;

if (arguments.Length is not (2 or 4))
{
return false;
}
return TryGetFromTwoArguments(arguments, out args) ||
TryGetFromFourArguments(arguments, out args);

if (arguments[0] is not { Value: string attributeName } ||
arguments[1] is not { Value: INamedTypeSymbol eventArgsType })
static bool TryGetFromTwoArguments(ImmutableArray<TypedConstant> arguments, out AttributeArgs args)
{
// Ctor 1: EventHandlerAttribute(string attributeName, Type eventArgsType);

if (arguments is [
{ Value: string attributeName },
{ Value: INamedTypeSymbol eventArgsType }])
{
args = new(attributeName, eventArgsType);
return true;
}

args = default;
return false;
}

// TODO: The enablePreventDefault and enableStopPropagation arguments are incorrectly swapped!
// However, they have been that way since the 4-argument constructor variant was introduced
// in https://github.com/dotnet/razor/commit/7635bba6ef2d3e6798d0846ceb96da6d5908e1b0.
// Fixing this is tracked be https://github.com/dotnet/razor/issues/10497

if (arguments is not [.., { Value: bool enablePreventDefault }, { Value: bool enableStopPropagation }])
static bool TryGetFromFourArguments(ImmutableArray<TypedConstant> arguments, out AttributeArgs args)
{
enableStopPropagation = false;
enablePreventDefault = false;
}
// Ctor 2: EventHandlerAttribute(string attributeName, Type eventArgsType, bool enableStopPropagation, bool enablePreventDefault);

// TODO: The enablePreventDefault and enableStopPropagation arguments are incorrectly swapped!
// However, they have been that way since the 4-argument constructor variant was introduced
// in https://github.com/dotnet/razor/commit/7635bba6ef2d3e6798d0846ceb96da6d5908e1b0.
// Fixing this is tracked be https://github.com/dotnet/razor/issues/10497

if (arguments is [
{ Value: string attributeName },
{ Value: INamedTypeSymbol eventArgsType },
{ Value: bool enablePreventDefault },
{ Value: bool enableStopPropagation }])
{
args = new(attributeName, eventArgsType, enableStopPropagation, enablePreventDefault);
return true;
}

args = new(attributeName, eventArgsType, enableStopPropagation, enablePreventDefault);
return true;
args = default;
return false;
}
}
}

Expand Down
Loading

0 comments on commit 277663c

Please sign in to comment.