Skip to content

Commit

Permalink
Fixed mixing interface unions with abstract base class unions
Browse files Browse the repository at this point in the history 
Added diagnostic for cases/arms implemented by throwing NotImplementedException, since CodeCleanup may do this silently (defeating the purpose)
=> release
  • Loading branch information
@hugener
hugener committed Apr 10, 2024
1 parent 44443ec commit 1b8acc5
Show file tree
Hide file tree
Showing 26 changed files with 554 additions and 108 deletions.
33 changes: 17 additions & 16 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,22 +106,23 @@ In addition, the DiscriminatedUnion attribute can specify a flags enum (Generato
* Segregate - Generates an extension method for IEnumerable<TUnion> that segregates all items into buckets of the different result.

## Supported diagnostics:
| Diagnostic Id | Description | Code Fix |
| ------------- | ---------------------------------------------------------------------- | :-------: |
| SDU0001 | Switch does not handled all cases | yes |
| SDU0002 | Switch should not handle default case | yes |
| SDU0003 | Switch has unreachable null case | yes |
| SDU0004 | Class unions must be abstract | yes |
| SDU0005 | Only unions can extended other unions | no |
| SDU0006 | Unions cannot be extended outside their assembly | no |
| SDU0007 | Cases must be declared in the same assembly as their unions | no |
| SDU0008 | Cases should be sealed | yes |
| SDU0009 | Unnested cases should have factory method | PDU0001 |
| SDU0010 | Factory method should have correct CaseTypeAttribute | yes |
| PDU0001 | Make union partial for code generator | yes |
| PDU0002 | Populate union factory methods | yes |
| SDU9999 | Switch should throw in default case | no |
| GDU0001 | Discriminated union declaration could not be found | no |
| Diagnostic Id | Description | Code Fix |
| ------------- | ------------------------------------------------------------------------------------------------------------------------- | :------: |
| SDU0001 | Switch does not handled all cases | yes |
| SDU0002 | Switch should not handle default case | yes |
| SDU0003 | Switch has unreachable null case | yes |
| SDU0004 | Class unions must be abstract | yes |
| SDU0005 | Only unions can extended other unions | no |
| SDU0006 | Unions cannot be extended outside their assembly | no |
| SDU0007 | Cases must be declared in the same assembly as their unions | no |
| SDU0008 | Cases should be sealed | yes |
| SDU0009 | Unnested cases should have factory method | PDU0001 |
| SDU0010 | Factory method should have correct CaseTypeAttribute | yes |
| SDU0011 | Reported when a case is implemented by throwing NotImplementedException, because CodeCleanup may siliently 'fix' SDU0001. | yes |
| PDU0001 | Make union partial for code generator | yes |
| PDU0002 | Populate union factory methods | yes |
| SDU9999 | Switch should throw in default case | no |
| GDU0001 | Discriminated union declaration could not be found | no |

## Issues/Todos
* Switch appears with red squiggly lines in VS: https://github.com/dotnet/roslyn/issues/57041
Expand Down
7 changes: 6 additions & 1 deletion Source/Sundew.DiscriminatedUnions.Analyzer/CaseInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,15 @@ public readonly struct CaseInfo
/// <summary>
/// Gets the type.
/// </summary>
public ISymbol Symbol { get; init; }
public ISymbol? Symbol { get; init; }

/// <summary>
/// Gets a value indicating whether the case is handled.
/// </summary>
public bool HandlesCase { get; init; }

/// <summary>
/// Gets the syntax node throwing NotImplementedException if present.
/// </summary>
public IOperation? ThrowingNotImplementedException { get; init; }
}
36 changes: 28 additions & 8 deletions Source/Sundew.DiscriminatedUnions.Analyzer/DiagnosticReporterHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,28 +16,48 @@ namespace Sundew.DiscriminatedUnions.Analyzer;
internal static class DiagnosticReporterHelper
{
private const string Null = "null";
private const string Unknown = "Unknown";

public static void ReportDiagnostics(
IReadOnlyList<ISymbol> caseTypes,
IEnumerable<ISymbol> handledCaseTypes,
IOperation? nullCase,
IReadOnlyList<CaseInfo> handledCaseTypes,
NullCase? nullCaseOption,
SwitchNullability switchNullability,
ITypeSymbol unionType,
IOperation operation,
Action<Diagnostic> reportDiagnostic)
{
foreach (var caseInfo in handledCaseTypes.Where(x => x.ThrowingNotImplementedException != default))
{
reportDiagnostic(Diagnostic.Create(
DiscriminatedUnionsAnalyzer.CaseShouldBeImplementedRule,
caseInfo.ThrowingNotImplementedException!.Syntax.GetLocation(),
caseInfo.Symbol?.Name ?? Unknown));
}

var missingCaseTypes = new HashSet<ISymbol>(caseTypes, SymbolEqualityComparer.Default);
foreach (var handledCaseType in handledCaseTypes)
foreach (var handledCaseType in handledCaseTypes.Where(x => x is { HandlesCase: true, Symbol: not null }).Select(x => x.Symbol!))
{
missingCaseTypes.Remove(handledCaseType);
}

if (nullCase != null && switchNullability == SwitchNullability.HasUnreachableNullCase)
if (nullCaseOption.HasValue)
{
reportDiagnostic(Diagnostic.Create(
DiscriminatedUnionsAnalyzer.SwitchHasUnreachableNullCaseRule,
nullCase.Syntax.GetLocation(),
DiagnosticSeverity.Error));
var nullCase = nullCaseOption.Value;
if (switchNullability == SwitchNullability.HasUnreachableNullCase)
{
reportDiagnostic(Diagnostic.Create(
DiscriminatedUnionsAnalyzer.SwitchHasUnreachableNullCaseRule,
nullCase.NullOperation.Syntax.GetLocation()));
}

if (nullCase.ThrowingNotImplementedException != default)
{
reportDiagnostic(Diagnostic.Create(
DiscriminatedUnionsAnalyzer.CaseShouldBeImplementedRule,
nullCase.ThrowingNotImplementedException.Syntax.GetLocation(),
Null));
}
}

var isNullCaseMissing = switchNullability == SwitchNullability.IsMissingNullCase;
Expand Down
19 changes: 19 additions & 0 deletions Source/Sundew.DiscriminatedUnions.Analyzer/DiscriminatedUnionsAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ public class DiscriminatedUnionsAnalyzer : DiagnosticAnalyzer
/// </summary>
public const string FactoryMethodShouldHaveMatchingCaseTypeAttributeDiagnosticId = "SDU0010";

/// <summary>
/// Diagnostic id indicating that cases should be implemented.
/// </summary>
public const string CaseShouldBeImplementedDiagnosticId = "SDU0011";

/// <summary>
/// Diagnostic id indicating that the switch should throw in default case.
/// </summary>
Expand Down Expand Up @@ -206,6 +211,19 @@ public class DiscriminatedUnionsAnalyzer : DiagnosticAnalyzer
true,
nameof(Resources.FactoryMethodShouldHaveMatchingCaseTypeAttributeDescription));

/// <summary>
/// A case should be implemented. E.g. not throw NotImplementedException.
/// </summary>
public static readonly DiagnosticDescriptor CaseShouldBeImplementedRule =
DiagnosticDescriptorHelper.Create(
CaseShouldBeImplementedDiagnosticId,
nameof(Resources.CaseShouldBeImplementedTitle),
nameof(Resources.CaseShouldBeImplementedMessageFormat),
Category,
DiagnosticSeverity.Error,
true,
nameof(Resources.CaseShouldBeImplementedDescription));

/// <summary>
/// The switch should throw in default case rule.
/// </summary>
Expand Down Expand Up @@ -235,6 +253,7 @@ public class DiscriminatedUnionsAnalyzer : DiagnosticAnalyzer
CasesShouldBeSealedRule,
UnnestedCasesShouldHaveFactoryMethodRule,
FactoryMethodShouldHaveMatchingCaseTypeAttributeRule,
CaseShouldBeImplementedRule,
SwitchShouldThrowInDefaultCaseRule);

/// <summary>
Expand Down
15 changes: 15 additions & 0 deletions Source/Sundew.DiscriminatedUnions.Analyzer/NullCase.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// --------------------------------------------------------------------------------------------------------------------
// <copyright file="NullCase.cs" company="Sundews">
// Copyright (c) Sundews. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
// </copyright>
// --------------------------------------------------------------------------------------------------------------------

namespace Sundew.DiscriminatedUnions.Analyzer;

using Microsoft.CodeAnalysis;

/// <summary>
/// Contains information about a case in a switch statement or expression.
/// </summary>
public readonly record struct NullCase(IOperation NullOperation, IOperation? ThrowingNotImplementedException);
27 changes: 27 additions & 0 deletions Source/Sundew.DiscriminatedUnions.Analyzer/Resources.Designer.cs
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions Source/Sundew.DiscriminatedUnions.Analyzer/Resources.resx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,4 +249,13 @@
<data name="MakeUnionPartialTitle" xml:space="preserve">
<value>Make union partial.</value>
</data>
<data name="CaseShouldBeImplementedDescription" xml:space="preserve">
<value>Reported when a case is implemented by throwing NotImplementedException, because CodeCleanup may siliently 'fix' SDU0001.</value>
</data>
<data name="CaseShouldBeImplementedMessageFormat" xml:space="preserve">
<value>Case '{0}' is not implemented</value>
</data>
<data name="CaseShouldBeImplementedTitle" xml:space="preserve">
<value>Case is implemented by throwing NotImplementedException.</value>
</data>
</root>
37 changes: 24 additions & 13 deletions Source/Sundew.DiscriminatedUnions.Analyzer/SwitchExpression/SwitchExpressionHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,25 +26,30 @@ public static IEnumerable<CaseInfo> GetHandledCaseTypes(ISwitchExpressionOperati
{
return switchExpressionOperation.Arms.SelectMany((switchExpressionArmOperation) =>
{
var throwingNotImplementedException = UnionHelper.GetThrowingNotImplementedException(switchExpressionArmOperation.Value);
if (switchExpressionArmOperation.Pattern is IDeclarationPatternOperation declarationPatternSyntax)
{
return UnionHelper.GetHandledSymbols(declarationPatternSyntax.MatchedType as INamedTypeSymbol, true);
return UnionHelper.GetHandledSymbols(declarationPatternSyntax.MatchedType as INamedTypeSymbol, true, throwingNotImplementedException);
}

if (switchExpressionArmOperation.Pattern is ITypePatternOperation typePatternOperation)
{
return UnionHelper.GetHandledSymbols(typePatternOperation.MatchedType as INamedTypeSymbol, true);
return UnionHelper.GetHandledSymbols(typePatternOperation.MatchedType as INamedTypeSymbol, true, throwingNotImplementedException);
}

if (switchExpressionArmOperation.Pattern is IConstantPatternOperation { Value: IFieldReferenceOperation fieldReferenceOperation })
{
return UnionHelper.GetHandledFieldTypeSymbols(fieldReferenceOperation.Field);
return UnionHelper.GetHandledFieldTypeSymbols(fieldReferenceOperation.Field, throwingNotImplementedException);
}

return UnionHelper.GetHandledSymbols(switchExpressionArmOperation.Pattern.NarrowedType as INamedTypeSymbol, false);
if (IsNullLiteralArmOperation(switchExpressionArmOperation))
{
return Enumerable.Empty<(ISymbol? Symbol, bool HandlesCase, IOperation? ThrowingNotImplementedException)>();
}

return UnionHelper.GetHandledSymbols(switchExpressionArmOperation.Pattern.NarrowedType as INamedTypeSymbol, false, throwingNotImplementedException);
})
.Where(x => x.Symbol != null)
.Select(x => new CaseInfo { Symbol = x.Symbol!, HandlesCase = x.HandlesCase });
.Select(x => new CaseInfo { Symbol = x.Symbol, HandlesCase = x.HandlesCase, ThrowingNotImplementedException = x.ThrowingNotImplementedException });
}

/// <summary>
Expand All @@ -54,14 +59,20 @@ public static IEnumerable<CaseInfo> GetHandledCaseTypes(ISwitchExpressionOperati
/// <returns>
/// <c>true</c> if [has null case] [the specified switch expression operation]; otherwise, <c>false</c>.
/// </returns>
public static ISwitchExpressionArmOperation? GetNullCase(ISwitchExpressionOperation switchExpressionOperation)
public static NullCase? GetNullCase(ISwitchExpressionOperation switchExpressionOperation)
{
var nullCase = switchExpressionOperation.Arms.FirstOrDefault(IsNullLiteralArmOperation);
return nullCase == default ? default(NullCase?) : new NullCase(nullCase, UnionHelper.GetThrowingNotImplementedException(nullCase.Value));
}

private static bool IsNullLiteralArmOperation(ISwitchExpressionArmOperation x)
{
return switchExpressionOperation.Arms.FirstOrDefault(x => x.Pattern is IConstantPatternOperation constantPatternOperation &&
((constantPatternOperation.Value is IConversionOperation conversionOperation &&
conversionOperation.Operand is ILiteralOperation conversionLiteralOperation &&
IsNullLiteral(conversionLiteralOperation)) ||
(constantPatternOperation.Value is ILiteralOperation literalOperation &&
IsNullLiteral(literalOperation))));
return x.Pattern is IConstantPatternOperation constantPatternOperation &&
((constantPatternOperation.Value is IConversionOperation conversionOperation &&
conversionOperation.Operand is ILiteralOperation conversionLiteralOperation &&
IsNullLiteral(conversionLiteralOperation)) ||
(constantPatternOperation.Value is ILiteralOperation literalOperation &&
IsNullLiteral(literalOperation)));
}

private static bool IsNullLiteral(ILiteralOperation literalOperation)
Expand Down
2 changes: 1 addition & 1 deletion Source/Sundew.DiscriminatedUnions.Analyzer/SwitchExpression/UnionSwitchExpressionAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public void Analyze(OperationAnalysisContext operationAnalysisContext)
var caseTypes = UnionHelper.GetKnownCases(nonNullableUnionType).ToList();
DiagnosticReporterHelper.ReportDiagnostics(
caseTypes,
SwitchExpressionHelper.GetHandledCaseTypes(switchExpressionOperation).Where(x => x.HandlesCase).Select(x => x.Symbol),
SwitchExpressionHelper.GetHandledCaseTypes(switchExpressionOperation).ToList(),
nullCase,
switchNullability,
unionType,
Expand Down
Loading

0 comments on commit 1b8acc5

Please sign in to comment.