Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 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
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,4 @@ MVVMTK0027 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator
MVVMTK0028 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Error | See https://aka.ms/mvvmtoolkit/error
MVVMTK0029 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Warning | See https://aka.ms/mvvmtoolkit/error
MVVMTK0030 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Warning | See https://aka.ms/mvvmtoolkit/error
MVVMTK0031 | CommunityToolkit.Mvvm.SourceGenerators.RelayCommandGenerator | Error | See https://aka.ms/mvvmtoolkit/error
Original file line number Diff line number Diff line change
Expand Up @@ -191,17 +191,17 @@ internal static class DiagnosticDescriptors
/// <summary>
/// Gets a <see cref="DiagnosticDescriptor"/> indicating when <c>RelayCommandAttribute.AllowConcurrentExecutions</c> is being set for a non-asynchronous method.
/// <para>
/// Format: <c>"The method {0}.{1} cannot be annotated with the [RelayCommand] attribute specifying a concurrency control setting, as it maps to a non-asynchronous command type"</c>.
/// Format: <c>"The method {0}.{1} cannot be annotated with the [RelayCommand] attribute specifying a concurrency control option, as it maps to a non-asynchronous command type"</c>.
/// </para>
/// </summary>
public static readonly DiagnosticDescriptor InvalidConcurrentExecutionsParameterError = new DiagnosticDescriptor(
id: "MVVMTK0012",
title: "Invalid concurrency control setting usage",
messageFormat: "The method {0}.{1} cannot be annotated with the [RelayCommand] attribute specifying a concurrency control setting, as it maps to a non-asynchronous command type",
title: "Invalid concurrency control option usage",
messageFormat: "The method {0}.{1} cannot be annotated with the [RelayCommand] attribute specifying a concurrency control option, as it maps to a non-asynchronous command type",
category: typeof(RelayCommandGenerator).FullName,
defaultSeverity: DiagnosticSeverity.Error,
isEnabledByDefault: true,
description: "Cannot apply the [RelayCommand] attribute specifying a concurrency control setting to methods mapping to non-asynchronous command types.",
description: "Cannot apply the [RelayCommand] attribute specifying a concurrency control option to methods mapping to non-asynchronous command types.",
helpLinkUri: "https://aka.ms/mvvmtoolkit");

/// <summary>
Expand Down Expand Up @@ -491,4 +491,20 @@ internal static class DiagnosticDescriptors
isEnabledByDefault: true,
description: "Annotating a field with [NotifyDataErrorInfo] is not necessary if the containing type has or inherits [NotifyDataErrorInfo] at the class-level.",
helpLinkUri: "https://aka.ms/mvvmtoolkit");

/// <summary>
/// Gets a <see cref="DiagnosticDescriptor"/> indicating when <c>RelayCommandAttribute.FlowExceptionsToTaskScheduler</c> is being set for a non-asynchronous method.
/// <para>
/// Format: <c>"The method {0}.{1} cannot be annotated with the [RelayCommand] attribute specifying an exception flow option, as it maps to a non-asynchronous command type"</c>.
/// </para>
/// </summary>
public static readonly DiagnosticDescriptor InvalidFlowExceptionsToTaskSchedulerParameterError = new DiagnosticDescriptor(
id: "MVVMTK0031",
title: "Invalid task scheduler exception flow option usage",
messageFormat: "The method {0}.{1} cannot be annotated with the [RelayCommand] attribute specifying a task scheduler exception flow option, as it maps to a non-asynchronous command type",
category: typeof(RelayCommandGenerator).FullName,
defaultSeverity: DiagnosticSeverity.Error,
isEnabledByDefault: true,
description: "Cannot apply the [RelayCommand] attribute specifying a task scheduler exception flow option to methods mapping to non-asynchronous command types.",
helpLinkUri: "https://aka.ms/mvvmtoolkit");
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ namespace CommunityToolkit.Mvvm.SourceGenerators.Input.Models;
/// <param name="CanExecuteMemberName">The member name for the can execute check, if available.</param>
/// <param name="CanExecuteExpressionType">The can execute expression type, if available.</param>
/// <param name="AllowConcurrentExecutions">Whether or not concurrent executions have been enabled.</param>
/// <param name="FlowExceptionsToTaskScheduler">Whether or not exceptions should flow to the task scheduler.</param>
/// <param name="IncludeCancelCommand">Whether or not to also generate a cancel command.</param>
internal sealed record CommandInfo(
string MethodName,
Expand All @@ -38,6 +39,7 @@ internal sealed record CommandInfo(
string? CanExecuteMemberName,
CanExecuteExpressionType? CanExecuteExpressionType,
bool AllowConcurrentExecutions,
bool FlowExceptionsToTaskScheduler,
bool IncludeCancelCommand)
{
/// <summary>
Expand All @@ -59,6 +61,7 @@ protected override void AddToHashCode(ref HashCode hashCode, CommandInfo obj)
hashCode.Add(obj.CanExecuteMemberName);
hashCode.Add(obj.CanExecuteExpressionType);
hashCode.Add(obj.AllowConcurrentExecutions);
hashCode.Add(obj.FlowExceptionsToTaskScheduler);
hashCode.Add(obj.IncludeCancelCommand);
}

Expand All @@ -77,6 +80,7 @@ protected override bool AreEqual(CommandInfo x, CommandInfo y)
x.CanExecuteMemberName == y.CanExecuteMemberName &&
x.CanExecuteExpressionType == y.CanExecuteExpressionType &&
x.AllowConcurrentExecutions == y.AllowConcurrentExecutions &&
x.FlowExceptionsToTaskScheduler == y.FlowExceptionsToTaskScheduler &&
x.IncludeCancelCommand == y.IncludeCancelCommand;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,17 @@ internal static class Execute
goto Failure;
}

// Check the switch to control exception flow
if (!TryGetFlowExceptionsToTaskSchedulerSwitch(
methodSymbol,
attributeData,
commandClassType,
builder,
out bool flowExceptionsToTaskScheduler))
{
goto Failure;
}

// Get the CanExecute expression type, if any
if (!TryGetCanExecuteExpressionType(
methodSymbol,
Expand Down Expand Up @@ -109,6 +120,7 @@ internal static class Execute
canExecuteMemberName,
canExecuteExpressionType,
allowConcurrentExecutions,
flowExceptionsToTaskScheduler,
generateCancelCommand);

Failure:
Expand Down Expand Up @@ -205,9 +217,37 @@ public static ImmutableArray<MemberDeclarationSyntax> GetSyntax(CommandInfo comm
}

// Enable concurrent executions, if requested
if (commandInfo.AllowConcurrentExecutions)
if (commandInfo.AllowConcurrentExecutions && !commandInfo.FlowExceptionsToTaskScheduler)
{
commandCreationArguments.Add(
Argument(MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
IdentifierName("global::CommunityToolkit.Mvvm.Input.AsyncRelayCommandOptions"),
IdentifierName("AllowConcurrentExecutions"))));
}
else if (commandInfo.FlowExceptionsToTaskScheduler && !commandInfo.AllowConcurrentExecutions)
{
commandCreationArguments.Add(Argument(LiteralExpression(SyntaxKind.TrueLiteralExpression)));
// Enable exception flow, if requested
commandCreationArguments.Add(
Argument(MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
IdentifierName("global::CommunityToolkit.Mvvm.Input.AsyncRelayCommandOptions"),
IdentifierName("FlowExceptionsToTaskScheduler"))));
}
else if (commandInfo.AllowConcurrentExecutions && commandInfo.FlowExceptionsToTaskScheduler)
{
// Enable both concurrency control and exception flow
commandCreationArguments.Add(
Argument(BinaryExpression(
SyntaxKind.BitwiseOrExpression,
MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
IdentifierName("global::CommunityToolkit.Mvvm.Input.AsyncRelayCommandOptions"),
IdentifierName("AllowConcurrentExecutions")),
MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
IdentifierName("global::CommunityToolkit.Mvvm.Input.AsyncRelayCommandOptions"),
IdentifierName("FlowExceptionsToTaskScheduler")))));
}

// Construct the generated property as follows (the explicit delegate cast is needed to avoid overload resolution conflicts):
Expand Down Expand Up @@ -555,6 +595,43 @@ private static bool TryGetAllowConcurrentExecutionsSwitch(
}
}

/// <summary>
/// Checks whether or not the user has requested to configure the task scheduler exception flow option.
/// </summary>
/// <param name="methodSymbol">The input <see cref="IMethodSymbol"/> instance to process.</param>
/// <param name="attributeData">The <see cref="AttributeData"/> instance the method was annotated with.</param>
/// <param name="commandClassType">The command class type name.</param>
/// <param name="diagnostics">The current collection of gathered diagnostics.</param>
/// <param name="flowExceptionsToTaskScheduler">Whether or not task scheduler exception flow have been enabled.</param>
/// <returns>Whether or not a value for <paramref name="flowExceptionsToTaskScheduler"/> could be retrieved successfully.</returns>
private static bool TryGetFlowExceptionsToTaskSchedulerSwitch(
IMethodSymbol methodSymbol,
AttributeData attributeData,
string commandClassType,
ImmutableArray<Diagnostic>.Builder diagnostics,
out bool flowExceptionsToTaskScheduler)
{
// Try to get the custom switch for task scheduler exception flow (the default is false)
if (!attributeData.TryGetNamedArgument("FlowExceptionsToTaskScheduler", out flowExceptionsToTaskScheduler))
{
flowExceptionsToTaskScheduler = false;

return true;
}

// Just like with the concurrency control option, check that the target command type is asynchronous
if (commandClassType is "global::CommunityToolkit.Mvvm.Input.AsyncRelayCommand")
{
return true;
}
else
{
diagnostics.Add(InvalidFlowExceptionsToTaskSchedulerParameterError, methodSymbol, methodSymbol.ContainingType, methodSymbol);

return false;
}
}

/// <summary>
/// Checks whether or not the user has requested to also generate a cancel command.
/// </summary>
Expand Down
52 changes: 34 additions & 18 deletions CommunityToolkit.Mvvm/Input/AsyncRelayCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,9 @@ public sealed class AsyncRelayCommand : IAsyncRelayCommand, ICancellationAwareCo
private readonly Func<bool>? canExecute;

/// <summary>
/// Indicates whether or not concurrent executions of the command are allowed.
/// The options being set for the current command.
/// </summary>
private readonly bool allowConcurrentExecutions;
private readonly AsyncRelayCommandOptions options;

/// <summary>
/// The <see cref="CancellationTokenSource"/> instance to use to cancel <see cref="cancelableExecute"/>.
Expand Down Expand Up @@ -91,14 +91,14 @@ public AsyncRelayCommand(Func<Task> execute)
/// Initializes a new instance of the <see cref="AsyncRelayCommand"/> class.
/// </summary>
/// <param name="execute">The execution logic.</param>
/// <param name="allowConcurrentExecutions">Whether or not to allow concurrent executions of the command.</param>
/// <param name="options">The options to use to configure the async command.</param>
/// <exception cref="System.ArgumentNullException">Thrown if <paramref name="execute"/> is <see langword="null"/>.</exception>
public AsyncRelayCommand(Func<Task> execute, bool allowConcurrentExecutions)
public AsyncRelayCommand(Func<Task> execute, AsyncRelayCommandOptions options)
{
ArgumentNullException.ThrowIfNull(execute);

this.execute = execute;
this.allowConcurrentExecutions = allowConcurrentExecutions;
this.options = options;
}

/// <summary>
Expand All @@ -117,14 +117,14 @@ public AsyncRelayCommand(Func<CancellationToken, Task> cancelableExecute)
/// Initializes a new instance of the <see cref="AsyncRelayCommand"/> class.
/// </summary>
/// <param name="cancelableExecute">The cancelable execution logic.</param>
/// <param name="allowConcurrentExecutions">Whether or not to allow concurrent executions of the command.</param>
/// <param name="options">The options to use to configure the async command.</param>
/// <exception cref="System.ArgumentNullException">Thrown if <paramref name="cancelableExecute"/> is <see langword="null"/>.</exception>
public AsyncRelayCommand(Func<CancellationToken, Task> cancelableExecute, bool allowConcurrentExecutions)
public AsyncRelayCommand(Func<CancellationToken, Task> cancelableExecute, AsyncRelayCommandOptions options)
{
ArgumentNullException.ThrowIfNull(cancelableExecute);

this.cancelableExecute = cancelableExecute;
this.allowConcurrentExecutions = allowConcurrentExecutions;
this.options = options;
}

/// <summary>
Expand All @@ -147,16 +147,16 @@ public AsyncRelayCommand(Func<Task> execute, Func<bool> canExecute)
/// </summary>
/// <param name="execute">The execution logic.</param>
/// <param name="canExecute">The execution status logic.</param>
/// <param name="allowConcurrentExecutions">Whether or not to allow concurrent executions of the command.</param>
/// <param name="options">The options to use to configure the async command.</param>
/// <exception cref="System.ArgumentNullException">Thrown if <paramref name="execute"/> or <paramref name="canExecute"/> are <see langword="null"/>.</exception>
public AsyncRelayCommand(Func<Task> execute, Func<bool> canExecute, bool allowConcurrentExecutions)
public AsyncRelayCommand(Func<Task> execute, Func<bool> canExecute, AsyncRelayCommandOptions options)
{
ArgumentNullException.ThrowIfNull(execute);
ArgumentNullException.ThrowIfNull(canExecute);

this.execute = execute;
this.canExecute = canExecute;
this.allowConcurrentExecutions = allowConcurrentExecutions;
this.options = options;
}

/// <summary>
Expand All @@ -179,16 +179,16 @@ public AsyncRelayCommand(Func<CancellationToken, Task> cancelableExecute, Func<b
/// </summary>
/// <param name="cancelableExecute">The cancelable execution logic.</param>
/// <param name="canExecute">The execution status logic.</param>
/// <param name="allowConcurrentExecutions">Whether or not to allow concurrent executions of the command.</param>
/// <param name="options">The options to use to configure the async command.</param>
/// <exception cref="System.ArgumentNullException">Thrown if <paramref name="cancelableExecute"/> or <paramref name="canExecute"/> are <see langword="null"/>.</exception>
public AsyncRelayCommand(Func<CancellationToken, Task> cancelableExecute, Func<bool> canExecute, bool allowConcurrentExecutions)
public AsyncRelayCommand(Func<CancellationToken, Task> cancelableExecute, Func<bool> canExecute, AsyncRelayCommandOptions options)
{
ArgumentNullException.ThrowIfNull(cancelableExecute);
ArgumentNullException.ThrowIfNull(canExecute);

this.cancelableExecute = cancelableExecute;
this.canExecute = canExecute;
this.allowConcurrentExecutions = allowConcurrentExecutions;
this.options = options;
}

private Task? executionTask;
Expand Down Expand Up @@ -238,7 +238,7 @@ static async void MonitorTask(AsyncRelayCommand @this, Task task)
@this.PropertyChanged?.Invoke(@this, CanBeCanceledChangedEventArgs);
}

if (!@this.allowConcurrentExecutions)
if ((@this.options & AsyncRelayCommandOptions.AllowConcurrentExecutions) == 0)
{
@this.CanExecuteChanged?.Invoke(@this, EventArgs.Empty);
}
Expand Down Expand Up @@ -273,13 +273,20 @@ public bool CanExecute(object? parameter)
{
bool canExecute = this.canExecute?.Invoke() != false;

return canExecute && (this.allowConcurrentExecutions || ExecutionTask is not { IsCompleted: false });
return canExecute && ((this.options & AsyncRelayCommandOptions.AllowConcurrentExecutions) != 0 || ExecutionTask is not { IsCompleted: false });
}

/// <inheritdoc/>
public void Execute(object? parameter)
{
_ = ExecuteAsync(parameter);
Task executionTask = ExecuteAsync(parameter);

// If exceptions shouldn't flow to the task scheduler, await the resulting task. This is
// delegated to a separate method to keep this one more compact in case the option is set.
if ((this.options & AsyncRelayCommandOptions.FlowExceptionsToTaskScheduler) == 0)
{
AwaitAndThrowIfFailed(executionTask);
}
}

/// <inheritdoc/>
Expand All @@ -304,7 +311,7 @@ public Task ExecuteAsync(object? parameter)
}

// If concurrent executions are disabled, notify the can execute change as well
if (!this.allowConcurrentExecutions)
if ((this.options & AsyncRelayCommandOptions.AllowConcurrentExecutions) == 0)
{
CanExecuteChanged?.Invoke(this, EventArgs.Empty);
}
Expand All @@ -323,4 +330,13 @@ public void Cancel()
PropertyChanged?.Invoke(this, IsCancellationRequestedChangedEventArgs);
}
}

/// <summary>
/// Awaits an input <see cref="Task"/> and throws an exception on the calling context, if the task fails.
/// </summary>
/// <param name="executionTask">The input <see cref="Task"/> instance to await.</param>
internal static async void AwaitAndThrowIfFailed(Task executionTask)
Comment thread
Sergio0694 marked this conversation as resolved.
{
await executionTask;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we add the ConfigureAwait(false) here?

Suggested change
await executionTask;
await executionTask.ConfigureAwait(false);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I didn't add this on purpose to be honest. The idea is that if one of the main points of having exceptions be rethrown is to also help debugging things (other than not get the app into a weird invalid state of course), I feel like it'd be clearer for users to see the exceptions be thrown on the calling context, which would generally be the UI thread. This is also handled in special ways on many frameworks, versus exceptions being thrown on random thread pool threads. I know there's a slight overhead in this, but I feel like that's fine here given it's not like this would be used in a hot path like in some asynchronous code dealing with loops of web requests or anything like that 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's fine then. I thought that could be intentional, but not sure if with the ConfigureAwait(false) the exception will be thrown in another context. Thanks for the explanation (:

}
}
Loading