-
Notifications
You must be signed in to change notification settings - Fork 269
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
MSTEST0013: AssemblyCleanup should be valid #2353
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
117 changes: 117 additions & 0 deletions
117
src/Analyzers/MSTest.Analyzers/AssemblyCleanupShouldBeValidAnalyzer.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using System.Collections.Immutable; | ||
|
|
||
| using Analyzer.Utilities.Extensions; | ||
|
|
||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.Diagnostics; | ||
|
|
||
| using MSTest.Analyzers.Helpers; | ||
|
|
||
| namespace MSTest.Analyzers; | ||
|
|
||
| [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] | ||
| public sealed class AssemblyCleanupShouldBeValidAnalyzer : DiagnosticAnalyzer | ||
| { | ||
| private static readonly LocalizableResourceString Title = new(nameof(Resources.AssemblyCleanupShouldBeValidTitle), Resources.ResourceManager, typeof(Resources)); | ||
| private static readonly LocalizableResourceString Description = new(nameof(Resources.AssemblyCleanupShouldBeValidDescription), Resources.ResourceManager, typeof(Resources)); | ||
| private static readonly LocalizableResourceString MessageFormat = new(nameof(Resources.AssemblyCleanupShouldBeValidMessageFormat_Public), Resources.ResourceManager, typeof(Resources)); | ||
|
|
||
| internal static readonly DiagnosticDescriptor PublicRule = DiagnosticDescriptorHelper.Create( | ||
| DiagnosticIds.AssemblyCleanupShouldBeValidRuleId, | ||
| Title, | ||
| MessageFormat, | ||
| Description, | ||
| Category.Usage, | ||
| DiagnosticSeverity.Warning, | ||
| isEnabledByDefault: true); | ||
|
|
||
| internal static readonly DiagnosticDescriptor NotStaticRule = PublicRule.WithMessage(new(nameof(Resources.AssemblyCleanupShouldBeValidMessageFormat_NotStatic), Resources.ResourceManager, typeof(Resources))); | ||
| internal static readonly DiagnosticDescriptor NoParametersRule = PublicRule.WithMessage(new(nameof(Resources.AssemblyCleanupShouldBeValidMessageFormat_NoParameters), Resources.ResourceManager, typeof(Resources))); | ||
| internal static readonly DiagnosticDescriptor ReturnTypeRule = PublicRule.WithMessage(new(nameof(Resources.AssemblyCleanupShouldBeValidMessageFormat_ReturnType), Resources.ResourceManager, typeof(Resources))); | ||
| internal static readonly DiagnosticDescriptor NotAsyncVoidRule = PublicRule.WithMessage(new(nameof(Resources.AssemblyCleanupShouldBeValidMessageFormat_NotAsyncVoid), Resources.ResourceManager, typeof(Resources))); | ||
| internal static readonly DiagnosticDescriptor NotGenericRule = PublicRule.WithMessage(new(nameof(Resources.AssemblyCleanupShouldBeValidMessageFormat_NotGeneric), Resources.ResourceManager, typeof(Resources))); | ||
| internal static readonly DiagnosticDescriptor OrdinaryRule = PublicRule.WithMessage(new(nameof(Resources.AssemblyCleanupShouldBeValidMessageFormat_Ordinary), Resources.ResourceManager, typeof(Resources))); | ||
| internal static readonly DiagnosticDescriptor NotAbstractRule = PublicRule.WithMessage(new(nameof(Resources.TestInitializeShouldBeValidMessageFormat_NotAbstract), Resources.ResourceManager, typeof(Resources))); | ||
|
|
||
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } | ||
| = ImmutableArray.Create(PublicRule); | ||
|
|
||
| public override void Initialize(AnalysisContext context) | ||
| { | ||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); | ||
| context.EnableConcurrentExecution(); | ||
|
|
||
| context.RegisterCompilationStartAction(context => | ||
| { | ||
| if (context.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.MicrosoftVisualStudioTestToolsUnitTestingAssemblyCleanupAttribute, out var assemblyCleanupAttributeSymbol)) | ||
| { | ||
| var taskSymbol = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemThreadingTasksTask); | ||
| var valueTaskSymbol = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemThreadingTasksValueTask); | ||
| bool canDiscoverInternals = context.Compilation.CanDiscoverInternals(); | ||
| context.RegisterSymbolAction( | ||
| context => AnalyzeSymbol(context, assemblyCleanupAttributeSymbol, taskSymbol, valueTaskSymbol, canDiscoverInternals), | ||
| SymbolKind.Method); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| private static void AnalyzeSymbol(SymbolAnalysisContext context, INamedTypeSymbol assemblyCleanupAttributeSymbol, INamedTypeSymbol? taskSymbol, | ||
| INamedTypeSymbol? valueTaskSymbol, bool canDiscoverInternals) | ||
| { | ||
| var methodSymbol = (IMethodSymbol)context.Symbol; | ||
| if (!methodSymbol.GetAttributes().Any(attr => SymbolEqualityComparer.Default.Equals(attr.AttributeClass, assemblyCleanupAttributeSymbol))) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (methodSymbol.MethodKind != MethodKind.Ordinary) | ||
| { | ||
| context.ReportDiagnostic(methodSymbol.CreateDiagnostic(OrdinaryRule, methodSymbol.Name)); | ||
|
|
||
| // Do not check the other criteria, users should fix the method kind first. | ||
| return; | ||
| } | ||
|
|
||
| if (methodSymbol.Parameters.Length > 0) | ||
| { | ||
| context.ReportDiagnostic(methodSymbol.CreateDiagnostic(NoParametersRule, methodSymbol.Name)); | ||
| } | ||
|
|
||
| if (methodSymbol.IsGenericMethod) | ||
| { | ||
| context.ReportDiagnostic(methodSymbol.CreateDiagnostic(NotGenericRule, methodSymbol.Name)); | ||
| } | ||
|
|
||
| if (methodSymbol.IsAbstract) | ||
| { | ||
| context.ReportDiagnostic(methodSymbol.CreateDiagnostic(NotAbstractRule, methodSymbol.Name)); | ||
| } | ||
|
|
||
| if (methodSymbol.IsStatic) | ||
| { | ||
| context.ReportDiagnostic(methodSymbol.CreateDiagnostic(NotStaticRule, methodSymbol.Name)); | ||
| } | ||
|
|
||
| if (methodSymbol.ReturnsVoid && methodSymbol.IsAsync) | ||
| { | ||
| context.ReportDiagnostic(methodSymbol.CreateDiagnostic(NotAsyncVoidRule, methodSymbol.Name)); | ||
| } | ||
|
|
||
| if (methodSymbol.DeclaredAccessibility != Accessibility.Public | ||
| || (!canDiscoverInternals && methodSymbol.GetResultantVisibility() != SymbolVisibility.Public) | ||
| || (canDiscoverInternals && methodSymbol.GetResultantVisibility() == SymbolVisibility.Private)) | ||
| { | ||
| context.ReportDiagnostic(methodSymbol.CreateDiagnostic(PublicRule, methodSymbol.Name)); | ||
| } | ||
|
|
||
| if (!methodSymbol.ReturnsVoid | ||
| && (taskSymbol is null || !SymbolEqualityComparer.Default.Equals(methodSymbol.ReturnType, taskSymbol)) | ||
| && (valueTaskSymbol is null || !SymbolEqualityComparer.Default.Equals(methodSymbol.ReturnType, valueTaskSymbol))) | ||
| { | ||
| context.ReportDiagnostic(methodSymbol.CreateDiagnostic(ReturnTypeRule, methodSymbol.Name)); | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not run.
The declared accessibility of the method should always be public. The DiscoverInternals should only apply to the class (when disabled only public classes with [TestClass] should be scanned, if enabled public and internal classes should be scanned).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assembly initialize rule has the same problem:
https://github.com/microsoft/testfx/pull/2328/files#diff-8a52ce5241463aca67cecbf9557a84903d08cf5e63c7889953064fdfa27ae28dR99
You will run into this:
https://github.com/microsoft/testfx/blob/main/src/Adapter/MSTest.TestAdapter/Extensions/MethodInfoExtensions.cs#L39-L48
@Evangelink this resource, and other resources don't reflect the changes made for Task and ValueTask https://github.com/microsoft/testfx/blob/main/src/Adapter/MSTest.TestAdapter/Resources/Resource.resx#L212
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nohwnd Yeah that's the correct case here, I added tests that test all those scenarios correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nohwnd
DeclaredAccesibilityis what is declared on the method and is the first check Engy's doing here. The other checks are usingGetResultantVisibilitythat walks up tree (class + outer classes) to get the final accessibility. We had offline chat with Engy and the code she wrotes LGTM.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I see some other resources are missing edit. I'll do a follow-up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, and I am wrong. :) (For others: The first check makes sure that the class always has
publicon the member, the other two check the visibility due to placement in a internal or private class (or public class in internal class etc.)).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@engyebrahim Would you mind doing a follow-up PR where you add a comment on top of the check with what Jakub wrotes for the various analyzers doing the same trick? It will be helpful for other devs.