Skip to content

Add more analyzers, enable unit tests for partial properties #1010

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 14 commits into from
Dec 3, 2024
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
24 changes: 24 additions & 0 deletions dotnet Community Toolkit.sln
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ Project("{D954291E-2A0B-460D-934E-DC6B0785DB48}") = "CommunityToolkit.Mvvm.CodeF
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "CommunityToolkit.Mvvm.CodeFixers.Roslyn4120", "src\CommunityToolkit.Mvvm.CodeFixers.Roslyn4120\CommunityToolkit.Mvvm.CodeFixers.Roslyn4120.csproj", "{98572004-D29A-486E-8053-6D409557CE44}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "CommunityToolkit.Mvvm.Roslyn4120.UnitTests", "tests\CommunityToolkit.Mvvm.Roslyn4120.UnitTests\CommunityToolkit.Mvvm.Roslyn4120.UnitTests.csproj", "{87BF1537-935A-414D-8318-458F61A6E562}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand Down Expand Up @@ -525,6 +527,26 @@ Global
{98572004-D29A-486E-8053-6D409557CE44}.Release|x64.Build.0 = Release|Any CPU
{98572004-D29A-486E-8053-6D409557CE44}.Release|x86.ActiveCfg = Release|Any CPU
{98572004-D29A-486E-8053-6D409557CE44}.Release|x86.Build.0 = Release|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Debug|Any CPU.Build.0 = Debug|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Debug|ARM.ActiveCfg = Debug|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Debug|ARM.Build.0 = Debug|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Debug|ARM64.ActiveCfg = Debug|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Debug|ARM64.Build.0 = Debug|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Debug|x64.ActiveCfg = Debug|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Debug|x64.Build.0 = Debug|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Debug|x86.ActiveCfg = Debug|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Debug|x86.Build.0 = Debug|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Release|Any CPU.ActiveCfg = Release|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Release|Any CPU.Build.0 = Release|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Release|ARM.ActiveCfg = Release|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Release|ARM.Build.0 = Release|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Release|ARM64.ActiveCfg = Release|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Release|ARM64.Build.0 = Release|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Release|x64.ActiveCfg = Release|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Release|x64.Build.0 = Release|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Release|x86.ActiveCfg = Release|Any CPU
{87BF1537-935A-414D-8318-458F61A6E562}.Release|x86.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand All @@ -548,6 +570,7 @@ Global
{ECFE93AA-4B98-4292-B3FA-9430D513B4F9} = {B30036C4-D514-4E5B-A323-587A061772CE}
{4FCD501C-1BB5-465C-AD19-356DAB6600C6} = {B30036C4-D514-4E5B-A323-587A061772CE}
{C342302D-A263-42D6-B8EE-01DEF8192690} = {B30036C4-D514-4E5B-A323-587A061772CE}
{87BF1537-935A-414D-8318-458F61A6E562} = {B30036C4-D514-4E5B-A323-587A061772CE}
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {5403B0C4-F244-4F73-A35C-FE664D0F4345}
Expand All @@ -556,6 +579,7 @@ Global
tests\CommunityToolkit.Mvvm.ExternalAssembly\CommunityToolkit.Mvvm.ExternalAssembly.projitems*{4fcd501c-1bb5-465c-ad19-356dab6600c6}*SharedItemsImports = 5
tests\CommunityToolkit.Mvvm.UnitTests\CommunityToolkit.Mvvm.UnitTests.projitems*{5b44f7f1-dca2-4776-924e-a266f7bbf753}*SharedItemsImports = 5
src\CommunityToolkit.Mvvm.SourceGenerators\CommunityToolkit.Mvvm.SourceGenerators.projitems*{5e7f1212-a54b-40ca-98c5-1ff5cd1a1638}*SharedItemsImports = 13
tests\CommunityToolkit.Mvvm.UnitTests\CommunityToolkit.Mvvm.UnitTests.projitems*{87bf1537-935a-414d-8318-458f61a6e562}*SharedItemsImports = 5
src\CommunityToolkit.Mvvm.CodeFixers\CommunityToolkit.Mvvm.CodeFixers.projitems*{98572004-d29a-486e-8053-6d409557ce44}*SharedItemsImports = 5
src\CommunityToolkit.Mvvm.CodeFixers\CommunityToolkit.Mvvm.CodeFixers.projitems*{a2ebda90-b720-430d-83f5-c6bcc355232c}*SharedItemsImports = 13
tests\CommunityToolkit.Mvvm.UnitTests\CommunityToolkit.Mvvm.UnitTests.projitems*{ad9c3223-8e37-4fd4-a0d4-a45119551d3a}*SharedItemsImports = 5
Expand Down
2 changes: 1 addition & 1 deletion global.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"sdk": {
"version": "8.0.403",
"version": "9.0.100",
"rollForward": "latestFeature",
"allowPrerelease": false
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,4 +92,7 @@ MVVMTK0047 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator
MVVMTK0048 | CommunityToolkit.Mvvm.SourceGenerators.RelayCommandGenerator | Warning | See https://aka.ms/mvvmtoolkit/errors/mvvmtk0048
MVVMTK0049 | CommunityToolkit.Mvvm.SourceGenerators.INotifyPropertyChangedGenerator | Warning | See https://aka.ms/mvvmtoolkit/errors/mvvmtk0049
MVVMTK0050 | CommunityToolkit.Mvvm.SourceGenerators.ObservableObjectGenerator | Warning | See https://aka.ms/mvvmtoolkit/errors/mvvmtk0050
MVVMTK0051 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Info | See https://aka.ms/mvvmtoolkit/errors/mvvmtk0050
MVVMTK0051 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Info | See https://aka.ms/mvvmtoolkit/errors/mvvmtk0051
MVVMTK0052 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Error | See https://aka.ms/mvvmtoolkit/errors/mvvmtk0052
MVVMTK0053 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Error | See https://aka.ms/mvvmtoolkit/errors/mvvmtk0053
MVVMTK0054 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Error | See https://aka.ms/mvvmtoolkit/errors/mvvmtk0054
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
<Compile Include="$(MSBuildThisFileDirectory)ComponentModel\TransitiveMembersGenerator.Execute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Diagnostics\Analyzers\AsyncVoidReturningRelayCommandMethodAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Diagnostics\Analyzers\InvalidGeneratedPropertyObservablePropertyAttributeAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Diagnostics\Analyzers\InvalidPartialPropertyLevelObservablePropertyAttributeAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Diagnostics\Analyzers\WinRTClassUsingNotifyPropertyChangedAttributesAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Diagnostics\Analyzers\WinRTGeneratedBindableCustomPropertyWithBasesMemberAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Diagnostics\Analyzers\WinRTRelayCommandIsNotGeneratedBindableCustomPropertyCompatibleAnalyzer.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ static bool IsCandidateField(SyntaxNode node, out TypeDeclarationSyntax? contain
/// <param name="node">The <see cref="MemberDeclarationSyntax"/> instance to process.</param>
/// <param name="semanticModel">The <see cref="SemanticModel"/> instance for the current run.</param>
/// <returns>Whether <paramref name="node"/> is valid.</returns>
public static bool IsCandidateValidForCompilation(SyntaxNode node, SemanticModel semanticModel)
public static bool IsCandidateValidForCompilation(MemberDeclarationSyntax node, SemanticModel semanticModel)
{
// At least C# 8 is always required
if (!semanticModel.Compilation.HasLanguageVersionAtLeastEqualTo(LanguageVersion.CSharp8))
Expand All @@ -90,6 +90,35 @@ public static bool IsCandidateValidForCompilation(SyntaxNode node, SemanticModel
return true;
}

/// <summary>
/// Performs additional checks before running the core generation logic.
/// </summary>
/// <param name="memberSymbol">The input <see cref="ISymbol"/> instance to process.</param>
/// <returns>Whether <paramref name="memberSymbol"/> is valid.</returns>
public static bool IsCandidateSymbolValid(ISymbol memberSymbol)
{
#if ROSLYN_4_12_0_OR_GREATER
// We only need additional checks for properties (Roslyn already validates things for fields in our scenarios)
if (memberSymbol is IPropertySymbol propertySymbol)
{
// Ensure that the property declaration is a partial definition with no implementation
if (propertySymbol is not { IsPartialDefinition: true, PartialImplementationPart: null })
{
return false;
}

// Also ignore all properties that have an invalid declaration
if (propertySymbol.ReturnsByRef || propertySymbol.ReturnsByRefReadonly || propertySymbol.Type.IsRefLikeType)
{
return false;
}
}
#endif

// We assume all other cases are supported (other failure cases will be detected later)
return true;
}

/// <summary>
/// Gets the candidate <see cref="MemberDeclarationSyntax"/> after the initial filtering.
/// </summary>
Expand Down Expand Up @@ -140,13 +169,11 @@ public static bool TryGetInfo(
return false;
}

using ImmutableArrayBuilder<DiagnosticInfo> builder = ImmutableArrayBuilder<DiagnosticInfo>.Rent();

// Validate the target type
if (!IsTargetTypeValid(memberSymbol, out bool shouldInvokeOnPropertyChanging))
{
propertyInfo = null;
diagnostics = builder.ToImmutable();
diagnostics = ImmutableArray<DiagnosticInfo>.Empty;

return false;
}
Expand All @@ -168,7 +195,7 @@ public static bool TryGetInfo(
if (fieldName == propertyName && memberSyntax.IsKind(SyntaxKind.FieldDeclaration))
{
propertyInfo = null;
diagnostics = builder.ToImmutable();
diagnostics = ImmutableArray<DiagnosticInfo>.Empty;

// If the generated property would collide, skip generating it entirely. This makes sure that
// users only get the helpful diagnostic about the collision, and not the normal compiler error
Expand All @@ -182,7 +209,7 @@ public static bool TryGetInfo(
if (IsGeneratedPropertyInvalid(propertyName, GetPropertyType(memberSymbol)))
{
propertyInfo = null;
diagnostics = builder.ToImmutable();
diagnostics = ImmutableArray<DiagnosticInfo>.Empty;

return false;
}
Expand Down Expand Up @@ -232,6 +259,8 @@ public static bool TryGetInfo(

token.ThrowIfCancellationRequested();

using ImmutableArrayBuilder<DiagnosticInfo> builder = ImmutableArrayBuilder<DiagnosticInfo>.Rent();

// Gather attributes info
foreach (AttributeData attributeData in memberSymbol.GetAttributes())
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
using CommunityToolkit.Mvvm.SourceGenerators.Helpers;
using CommunityToolkit.Mvvm.SourceGenerators.Models;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;

namespace CommunityToolkit.Mvvm.SourceGenerators;
Expand Down Expand Up @@ -38,6 +37,14 @@ public void Initialize(IncrementalGeneratorInitializationContext context)
return default;
}

// Validate the symbol as well before doing any work
if (!Execute.IsCandidateSymbolValid(context.TargetSymbol))
{
return default;
}

token.ThrowIfCancellationRequested();

// Get the hierarchy info for the target symbol, and try to gather the property info
HierarchyInfo hierarchy = HierarchyInfo.From(context.TargetSymbol.ContainingType);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#if ROSLYN_4_12_0_OR_GREATER

using System.Collections.Immutable;
using CommunityToolkit.Mvvm.SourceGenerators.Extensions;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using static CommunityToolkit.Mvvm.SourceGenerators.Diagnostics.DiagnosticDescriptors;

namespace CommunityToolkit.Mvvm.SourceGenerators;

/// <summary>
/// A diagnostic analyzer that generates an error whenever <c>[ObservableProperty]</c> is used on an invalid partial property declaration.
/// </summary>
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class InvalidPartialPropertyLevelObservablePropertyAttributeAnalyzer : DiagnosticAnalyzer
{
/// <inheritdoc/>
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(
InvalidObservablePropertyDeclarationIsNotIncompletePartialDefinition,
InvalidObservablePropertyDeclarationReturnsByRef,
InvalidObservablePropertyDeclarationReturnsRefLikeType);

/// <inheritdoc/>
public override void Initialize(AnalysisContext context)
{
// This generator is intentionally also analyzing generated code, because Roslyn will interpret properties
// that have '[GeneratedCode]' on them as being generated (and the same will apply to all partial parts).
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics);
context.EnableConcurrentExecution();

context.RegisterCompilationStartAction(static context =>
{
// Get the [ObservableProperty] and [GeneratedCode] symbols
if (context.Compilation.GetTypeByMetadataName("CommunityToolkit.Mvvm.ComponentModel.ObservablePropertyAttribute") is not INamedTypeSymbol observablePropertySymbol ||
context.Compilation.GetTypeByMetadataName("System.CodeDom.Compiler.GeneratedCodeAttribute") is not { } generatedCodeAttributeSymbol)
{
return;
}

context.RegisterSymbolAction(context =>
{
// Ensure that we have some target property to analyze (also skip implementation parts)
if (context.Symbol is not IPropertySymbol { PartialDefinitionPart: null } propertySymbol)
{
return;
}

// If the property is not using [ObservableProperty], there's nothing to do
if (!context.Symbol.TryGetAttributeWithType(observablePropertySymbol, out AttributeData? observablePropertyAttribute))
{
return;
}

// Emit an error if the property is not a partial definition with no implementation...
if (propertySymbol is not { IsPartialDefinition: true, PartialImplementationPart: null })
{
// ...But only if it wasn't actually generated by the [ObservableProperty] generator.
bool isImplementationAllowed =
propertySymbol is { IsPartialDefinition: true, PartialImplementationPart: IPropertySymbol implementationPartSymbol } &&
implementationPartSymbol.TryGetAttributeWithType(generatedCodeAttributeSymbol, out AttributeData? generatedCodeAttributeData) &&
generatedCodeAttributeData.TryGetConstructorArgument(0, out string? toolName) &&
toolName == typeof(ObservablePropertyGenerator).FullName;

// Emit the diagnostic only for cases that were not valid generator outputs
if (!isImplementationAllowed)
{
context.ReportDiagnostic(Diagnostic.Create(
InvalidObservablePropertyDeclarationIsNotIncompletePartialDefinition,
observablePropertyAttribute.GetLocation(),
propertySymbol.ContainingType,
propertySymbol.Name));
}
}

// Emit an error if the property returns a value by ref
if (propertySymbol.ReturnsByRef || propertySymbol.ReturnsByRefReadonly)
{
context.ReportDiagnostic(Diagnostic.Create(
InvalidObservablePropertyDeclarationReturnsByRef,
observablePropertyAttribute.GetLocation(),
propertySymbol.ContainingType,
propertySymbol.Name));
}

// Emit an error if the property type is a ref struct
if (propertySymbol.Type.IsRefLikeType)
{
context.ReportDiagnostic(Diagnostic.Create(
InvalidObservablePropertyDeclarationReturnsRefLikeType,
observablePropertyAttribute.GetLocation(),
propertySymbol.ContainingType,
propertySymbol.Name));
}
}, SymbolKind.Property);
});
}
}

#endif
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ public override void Initialize(AnalysisContext context)
InvalidPropertyDeclarationForObservableProperty,
observablePropertyAttribute.GetLocation(),
propertySymbol.ContainingType,
propertySymbol));
propertySymbol.Name));

return;
}
}
}, SymbolKind.Property);
Expand Down Expand Up @@ -91,6 +93,14 @@ internal static bool IsValidCandidateProperty(SyntaxNode node, out TypeDeclarati
return false;
}

// Static properties are not supported
if (property.Modifiers.Any(SyntaxKind.StaticKeyword))
{
containingTypeNode = null;

return false;
}

// The accessors must be a get and a set (with any accessibility)
if (accessors[0].Kind() is not (SyntaxKind.GetAccessorDeclaration or SyntaxKind.SetAccessorDeclaration) ||
accessors[1].Kind() is not (SyntaxKind.GetAccessorDeclaration or SyntaxKind.SetAccessorDeclaration))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public override void Initialize(AnalysisContext context)
UnsupportedRoslynVersionForObservablePartialPropertySupport,
propertySymbol.Locations.FirstOrDefault(),
propertySymbol.ContainingType,
propertySymbol));
propertySymbol.Name));
}
}, SymbolKind.Property);
});
Expand Down
Loading