From 6202f9b3c4a50fbeb03c622a5fd984194279aace Mon Sep 17 00:00:00 2001 From: "Tomohisa Tanaka (Maroontress)" Date: Fri, 17 Jan 2025 18:25:53 +0900 Subject: [PATCH] Add NoUsingDeclaration analyzer (#195) - Fix the test framework so that test cases can import System.Net - Ignore SA1008 - Move StyleChecker.Refactoring.UnnecessaryUsing.Classes to StyleChecker.Refactoring - Add GetTypeInfoSupplier(SemanticModelAnalysisContext) methods to SmaContextExtentions class - Add GetFullNameWithoutNullability(ITypeSymbol) method to TypeSymbols class - Add ToSymbol(VariableDeclaratorSyntax) to ISymbolizer interface --- .../StyleChecker.Test/Framework/Projects.cs | 10 +- .../NoUsingDeclaration/AnalyzerTest.cs | 33 +++ .../Refactoring/NoUsingDeclaration/Code.cs | 47 +++ .../Refactoring/NoUsingDeclaration/CodeFix.cs | 40 +++ .../Refactoring/NoUsingDeclaration/Okay.cs | 113 ++++++++ .../StyleChecker.Test.csproj | 12 + StyleChecker/StyleChecker/.editorconfig | 3 + .../{UnnecessaryUsing => }/Classes.cs | 2 +- .../StyleChecker/Refactoring/ISymbolizer.cs | 11 + .../NoUsingDeclaration/Analyzer.cs | 274 ++++++++++++++++++ .../NoUsingDeclaration/CodeFixer.cs | 78 +++++ .../NoUsingDeclaration/Resources.Designer.cs | 99 +++++++ .../NoUsingDeclaration/Resources.resx | 132 +++++++++ .../Refactoring/SmaContextExtentions.cs | 23 ++ .../StyleChecker/Refactoring/TypeSymbols.cs | 15 + StyleChecker/StyleChecker/StyleChecker.csproj | 9 + doc/rules/NoUsingDeclaration.md | 243 ++++++++++++++++ 17 files changed, 1142 insertions(+), 2 deletions(-) create mode 100644 StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/AnalyzerTest.cs create mode 100644 StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/Code.cs create mode 100644 StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/CodeFix.cs create mode 100644 StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/Okay.cs rename StyleChecker/StyleChecker/Refactoring/{UnnecessaryUsing => }/Classes.cs (94%) create mode 100644 StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/Analyzer.cs create mode 100644 StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/CodeFixer.cs create mode 100644 StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/Resources.Designer.cs create mode 100644 StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/Resources.resx create mode 100644 doc/rules/NoUsingDeclaration.md diff --git a/StyleChecker/StyleChecker.Test/Framework/Projects.cs b/StyleChecker/StyleChecker.Test/Framework/Projects.cs index ac894b50..d2174008 100644 --- a/StyleChecker/StyleChecker.Test/Framework/Projects.cs +++ b/StyleChecker/StyleChecker.Test/Framework/Projects.cs @@ -5,6 +5,8 @@ namespace StyleChecker.Test.Framework; using System.Collections.Immutable; using System.IO; using System.Linq; +using System.Net; +using System.Net.Sockets; using Maroontress.Extensions; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; @@ -39,7 +41,13 @@ public static IEnumerable /* Microsoft.CodeAnalysis */ NewReference(), /* System.Runtime.Extensions */ - NewReference() + NewReference(), + /* System.Net.Sockets */ + NewReference(), + /* System.Private.Url */ + NewReference(), + /* System.Net.Primitives */ + NewReference(), ]; /// diff --git a/StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/AnalyzerTest.cs b/StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/AnalyzerTest.cs new file mode 100644 index 00000000..5ef965f9 --- /dev/null +++ b/StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/AnalyzerTest.cs @@ -0,0 +1,33 @@ +namespace StyleChecker.Test.Refactoring.NoUsingDeclaration; + +using Microsoft.VisualStudio.TestTools.UnitTesting; +using StyleChecker.Refactoring.NoUsingDeclaration; +using StyleChecker.Test.Framework; + +[TestClass] +public sealed class AnalyzerTest : CodeFixVerifier +{ + public AnalyzerTest() + : base(new Analyzer(), new CodeFixer()) + { + } + + [TestMethod] + public void Okay() + => VerifyDiagnostic(ReadText("Okay"), Atmosphere.Default); + + [TestMethod] + public void Code() + { + var code = ReadText("Code"); + var fix = ReadText("CodeFix"); + static Result Expected(Belief b) + { + return b.ToResult( + Analyzer.DiagnosticId, + $"Insert 'using' before '{b.Message}'."); + } + + VerifyDiagnosticAndFix(code, Atmosphere.Default, Expected, fix); + } +} diff --git a/StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/Code.cs b/StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/Code.cs new file mode 100644 index 00000000..dcdc9934 --- /dev/null +++ b/StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/Code.cs @@ -0,0 +1,47 @@ +#nullable enable +namespace StyleChecker.Test.Refactoring.NoUsingDeclaration; + +using System; +using System.IO; + +public sealed class Code +{ + public static void Main() + { + var i = new StreamReader("file.txt"); + //@ ^var + StreamReader j = new("file.txt"); + //@ ^StreamReader + } + + public static void ExplicitTypeDeclarationsWithMultipleDeclarators() + { + StreamReader i1 = new StreamReader("1.txt"), i2 = new StreamReader("2.txt"); + //@ ^StreamReader + StreamReader j1 = new StreamReader("1.txt"), j2 = new("2.txt"); + //@ ^StreamReader + StreamReader k1 = new("1.txt"), k2 = new StreamReader("2.txt"); + //@ ^StreamReader + } + + public static void Using() + { + // StringReader 'i' does not need to be disposed + StringReader i = new("hello"); + // IDisposable 'j' should be disposed + IDisposable j = new StringReader("world"); + //@ ^IDisposable + } + + public static void Nullable() + { + StreamReader? k = new StreamReader("file.txt"); + //@ ^StreamReader? + } + + public static void KeepTrivia() + { + /*foo*/ var i = new StreamReader("file.txt"); // bar + //@ ^var + } +} diff --git a/StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/CodeFix.cs b/StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/CodeFix.cs new file mode 100644 index 00000000..f1ca60bc --- /dev/null +++ b/StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/CodeFix.cs @@ -0,0 +1,40 @@ +#nullable enable +namespace StyleChecker.Test.Refactoring.NoUsingDeclaration; + +using System; +using System.IO; + +public sealed class Code +{ + public static void Main() + { + using var i = new StreamReader("file.txt"); + using StreamReader j = new("file.txt"); + } + + public static void ExplicitTypeDeclarationsWithMultipleDeclarators() + { + using StreamReader i1 = new StreamReader("1.txt"), i2 = new StreamReader("2.txt"); + using StreamReader j1 = new StreamReader("1.txt"), j2 = new("2.txt"); + using StreamReader k1 = new("1.txt"), k2 = new StreamReader("2.txt"); + } + + public static void Using() + { + // StringReader 'i' does not need to be disposed + StringReader i = new("hello"); + // IDisposable 'j' should be disposed + using IDisposable j = new StringReader("world"); + } + + public static void Nullable() + { + using StreamReader? k = new StreamReader("file.txt"); + } + + public static void KeepTrivia() + { + /*foo*/ + using var i = new StreamReader("file.txt"); // bar + } +} diff --git a/StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/Okay.cs b/StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/Okay.cs new file mode 100644 index 00000000..d4ac3d63 --- /dev/null +++ b/StyleChecker/StyleChecker.Test/Refactoring/NoUsingDeclaration/Okay.cs @@ -0,0 +1,113 @@ +#nullable enable +namespace StyleChecker.Test.Refactoring.NoUsingDeclaration; + +using System; +using System.IO; +using System.Net.Sockets; + +public sealed class Okay +{ + private StreamReader? streamReader; + + private TextWriter? SharedWriter { get; set; } + + public void AssignedToFieldOrProperty() + { + // Replacing 'var' with 'using var' makes no sense. + var reader = new StreamReader("input.txt"); + streamReader = reader; + var writer = new StreamWriter("output.txt"); + SharedWriter = writer; + } + + public static BufferedStream UsedAsAParameter() + { + // Replacing 'var' with 'using var' makes no sense. + var clientSocket = new Socket( + AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); + var b = clientSocket.Connected; + var netStream = new NetworkStream(clientSocket, true); + var bufStream = new BufferedStream(netStream, 1024); + return bufStream; + } + + // A factory method that returns a new IDisposable object. + public static Socket? Returned(Uri uri, int port) + { + // Replacing 'var' with 'using var' makes no sense. + var socket = new Socket(SocketType.Stream, ProtocolType.Tcp); + try + { + socket.Connect(uri.Host, port); + return socket; + } + catch (Exception) + { + socket.Dispose(); + } + return null; + } + + public static void Reassigned() + { + // Replacing 'var' with 'using var' causes an error CS1656. + var i = new StreamReader("file.txt"); + Console.WriteLine(i.ReadLine()); + i = new StreamReader("another.txt"); + Console.WriteLine(i.ReadLine()); + } + + public static Action Captured() + { + var i = new StreamWriter("file.txt"); + return () => + { + i.WriteLine("hello"); + }; + } + + public static void Main() + { + using var i = new StreamReader("file.txt"); + // StringReader does not need to be disposed. + var j = new StringReader("hello"); + } + + public static void ExplicitTypeDeclarations() + { + using StreamReader i = new("file.txt"); + using IDisposable j = new StringReader("world"); + } + + public static void Nullable() + { + using StreamReader? k = new StreamReader("file.txt"); + } + + public static void NotNewOperator() + { + var out1 = Console.Out; + TextWriter out2 = Console.Out; + var file1 = NewStreamReader("file.txt"); + StreamReader? file2 = NewStreamReader("file.txt"); + } + + private static StreamReader? NewStreamReader(string path) + { + try + { + return new StreamReader(path); + } + catch (IOException) + { + return null; + } + } + + private static void Loophole() + { + var (r1, r2) = (new StreamReader("1"), new StreamReader("2")); + TextReader[] array = [new StreamReader("1")]; + var anotherArray = new[] { new StreamReader("1") }; + } +} diff --git a/StyleChecker/StyleChecker.Test/StyleChecker.Test.csproj b/StyleChecker/StyleChecker.Test/StyleChecker.Test.csproj index e3052fb5..5038b39e 100644 --- a/StyleChecker/StyleChecker.Test/StyleChecker.Test.csproj +++ b/StyleChecker/StyleChecker.Test/StyleChecker.Test.csproj @@ -45,6 +45,9 @@ + + + @@ -255,6 +258,15 @@ PreserveNewest + + PreserveNewest + + + PreserveNewest + + + PreserveNewest + PreserveNewest diff --git a/StyleChecker/StyleChecker/.editorconfig b/StyleChecker/StyleChecker/.editorconfig index 2d90b449..e2e907e0 100644 --- a/StyleChecker/StyleChecker/.editorconfig +++ b/StyleChecker/StyleChecker/.editorconfig @@ -29,3 +29,6 @@ dotnet_diagnostic.SA1013.severity = none # SA1012: Opening braces should be spaced correctly dotnet_diagnostic.SA1012.severity = none + +# SA1008: Opening parenthesis should be spaced correctly +dotnet_diagnostic.SA1008.severity = none diff --git a/StyleChecker/StyleChecker/Refactoring/UnnecessaryUsing/Classes.cs b/StyleChecker/StyleChecker/Refactoring/Classes.cs similarity index 94% rename from StyleChecker/StyleChecker/Refactoring/UnnecessaryUsing/Classes.cs rename to StyleChecker/StyleChecker/Refactoring/Classes.cs index b3f62683..7feb5681 100644 --- a/StyleChecker/StyleChecker/Refactoring/UnnecessaryUsing/Classes.cs +++ b/StyleChecker/StyleChecker/Refactoring/Classes.cs @@ -1,4 +1,4 @@ -namespace StyleChecker.Refactoring.UnnecessaryUsing; +namespace StyleChecker.Refactoring; using System.Collections.Immutable; using System.IO; diff --git a/StyleChecker/StyleChecker/Refactoring/ISymbolizer.cs b/StyleChecker/StyleChecker/Refactoring/ISymbolizer.cs index 75fa1bc8..76705d7b 100644 --- a/StyleChecker/StyleChecker/Refactoring/ISymbolizer.cs +++ b/StyleChecker/StyleChecker/Refactoring/ISymbolizer.cs @@ -283,4 +283,15 @@ public interface ISymbolizer /// The symbol representing the query range variable. /// IRangeVariableSymbol? ToSymbol(QueryContinuationSyntax node); + + /// + /// Gets the symbol corresponding to the specified variable declarator. + /// + /// + /// The variable declarator. + /// + /// + /// The symbol representing the variable. + /// + ISymbol? ToSymbol(VariableDeclaratorSyntax node); } diff --git a/StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/Analyzer.cs b/StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/Analyzer.cs new file mode 100644 index 00000000..64aa6c0d --- /dev/null +++ b/StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/Analyzer.cs @@ -0,0 +1,274 @@ +namespace StyleChecker.Refactoring.NoUsingDeclaration; + +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.FindSymbols; +using Microsoft.CodeAnalysis.Operations; +using R = Resources; + +/// +/// NoUsingDeclaration analyzer. +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class Analyzer : AbstractAnalyzer +{ + /// + /// The ID of this analyzer. + /// + public const string DiagnosticId = "NoUsingDeclaration"; + + private const string Category = Categories.Refactoring; + private static readonly DiagnosticDescriptor Rule = NewRule(); + + /// + public override ImmutableArray + SupportedDiagnostics => [Rule]; + + private static Func SymbolEquals { get; } + = SymbolEqualityComparer.Default.Equals; + + /// + private protected override void Register(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.RegisterSemanticModelAction(AnalyzeModel); + } + + private static DiagnosticDescriptor NewRule() + { + var localize = Localizers.Of(R.ResourceManager); + return new DiagnosticDescriptor( + DiagnosticId, + localize(nameof(R.Title)), + localize(nameof(R.MessageFormat)), + Category, + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: localize(nameof(R.Description)), + helpLinkUri: HelpLink.ToUri(DiagnosticId)); + } + + private static void AnalyzeModel(SemanticModelAnalysisContext context) + { + var token = context.CancellationToken; + var model = context.SemanticModel; + var toImplicitTypeDiagnostics = NewDiagnosticsFactory(context); + var root = model.SyntaxTree + .GetCompilationUnitRoot(token); + var diagnostics = root.DescendantNodes() + .OfType() + .SelectMany(toImplicitTypeDiagnostics) + .ToList(); + foreach (var d in diagnostics) + { + context.ReportDiagnostic(d); + } + } + + private static Func> + NewDiagnosticsFactory(SemanticModelAnalysisContext context) + { + static bool IsTypeDisposable(ITypeSymbol s) + => s.SpecialType is SpecialType.System_IDisposable; + + static bool DoesImplementDisposable(ITypeSymbol s) + { + var allInterfaces = s.AllInterfaces; + return allInterfaces.Length > 0 + && allInterfaces.Any(IsTypeDisposable); + } + + static bool IsTrulyDisposableSymbol(ITypeSymbol s) + => (s.TypeKind is TypeKind.Interface) + ? IsTypeDisposable(s) + || DoesImplementDisposable(s) + : DoesImplementDisposable(s) + && !Classes.DisposesNothing( + TypeSymbols.GetFullNameWithoutNullability(s)); + + static bool IsTrulyDisposable(TypeInfo typeInfo) + => typeInfo.Type is {} type + && IsTrulyDisposableSymbol(type); + + static IdentifierNameSyntax? GetTypeIdentifier(TypeSyntax s) + { + return s is NullableTypeSyntax nullableType + ? GetTypeIdentifier(nullableType.ElementType) + : s is IdentifierNameSyntax identifier + ? identifier + : null; + } + + static bool IsNewOperatorUsed(ExpressionSyntax s) + => s is ObjectCreationExpressionSyntax + || s is ImplicitObjectCreationExpressionSyntax; + + static ExpressionSyntax? ToNewExpr(VariableDeclaratorSyntax s) + => ((s.Initializer?.Value is { } value) + && IsNewOperatorUsed(value)) + ? value + : null; + + static bool CanUseUsingPotentially( + IdentifierNameSyntax s, + SeparatedSyntaxList variables, + Func toTypeInfo) + => s.Identifier.Text is not "var" + /* Explicit declaration */ + ? !variables.All(i => ToNewExpr(i) is not null) + || !IsTrulyDisposable(toTypeInfo(s)) + /* Implicit declaaation */ + : variables.Count < 1 + || ToNewExpr(variables[0]) is not {} value + || !IsTrulyDisposable(toTypeInfo(value)); + + return s => + { + var toTypeInfo = context.GetTypeInfoSupplier(); + var declaration = s.Declaration; + var variables = declaration.Variables; + var id = declaration.Type; + if (s.UsingKeyword != default + || GetTypeIdentifier(id) is not {} typeId + || CanUseUsingPotentially(typeId, variables, toTypeInfo) + || IsDeclarationSingleAndToBeReturned(context, s) + || IsReassigned(context, s) + || UsedAsParameterOrAssignedToAny(context, s)) + { + return []; + } + var location = id.GetLocation(); + var d = Diagnostic.Create(Rule, location, id); + return [d]; + }; + } + + private static IEnumerable> ToBlockSuppliers(SyntaxNode i) + => i is BaseMethodDeclarationSyntax method + ? [() => method.Body] + : i is LocalFunctionStatementSyntax localFunction + ? [() => localFunction.Body] + : []; + + private static BlockSyntax? ToContainingBody(LocalDeclarationStatementSyntax s) + => s.Ancestors() + .SelectMany(ToBlockSuppliers) + .FirstOrDefault() is not {} bodyProvider + ? null + : bodyProvider(); + + private static bool IsDeclarationSingleAndToBeReturned( + SemanticModelAnalysisContext context, + LocalDeclarationStatementSyntax s) + { + var model = context.SemanticModel; + var cancellationToken = context.CancellationToken; + var symbolizer = context.GetSymbolizer(); + + bool DoesReturnStatementReturnSymbol( + ReturnStatementSyntax r, ISymbol symbol) + => r.Expression is {} expr + && model.GetSymbolInfo(expr, cancellationToken) + .Symbol is {} s + && SymbolEquals(s, symbol); + + var declaration = s.Declaration; + var variables = declaration.Variables; + return variables.Count is 1 + && symbolizer.ToSymbol(variables[0]) is {} symbol + && ToContainingBody(s) is {} body + && model.AnalyzeControlFlow(body) is {} controlFlow + && controlFlow.Succeeded + && controlFlow.ReturnStatements + .OfType() + .Any(s => DoesReturnStatementReturnSymbol(s, symbol)); + } + + private static bool IsReassigned( + SemanticModelAnalysisContext context, + LocalDeclarationStatementSyntax s) + { + var model = context.SemanticModel; + var cancellationToken = context.CancellationToken; + var symbolizer = context.GetSymbolizer(); + var declaration = s.Declaration; + var variables = declaration.Variables; + return ToContainingBody(s) is {} body + && model.AnalyzeDataFlow(s) is {} dataFlow + && dataFlow.Succeeded + && variables.Count > 0 + && variables.Any(v => symbolizer.ToSymbol(v) is {} symbol + && ContainsSymbol( + dataFlow.WrittenOutside.Concat(dataFlow.Captured), + symbol)); + } + + private static bool ContainsSymbol(IEnumerable all, ISymbol s) + => all.Any(i => SymbolEquals(i, s)); + + private static bool IsUsedAs(IOperation o) + { + var parent = o.Parent; + var target = parent is IConversionOperation + /* for implicit conversions */ + ? parent.Parent + : parent; + return target is T; + } + + private static bool IsUsedAsArgument(IOperation o) + => IsUsedAs(o); + + private static bool IsUsedAsElement(IOperation o) + => IsUsedAs(o); + + private static bool IsUsedAsAssignmentValue(IOperation o) + { + var parent = o.Parent; + return (parent is ISimpleAssignmentOperation a + && ReferenceEquals(a.Value, o)) + /* for implicit conversions */ + || (parent is IConversionOperation + && parent.Parent is ISimpleAssignmentOperation b + && ReferenceEquals(b.Value, parent)); + } + + private static bool UsedAsParameterOrAssignedToAny( + SemanticModelAnalysisContext context, + LocalDeclarationStatementSyntax s) + { + static bool Predicate(ILocalReferenceOperation o) + => IsUsedAsArgument(o) + || IsUsedAsElement(o) + || IsUsedAsAssignmentValue(o); + + return IsUsedAs(context, s, Predicate); + } + + private static bool IsUsedAs( + SemanticModelAnalysisContext context, + LocalDeclarationStatementSyntax s, + Func isUsed) + { + var model = context.SemanticModel; + var cancellationToken = context.CancellationToken; + var symbolizer = context.GetSymbolizer(); + var declaration = s.Declaration; + var variables = declaration.Variables; + return ToContainingBody(s) is {} body + && variables.Count > 0 + && model.GetOperation(body, cancellationToken) is {} bodyOperation + && variables.Any(v => symbolizer.ToSymbol(v) is {} symbol + && bodyOperation.DescendantsAndSelf() + .OfType() + .Where(o => SymbolEquals(o.Local, symbol)) + .Any(isUsed)); + } +} diff --git a/StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/CodeFixer.cs b/StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/CodeFixer.cs new file mode 100644 index 00000000..0f5b7385 --- /dev/null +++ b/StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/CodeFixer.cs @@ -0,0 +1,78 @@ +namespace StyleChecker.Refactoring.NoUsingDeclaration; + +using System; +using System.Collections.Immutable; +using System.Composition; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Formatting; +using R = Resources; + +/// +/// NoUsingDeclaration CodeFix provider. +/// +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(CodeFixer))] +[Shared] +public sealed class CodeFixer : AbstractCodeFixProvider +{ + /// + public override ImmutableArray FixableDiagnosticIds + => [Analyzer.DiagnosticId]; + + /// + public override FixAllProvider GetFixAllProvider() + => WellKnownFixAllProviders.BatchFixer; + + /// + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + string FixTitle(string key) + { + var localize = Localizers.Of(R.ResourceManager); + return localize(key).ToString(CompilerCulture); + } + + var document = context.Document; + if (await document.GetSyntaxRootAsync(context.CancellationToken) + .ConfigureAwait(false) is not {} root) + { + return; + } + var diagnostic = context.Diagnostics[0]; + var span = diagnostic.Location.SourceSpan; + if (root.FindNodeOfType(span) is not {} typeNode + || typeNode.Parent is not VariableDeclarationSyntax declNode + || declNode.Parent is not LocalDeclarationStatementSyntax node + || node.UsingKeyword != default) + { + return; + } + var title = FixTitle(R.FixTitle); + var action = CodeAction.Create( + title: title, + createChangedDocument: + c => Replace(document, root, node, AddUsing), + equivalenceKey: title); + context.RegisterCodeFix(action, diagnostic); + } + + private static SyntaxNode AddUsing(LocalDeclarationStatementSyntax node) + => node.WithoutLeadingTrivia() + .WithUsingKeyword(SyntaxFactory.Token(SyntaxKind.UsingKeyword)) + .WithLeadingTrivia(node.GetLeadingTrivia()) + .WithAdditionalAnnotations(Formatter.Annotation); + + private static Task Replace( + Document document, + SyntaxNode root, + LocalDeclarationStatementSyntax node, + Func replacer) + { + var newRoot = root.ReplaceNode(node, replacer(node)); + return Task.FromResult(document.WithSyntaxRoot(newRoot)); + } +} diff --git a/StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/Resources.Designer.cs b/StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/Resources.Designer.cs new file mode 100644 index 00000000..7c241d41 --- /dev/null +++ b/StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/Resources.Designer.cs @@ -0,0 +1,99 @@ +//------------------------------------------------------------------------------ +// +// This code was generated by a tool. +// Runtime Version:4.0.30319.42000 +// +// Changes to this file may cause incorrect behavior and will be lost if +// the code is regenerated. +// +//------------------------------------------------------------------------------ + +namespace StyleChecker.Refactoring.NoUsingDeclaration { + using System; + + + /// + /// A strongly-typed resource class, for looking up localized strings, etc. + /// + // This class was auto-generated by the StronglyTypedResourceBuilder + // class via a tool like ResGen or Visual Studio. + // To add or remove a member, edit your .ResX file then rerun ResGen + // with the /str option, or rebuild your VS project. + [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "17.0.0.0")] + [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] + [global::System.Runtime.CompilerServices.CompilerGeneratedAttribute()] + internal class Resources { + + private static global::System.Resources.ResourceManager resourceMan; + + private static global::System.Globalization.CultureInfo resourceCulture; + + [global::System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("Microsoft.Performance", "CA1811:AvoidUncalledPrivateCode")] + internal Resources() { + } + + /// + /// Returns the cached ResourceManager instance used by this class. + /// + [global::System.ComponentModel.EditorBrowsableAttribute(global::System.ComponentModel.EditorBrowsableState.Advanced)] + internal static global::System.Resources.ResourceManager ResourceManager { + get { + if (object.ReferenceEquals(resourceMan, null)) { + global::System.Resources.ResourceManager temp = new global::System.Resources.ResourceManager("StyleChecker.Refactoring.NoUsingDeclaration.Resources", typeof(Resources).Assembly); + resourceMan = temp; + } + return resourceMan; + } + } + + /// + /// Overrides the current thread's CurrentUICulture property for all + /// resource lookups using this strongly typed resource class. + /// + [global::System.ComponentModel.EditorBrowsableAttribute(global::System.ComponentModel.EditorBrowsableState.Advanced)] + internal static global::System.Globalization.CultureInfo Culture { + get { + return resourceCulture; + } + set { + resourceCulture = value; + } + } + + /// + /// Looks up a localized string similar to The local variable can be declared with the 'using' keyword when its type is an IDisposable.. + /// + internal static string Description { + get { + return ResourceManager.GetString("Description", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Use using declaration. + /// + internal static string FixTitle { + get { + return ResourceManager.GetString("FixTitle", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Insert 'using' before '{0}'.. + /// + internal static string MessageFormat { + get { + return ResourceManager.GetString("MessageFormat", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to The local variable can be declared with 'using' keyword.. + /// + internal static string Title { + get { + return ResourceManager.GetString("Title", resourceCulture); + } + } + } +} diff --git a/StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/Resources.resx b/StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/Resources.resx new file mode 100644 index 00000000..ce605143 --- /dev/null +++ b/StyleChecker/StyleChecker/Refactoring/NoUsingDeclaration/Resources.resx @@ -0,0 +1,132 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + text/microsoft-resx + + + 2.0 + + + System.Resources.ResXResourceReader, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + + System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + + The local variable can be declared with the 'using' keyword when its type is an IDisposable. + + + Use using declaration + + + Insert 'using' before '{0}'. + + + The local variable can be declared with 'using' keyword. + + \ No newline at end of file diff --git a/StyleChecker/StyleChecker/Refactoring/SmaContextExtentions.cs b/StyleChecker/StyleChecker/Refactoring/SmaContextExtentions.cs index 93976ed2..b347544d 100644 --- a/StyleChecker/StyleChecker/Refactoring/SmaContextExtentions.cs +++ b/StyleChecker/StyleChecker/Refactoring/SmaContextExtentions.cs @@ -32,6 +32,26 @@ public static class SmaContextExtentions return n => model.GetOperation(n, cancellationToken); } + /// + /// Gets the function that takes a and returns the + /// corresponding to the node, with the specified + /// context. + /// + /// + /// The semantic model analysis context. + /// + /// + /// The function that takes a and returns the corresponding to the node. + /// + public static Func + GetTypeInfoSupplier(this SemanticModelAnalysisContext context) + { + var cancellationToken = context.CancellationToken; + var model = context.SemanticModel; + return e => model.GetTypeInfo(e, cancellationToken); + } + /// /// Gets the root of the syntax tree. /// @@ -143,5 +163,8 @@ private class SymbolizerImpl(SemanticModelAnalysisContext context) public IRangeVariableSymbol? ToSymbol(QueryContinuationSyntax node) => Model.GetDeclaredSymbol(node, CancellationToken); + + public ISymbol? ToSymbol(VariableDeclaratorSyntax node) + => Model.GetDeclaredSymbol(node, CancellationToken); } } diff --git a/StyleChecker/StyleChecker/Refactoring/TypeSymbols.cs b/StyleChecker/StyleChecker/Refactoring/TypeSymbols.cs index 5b3440ec..75e00bef 100644 --- a/StyleChecker/StyleChecker/Refactoring/TypeSymbols.cs +++ b/StyleChecker/StyleChecker/Refactoring/TypeSymbols.cs @@ -23,4 +23,19 @@ public static string GetFullName(ITypeSymbol typeSymbol) ? GetFullName(arrayTypeSymbol.ElementType) + "[]" : typeSymbol.ContainingNamespace + "." + typeSymbol.Name; } + + /// + /// Gets the Fully Qualified Type Name (FQTN) of the specified type symbol + /// without nullability. + /// + /// + /// The type symbol. + /// + /// + /// The FQTN of the specified type symbol without nullability, with array + /// notation if it is an array type. + /// + public static string GetFullNameWithoutNullability(ITypeSymbol typeSymbol) + => GetFullName( + typeSymbol.WithNullableAnnotation(NullableAnnotation.NotAnnotated)); } diff --git a/StyleChecker/StyleChecker/StyleChecker.csproj b/StyleChecker/StyleChecker/StyleChecker.csproj index eab62113..b6341c77 100644 --- a/StyleChecker/StyleChecker/StyleChecker.csproj +++ b/StyleChecker/StyleChecker/StyleChecker.csproj @@ -122,6 +122,11 @@ True True + + Resources.resx + True + True + Resources.resx True @@ -263,6 +268,10 @@ Resources.Designer.cs ResXFileCodeGenerator + + Resources.Designer.cs + ResXFileCodeGenerator + Resources.Designer.cs ResXFileCodeGenerator diff --git a/doc/rules/NoUsingDeclaration.md b/doc/rules/NoUsingDeclaration.md new file mode 100644 index 00000000..19fca762 --- /dev/null +++ b/doc/rules/NoUsingDeclaration.md @@ -0,0 +1,243 @@ + +
+ +# NoUsingDeclaration + +
+ +![NoUsingDeclaration][fig-NoUsingDeclaration] + +
+ +## Summary + +Use Using Declarations to declare a local variable whenever possible. + +## Default severity + +Info + +## Description + +When you declare a local variable which is an +[`IDisposable`][system.idisposable] object, you should use Using Declarations +\[[1](#ref1)\] whenever possible as follows: + +```csharp +// For implicitly-typed variables +using var reader = new StreamReader("file.txt"); + +// For explicitly-typed variables +using StreamReader i = new("file.txt"); + +// Multiple declarators are allowed in explicit type declarations +using StreamReader j = new("1.txt"), k = new("2.txt"); +``` + +Using Declaration is preferred to Using Statement or the `try`-`finally` idiom +because it is easier to describe RAII in C#. + +### Remarks + +This analyzer only triggers diagnostics if the local variable is declared and +initialized with the `new` operator. It ignores the declaration of the local +variable that is initialized with an `IDisposable` instance any function or +property returns, as follows: + +```csharp +// OK +var out = System.Console.Out; + +static TextReader NewStreamReader() => new StreamReader("file.txt"); + +// OK +var reader = NewStreamReader(); +``` + +Initialization with an `IDisposable` instance created with an operator other +than the `new` operator is also not covered as follows: + +```csharp +// OK (but you should prevent resource leaks) +var reader = (inputFile is null) + ? new StringReader(defaultText) + : new StreamReader(inputFile); +``` + +### Cases where no diagnosis is issued + +This analyzer should not raise a diagnostic when a factory method creates and +returns an `IDisposable` object as follows: + +```csharp +public Socket? NewTcpSocket(Uri uri, int port) +{ + // XXX (you cannot use Using Declaration) + var socket = new Socket(SocketType.Stream, ProtocolType.Tcp); + try + { + socket.Connect(uri.Host, port); + return socket; + } + catch (Exception) + { + socket.Dispose(); + } + return null; +} +``` + +Similarly, this analyzer should not raise a diagnostic if you instantiate an +`IDisposable` object and assign it to a field or property, or capture it as +follows: + +```csharp +private StreamReader? streamReader; + +private StreamWriter? SharedWriter { get; set; } + +public void PrepareStream() +{ + // XXX (you cannot use Using Declaration) + var reader = new StreamReader("input.txt"); + streamReader = reader; + var writer = new StreamWriter("output.txt"); + SharedWriter = writer; + ⋮ +} + +public static Action WriteHelloAction() +{ + // XXX (you cannot use Using Declaration) + var writer = new StreamWriter("file.txt"); + return () => + { + // How do you dispose of it!? + writer.WriteLine("hello"); + }; +} +``` + +This analyzer should still not raise a diagnostic in the following complex case +where you instantiate an `IDisposable` object wrapped in a decorator pattern: + +```csharp +public BufferedStream NewClientStream(…) +{ + // XXX (you cannot use Using Declaration) + var clientSocket = new Socket( + AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); + clientSocket.Connect(…); + var netStream = new NetworkStream(clientSocket, true); + var bufStream = new BufferedStream(netStream, streamBufferSize); + return bufStream; +} +``` + +It also does not issue a diagnostic when variables are reassigned as follows: + +```csharp +// XXX (Using Declaration causes an error CS1656 at the line /*💀*/.) +var i = new StreamReader("file.txt"); +Console.WriteLine(i.ReadLine()); +/*💀*/ i = new StreamReader("another.txt"); +Console.WriteLine(i.ReadLine()); +``` + +In summary, a diagnostic will not be issued if the variable representing the +created instance is: + +- used as a parameter of a method or constructor +- on the right side of the assignment expression +- captured +- returned +- reassigned + +Therefore, you should be aware that resource leaks can occur even though this +analyzer does not issue any diagnostics. + +### Classes whose dispose method does nothing + +The local variables whose type is one of the following are not covered: + +- [`System.IO.StringReader`][system.io.stringreader] +- [`System.IO.StringWriter`][system.io.stringwriter] +- [`System.IO.MemoryStream`][system.io.memorystream] +- [`System.IO.UnmanagedMemoryStream`][system.io.unmanagedmemorystream] +- [`System.IO.UnmanagedMemoryAccessor`][system.io.unmanagedmemoryaccessor] + +> See also [UnnecessaryUsing][] analyzer. + +For example: + +```csharp +// OK +var reader = new StringReader("hello"); + +// OK +StringReader i = new("hello"); +``` + +However, even if the concrete class of the instance is one of those above, it is +covered if the type of the variable is not so, as follows: + +```csharp +// NG +TextReader i = new StringReader("hello"); +``` + +## Code fix + +The code fix provides an option inserting `using` keyword before `var` or the +type name. + +## Example + +### Diagnostic + +```csharp +public sealed class Code +{ + public static void Main() + { + var i = new StreamReader("file.txt"); + StreamReader j = new("file.txt"); + } +} +``` + +### Code fix + +```csharp +public sealed class Code +{ + public static void Main() + { + using var i = new StreamReader("file.txt"); + using StreamReader j = new("file.txt"); + } +} +``` + +## References + + [1] +[Microsoft, _"pattern-based using" and "using declarations"_][dotnet-csharp-using-declaration] + +[dotnet-csharp-using-declaration]: + https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-8.0/using +[system.io.memorystream]: + https://docs.microsoft.com/en-us/dotnet/api/system.io.memorystream?view=netstandard-1.0 +[system.io.unmanagedmemorystream]: + https://docs.microsoft.com/en-us/dotnet/api/system.io.unmanagedmemorystream?view=netstandard-2.0 +[system.io.unmanagedmemoryaccessor]: + https://docs.microsoft.com/en-us/dotnet/api/system.io.unmanagedmemoryaccessor?view=netstandard-2.0 +[system.io.stringreader]: + https://docs.microsoft.com/en-us/dotnet/api/system.io.stringreader?view=netstandard-1.0 +[system.io.stringwriter]: + https://docs.microsoft.com/en-us/dotnet/api/system.io.stringwriter?view=netstandard-1.0 +[system.idisposable]: + https://docs.microsoft.com/en-us/dotnet/api/system.idisposable?view=netstandard-1.0 +[fig-NoUsingDeclaration]: + https://maroontress.github.io/StyleChecker/images/TurnOfUsingDeclaration.png +[UnnecessaryUsing]: UnnecessaryUsing.md