From 8f459c2460438930b89399c8fe4663ca8b6e6919 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Thu, 4 Oct 2012 20:29:08 +0200 Subject: [PATCH 01/29] Ensure IVariable.ConstantValue has the correct type when a local constant declaration involves an implicit conversion E.g. "const int MAXSIZE = ushort.MaxValue;" --- .../Resolver/ResolveVisitor.cs | 4 ++- .../CSharp/Resolver/CastTests.cs | 31 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/ICSharpCode.NRefactory.CSharp/Resolver/ResolveVisitor.cs b/ICSharpCode.NRefactory.CSharp/Resolver/ResolveVisitor.cs index 2a3ce9f8eb..eb727b4faa 100644 --- a/ICSharpCode.NRefactory.CSharp/Resolver/ResolveVisitor.cs +++ b/ICSharpCode.NRefactory.CSharp/Resolver/ResolveVisitor.cs @@ -2913,7 +2913,9 @@ namespace ICSharpCode.NRefactory.CSharp.Resolver foreach (VariableInitializer vi in variableDeclarationStatement.Variables) { IVariable v; if (isConst) { - v = MakeConstant(type, vi.NameToken, Resolve(vi.Initializer).ConstantValue); + ResolveResult rr = Resolve(vi.Initializer); + rr = resolver.ResolveCast(type, rr); + v = MakeConstant(type, vi.NameToken, rr.ConstantValue); } else { v = MakeVariable(type, vi.NameToken); } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/Resolver/CastTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/Resolver/CastTests.cs index 99f8be6f85..07d98aeb52 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/Resolver/CastTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/Resolver/CastTests.cs @@ -97,5 +97,36 @@ namespace ICSharpCode.NRefactory.CSharp.Resolver { AssertError(typeof(StringComparison), resolver.WithCheckForOverflow(true).ResolveCast(ResolveType(typeof(StringComparison)), MakeConstant(long.MaxValue))); } + + [Test] + public void ImplicitCastInConstant() + { + string program = @"using System; +class Test { + const int $MAXSIZE = ushort.MaxValue; +}"; + var rr = ResolveAtLocation(program); + IField field = (IField)rr.Member; + Assert.IsTrue(field.IsConst); + Assert.AreEqual("System.Int32", field.Type.FullName); + Assert.AreEqual(typeof(int), field.ConstantValue.GetType()); + Assert.AreEqual(ushort.MaxValue, (int)field.ConstantValue); + } + + [Test] + public void ImplicitCastInLocalConstant() + { + string program = @"using System; +class Test { + void M() { + const int $MAXSIZE = ushort.MaxValue; + } +}"; + var rr = ResolveAtLocation(program); + Assert.IsTrue(rr.Variable.IsConst); + Assert.AreEqual("System.Int32", rr.Variable.Type.FullName); + Assert.AreEqual(typeof(int), rr.Variable.ConstantValue.GetType()); + Assert.AreEqual(ushort.MaxValue, (int)rr.Variable.ConstantValue); + } } } From c3f46aadfb60f05b7e28c05e605c52f3710bf5cb Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Thu, 4 Oct 2012 20:41:05 +0200 Subject: [PATCH 02/29] Move some code actions from 'CodeActions' namespace to refactoring namespace, to be consistent with all other code actions. --- .../Refactoring/CodeActions/ConvertConditionalToIfAction.cs | 2 +- .../Refactoring/CodeActions/ConvertIfToConditionalAction.cs | 2 +- .../Refactoring/CodeActions/SortUsingsAction.cs | 2 +- .../InconsistentNamingIssue/NamingConventionService.cs | 4 ++-- .../CSharp/CodeActions/ConvertConditionalToIfTests.cs | 2 +- .../CSharp/CodeActions/ConvertIfToConditionalTests.cs | 2 +- .../CSharp/CodeActions/SortUsingsTests.cs | 2 +- 7 files changed, 8 insertions(+), 8 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/ConvertConditionalToIfAction.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/ConvertConditionalToIfAction.cs index f69f18b184..35a8435983 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/ConvertConditionalToIfAction.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/ConvertConditionalToIfAction.cs @@ -28,7 +28,7 @@ using System; using System.Collections.Generic; using System.Linq; -namespace ICSharpCode.NRefactory.CSharp.Refactoring.CodeActions +namespace ICSharpCode.NRefactory.CSharp.Refactoring { [ContextAction ("Convert '?:' to 'if'", Description = "Convert '?:' operator to 'if' statement.")] public class ConvertConditionalToIfAction : ICodeActionProvider diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/ConvertIfToConditionalAction.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/ConvertIfToConditionalAction.cs index 31633994b7..62f2e7feb1 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/ConvertIfToConditionalAction.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/ConvertIfToConditionalAction.cs @@ -28,7 +28,7 @@ using System; using System.Linq; using ICSharpCode.NRefactory.PatternMatching; -namespace ICSharpCode.NRefactory.CSharp.Refactoring.CodeActions +namespace ICSharpCode.NRefactory.CSharp.Refactoring { [ContextAction ("Convert 'if' to '?:'", Description = "Convert 'if' statement to '?:' operator.")] public class ConvertIfToConditionalAction : SpecializedCodeAction diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/SortUsingsAction.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/SortUsingsAction.cs index 6fa08d4e2a..12c15c765c 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/SortUsingsAction.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/SortUsingsAction.cs @@ -5,7 +5,7 @@ using ICSharpCode.NRefactory.Semantics; using ICSharpCode.NRefactory.TypeSystem; using ICSharpCode.NRefactory.TypeSystem.Implementation; -namespace ICSharpCode.NRefactory.CSharp.Refactoring.CodeActions +namespace ICSharpCode.NRefactory.CSharp.Refactoring { [ContextAction("Sort usings", Description = "Sorts usings by their origin and then alphabetically.")] public class SortUsingsAction: ICodeActionProvider diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/InconsistentNamingIssue/NamingConventionService.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/InconsistentNamingIssue/NamingConventionService.cs index 51d2cf5afc..4b1e1e8207 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/InconsistentNamingIssue/NamingConventionService.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/InconsistentNamingIssue/NamingConventionService.cs @@ -1,4 +1,4 @@ -// +// // NamingConventionService.cs // // Author: @@ -48,7 +48,7 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring continue; } if (!rule.IsValid(name)) { - IList suggestedNames; + IList suggestedNames; var msg = rule.GetErrorMessage(ctx, name, out suggestedNames); if (suggestedNames.Any ()) return suggestedNames [0]; diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/ConvertConditionalToIfTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/ConvertConditionalToIfTests.cs index ac075ddae3..a0c7dc0aca 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/ConvertConditionalToIfTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/ConvertConditionalToIfTests.cs @@ -24,7 +24,7 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN // THE SOFTWARE. -using ICSharpCode.NRefactory.CSharp.Refactoring.CodeActions; +using ICSharpCode.NRefactory.CSharp.Refactoring; using NUnit.Framework; namespace ICSharpCode.NRefactory.CSharp.CodeActions diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/ConvertIfToConditionalTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/ConvertIfToConditionalTests.cs index 993f087fbe..2423db2de6 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/ConvertIfToConditionalTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/ConvertIfToConditionalTests.cs @@ -25,7 +25,7 @@ // THE SOFTWARE. -using ICSharpCode.NRefactory.CSharp.Refactoring.CodeActions; +using ICSharpCode.NRefactory.CSharp.Refactoring; using NUnit.Framework; namespace ICSharpCode.NRefactory.CSharp.CodeActions diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/SortUsingsTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/SortUsingsTests.cs index 0a47561ab2..71af592bfd 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/SortUsingsTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/SortUsingsTests.cs @@ -1,5 +1,5 @@ using NUnit.Framework; -using ICSharpCode.NRefactory.CSharp.Refactoring.CodeActions; +using ICSharpCode.NRefactory.CSharp.Refactoring; namespace ICSharpCode.NRefactory.CSharp.CodeActions { From 0cdb73aad63b60e8199d9558fb7b55ed9ad48007 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Thu, 4 Oct 2012 20:41:30 +0200 Subject: [PATCH 03/29] Add setter for the refactoring context's service container. --- .../Refactoring/BaseRefactoringContext.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/BaseRefactoringContext.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/BaseRefactoringContext.cs index 6f4ccfc720..c7e0c68779 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/BaseRefactoringContext.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/BaseRefactoringContext.cs @@ -185,13 +185,14 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring } #region IServiceProvider implementation - readonly ServiceContainer services = new ServiceContainer(); + IServiceContainer services = new ServiceContainer(); /// /// Gets a service container used to associate services with this context. /// - public ServiceContainer Services { + public IServiceContainer Services { get { return services; } + protected set { services = value; } } /// From 4d75ecc36c3201586261745636790e43750d1a11 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Thu, 4 Oct 2012 20:41:54 +0200 Subject: [PATCH 04/29] Fix InvalidCastException in AccessToClosureIssue. --- .../AccessToClosureIssues/AccessToClosureIssue.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToClosureIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToClosureIssue.cs index 7ff3a99360..9691ae63e6 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToClosureIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToClosureIssue.cs @@ -115,8 +115,11 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring } else if (parent is OperatorDeclaration) { body = ((OperatorDeclaration)parent).Body; } - if (body != null) - CheckVariable (((LocalResolveResult)ctx.Resolve (parameterDeclaration)).Variable, body); + if (body != null) { + var lrr = ctx.Resolve (parameterDeclaration) as LocalResolveResult; + if (lrr != null) + CheckVariable (lrr.Variable, body); + } base.VisitParameterDeclaration (parameterDeclaration); } From eabc6dbfae12cf4381c24d5393dc41fde37b8820 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Thu, 4 Oct 2012 21:16:53 +0200 Subject: [PATCH 05/29] Don't warn on empty custom events. --- .../CodeIssues/ValueParameterUnusedIssue.cs | 5 +++- .../CodeIssues/ValueParameterUnusedTests.cs | 23 ++++++++++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ValueParameterUnusedIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ValueParameterUnusedIssue.cs index 8d0412c412..489f29b5f8 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ValueParameterUnusedIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ValueParameterUnusedIssue.cs @@ -62,8 +62,11 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring public override void VisitCustomEventDeclaration(CustomEventDeclaration eventDeclaration) { var addAccessor = eventDeclaration.AddAccessor; - FindIssuesInNode(addAccessor, addAccessor.Body, "add accessor"); var removeAccessor = eventDeclaration.RemoveAccessor; + // don't warn on empty custom events + if (addAccessor.Body.Statements.Count == 0 && removeAccessor.Body.Statements.Count == 0) + return; + FindIssuesInNode(addAccessor, addAccessor.Body, "add accessor"); FindIssuesInNode(removeAccessor, removeAccessor.Body, "remove accessor"); } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ValueParameterUnusedTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ValueParameterUnusedTests.cs index f1de52cf6d..a3327d82f6 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ValueParameterUnusedTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ValueParameterUnusedTests.cs @@ -76,9 +76,11 @@ namespace ICSharpCode.NRefactory.CSharp.CodeIssues var input = @"class A { delegate void TestEventHandler (); + TestEventHandler eventTested; event TestEventHandler EventTested { add { + eventTested += value; } remove { } @@ -86,7 +88,7 @@ namespace ICSharpCode.NRefactory.CSharp.CodeIssues }"; TestRefactoringContext context; var issues = GetIssues(new ValueParameterUnusedIssue(), input, out context); - Assert.AreEqual(2, issues.Count); + Assert.AreEqual(1, issues.Count); } [Test] @@ -153,6 +155,25 @@ namespace ICSharpCode.NRefactory.CSharp.CodeIssues throw new Exception(); } } +}"; + TestRefactoringContext context; + var issues = GetIssues(new ValueParameterUnusedIssue(), input, out context); + Assert.AreEqual(0, issues.Count); + } + + [Test] + public void DoesNotWarnOnEmptyCustomEvent() + { + // Empty custom events are often used when the event can never be raised + // by a class (but the event is required e.g. due to an interface). + var input = @"class A +{ + delegate void TestEventHandler (); + event TestEventHandler EventTested + { + add { } + remove { } + } }"; TestRefactoringContext context; var issues = GetIssues(new ValueParameterUnusedIssue(), input, out context); From 03f66e700fe6422bf643a5852c9cc05179e95fad Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Thu, 4 Oct 2012 21:25:27 +0200 Subject: [PATCH 06/29] CastExpressionOfIncompatibleTypeIssue: don't produce an issue when the source or target type is unknown. --- .../CastExpressionOfIncompatibleTypeIssue.cs | 2 ++ ...tExpressionOfIncompatibleTypeIssueTests.cs | 34 ++++++++++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/CastExpressionOfIncompatibleTypeIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/CastExpressionOfIncompatibleTypeIssue.cs index 0cd039c107..c81d39693a 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/CastExpressionOfIncompatibleTypeIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/CastExpressionOfIncompatibleTypeIssue.cs @@ -71,6 +71,8 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring void VisitTypeCastExpression (Expression expression, IType exprType, IType castToType) { + if (exprType.Kind == TypeKind.Unknown || castToType.Kind == TypeKind.Unknown) + return; var foundConversion = conversion.ExplicitConversion(exprType, castToType); if (foundConversion == Conversion.None) AddIssue (expression, ctx.TranslateString ("Type cast expression of incompatible type")); diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/CastExpressionOfIncompatibleTypeIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/CastExpressionOfIncompatibleTypeIssueTests.cs index be33b5ff15..290b346080 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/CastExpressionOfIncompatibleTypeIssueTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/CastExpressionOfIncompatibleTypeIssueTests.cs @@ -56,7 +56,39 @@ class TestClass void TestMethod () { var x1 = (int)123; - var x2 = (int)System.ConsoleKey.A; + var x2 = (short)123; + var x3 = (int)System.ConsoleKey.A; + } +}"; + Test (input, 0); + } + + [Test] + public void UnknownIdentifierDoesNotCauseIncompatibleCastIssue () + { + var input = @" +class TestClass +{ + void TestMethod () + { + var x1 = unknown as string; + var x2 = (string)unknown; + } +}"; + + Test (input, 0); + } + + [Test] + public void UnknownTargetTypeDoesNotCauseIncompatibleCastIssue () + { + var input = @" +class TestClass +{ + void TestMethod (int p) + { + var x1 = (unknown)p; + var x2 = p as unknown; } }"; From d4df4d9790f81cc9c588421ef01ab7200d7b7b68 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Thu, 4 Oct 2012 21:36:22 +0200 Subject: [PATCH 07/29] Fixed using implicit user-defined conversions as part of explicit conversions. --- .../Resolver/CSharpConversions.cs | 2 +- .../CSharp/Resolver/ExplicitConversionsTest.cs | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/ICSharpCode.NRefactory.CSharp/Resolver/CSharpConversions.cs b/ICSharpCode.NRefactory.CSharp/Resolver/CSharpConversions.cs index 8e6e92248f..e1ff416411 100644 --- a/ICSharpCode.NRefactory.CSharp/Resolver/CSharpConversions.cs +++ b/ICSharpCode.NRefactory.CSharp/Resolver/CSharpConversions.cs @@ -788,7 +788,7 @@ namespace ICSharpCode.NRefactory.CSharp.Resolver // Find the candidate operators: Predicate opFilter; if (isExplicit) - opFilter = m => m.IsStatic && m.IsOperator && m.Name == "op_Explicit" && m.Parameters.Count == 1; + opFilter = m => m.IsStatic && m.IsOperator && (m.Name == "op_Explicit" || m.Name == "op_Implicit") && m.Parameters.Count == 1; else opFilter = m => m.IsStatic && m.IsOperator && m.Name == "op_Implicit" && m.Parameters.Count == 1; diff --git a/ICSharpCode.NRefactory.Tests/CSharp/Resolver/ExplicitConversionsTest.cs b/ICSharpCode.NRefactory.Tests/CSharp/Resolver/ExplicitConversionsTest.cs index f8395cdabf..4d9bbd7fe0 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/Resolver/ExplicitConversionsTest.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/Resolver/ExplicitConversionsTest.cs @@ -490,6 +490,24 @@ class C { Assert.AreEqual("B", rr.Input.Type.Name); } + [Test] + public void ImplicitUserDefinedConversionFollowedByExplicitNumericConversion() + { + var rr = Resolve(@" + struct T { + public static implicit operator float(T t) { return 0; } + } + class Test { + void Run(T t) { + int x = $(int)t$; + } + }"); + Assert.IsTrue(rr.Conversion.IsValid); + Assert.IsTrue(rr.Conversion.IsUserDefined); + // even though the user-defined conversion is implicit, the combined conversion is explicit + Assert.IsTrue(rr.Conversion.IsExplicit); + } + [Test] [Ignore("Not implemented yet.")] public void BothDirectConversionAndBaseClassConversionAvailable() From 1a2689edcbd12f8c326b3fc6a284e3a36100fa4e Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Thu, 4 Oct 2012 21:51:36 +0200 Subject: [PATCH 08/29] CompareFloatWithEqualityOperatorIssue: use short name for System.Math if possible. --- .../Refactoring/BaseRefactoringContext.cs | 6 ++++++ .../CompareFloatWithEqualityOperatorIssue.cs | 14 +++++--------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/BaseRefactoringContext.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/BaseRefactoringContext.cs index c7e0c68779..8155ffe459 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/BaseRefactoringContext.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/BaseRefactoringContext.cs @@ -122,6 +122,12 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring { return resolver.GetConversion(expression, cancellationToken); } + + public TypeSystemAstBuilder CreateTypeSytemAstBuilder(AstNode node) + { + var csResolver = resolver.GetResolverStateBefore(node); + return new TypeSystemAstBuilder(csResolver); + } #endregion #region Code Analyzation diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/CompareFloatWithEqualityOperatorIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/CompareFloatWithEqualityOperatorIssue.cs index 08103da6fb..4c67c2ee9f 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/CompareFloatWithEqualityOperatorIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/CompareFloatWithEqualityOperatorIssue.cs @@ -80,12 +80,11 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring void AddIsNaNIssue(BinaryOperatorExpression binaryOperatorExpr, Expression argExpr, string floatType) { - if (ctx.Resolve (argExpr).Type.ReflectionName != "System.Single") + if (!ctx.Resolve(argExpr).Type.IsKnownType(KnownTypeCode.Single)) floatType = "double"; - AddIssue (binaryOperatorExpr, string.Format(ctx.TranslateString ("Use {0}.IsNan()"), floatType), + AddIssue(binaryOperatorExpr, string.Format(ctx.TranslateString ("Use {0}.IsNaN()"), floatType), script => { - Expression expr = new InvocationExpression (new MemberReferenceExpression ( - new TypeReferenceExpression (new PrimitiveType (floatType)), "IsNaN"), argExpr.Clone ()); + Expression expr = new PrimitiveType(floatType).Invoke("IsNaN", argExpr.Clone()); if (binaryOperatorExpr.Operator == BinaryOperatorType.InEquality) expr = new UnaryOperatorExpression (UnaryOperatorType.Not, expr); script.Replace (binaryOperatorExpr, expr); @@ -109,13 +108,10 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring AddIssue (binaryOperatorExpression, ctx.TranslateString ("Compare a difference with EPSILON"), script => { // Math.Abs(diff) op EPSILON + var builder = ctx.CreateTypeSytemAstBuilder(binaryOperatorExpression); var diff = new BinaryOperatorExpression (binaryOperatorExpression.Left.Clone (), BinaryOperatorType.Subtract, binaryOperatorExpression.Right.Clone ()); - var abs = new InvocationExpression ( - new MemberReferenceExpression ( - new TypeReferenceExpression (new SimpleType ("System.Math")), - "Abs"), - diff); + var abs = builder.ConvertType(new TopLevelTypeName("System", "Math")).Invoke("Abs", diff); var op = binaryOperatorExpression.Operator == BinaryOperatorType.Equality ? BinaryOperatorType.LessThan : BinaryOperatorType.GreaterThan; var epsilon = new IdentifierExpression ("EPSILON"); From 3eb4ea84f810c673c1a3fe2d10d6b1a7067ab465 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Thu, 4 Oct 2012 23:31:22 +0200 Subject: [PATCH 09/29] Fix NullReferenceException in IterateViaForeachAction. --- .../Refactoring/CodeActions/IterateViaForeachAction.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/IterateViaForeachAction.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/IterateViaForeachAction.cs index 3e5049a9fd..0e3fed4fe2 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/IterateViaForeachAction.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/IterateViaForeachAction.cs @@ -149,11 +149,10 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring { IType collectionType = null; foreach (var baseType in type.GetAllBaseTypes()) { - var baseTypeDefinition = baseType.GetDefinition(); - if (baseTypeDefinition.IsKnownType(KnownTypeCode.IEnumerableOfT)) { + if (baseType.IsKnownType(KnownTypeCode.IEnumerableOfT)) { collectionType = baseType; break; - } else if (baseTypeDefinition.IsKnownType(KnownTypeCode.IEnumerable)) { + } else if (baseType.IsKnownType(KnownTypeCode.IEnumerable)) { collectionType = baseType; // Don't break, continue in case type implements IEnumerable } From caa61c79cadbcc5b13ece0efe680437fed5873ba Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Thu, 4 Oct 2012 23:31:40 +0200 Subject: [PATCH 10/29] Add helper method for exporting the control flow graph to GraphViz. --- .../Analysis/ControlFlow.cs | 55 ++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/ICSharpCode.NRefactory.CSharp/Analysis/ControlFlow.cs b/ICSharpCode.NRefactory.CSharp/Analysis/ControlFlow.cs index 8990fb5d82..032aa64832 100644 --- a/ICSharpCode.NRefactory.CSharp/Analysis/ControlFlow.cs +++ b/ICSharpCode.NRefactory.CSharp/Analysis/ControlFlow.cs @@ -1,4 +1,4 @@ -// Copyright (c) AlphaSierraPapa for the SharpDevelop Team +// Copyright (c) AlphaSierraPapa for the SharpDevelop Team // // Permission is hereby granted, free of charge, to any person obtaining a copy of this // software and associated documentation files (the "Software"), to deal in the Software @@ -26,6 +26,7 @@ using ICSharpCode.NRefactory.CSharp.TypeSystem; using ICSharpCode.NRefactory.Semantics; using ICSharpCode.NRefactory.TypeSystem; using ICSharpCode.NRefactory.TypeSystem.Implementation; +using ICSharpCode.NRefactory.Utils; namespace ICSharpCode.NRefactory.CSharp.Analysis { @@ -750,5 +751,57 @@ namespace ICSharpCode.NRefactory.CSharp.Analysis return CreateConnectedEndNode(fixedStatement, bodyEnd); } } + + /// + /// Debugging helper that exports a control flow graph. + /// + public static GraphVizGraph ExportGraph(IList nodes) + { + GraphVizGraph g = new GraphVizGraph(); + GraphVizNode[] n = new GraphVizNode[nodes.Count]; + Dictionary dict = new Dictionary(); + for (int i = 0; i < n.Length; i++) { + dict.Add(nodes[i], i); + n[i] = new GraphVizNode(i); + string name = "#" + i + " = "; + switch (nodes[i].Type) { + case ControlFlowNodeType.StartNode: + case ControlFlowNodeType.BetweenStatements: + name += nodes[i].NextStatement.DebugToString(); + break; + case ControlFlowNodeType.EndNode: + name += "End of " + nodes[i].PreviousStatement.DebugToString(); + break; + case ControlFlowNodeType.LoopCondition: + name += "Condition in " + nodes[i].NextStatement.DebugToString(); + break; + default: + name += "?"; + break; + } + n[i].label = name; + g.AddNode(n[i]); + } + for (int i = 0; i < n.Length; i++) { + foreach (ControlFlowEdge edge in nodes[i].Outgoing) { + GraphVizEdge ge = new GraphVizEdge(i, dict[edge.To]); + if (edge.IsLeavingTryFinally) + ge.style = "dashed"; + switch (edge.Type) { + case ControlFlowEdgeType.ConditionTrue: + ge.color = "green"; + break; + case ControlFlowEdgeType.ConditionFalse: + ge.color = "red"; + break; + case ControlFlowEdgeType.Jump: + ge.color = "blue"; + break; + } + g.AddEdge(ge); + } + } + return g; + } } } From ad431543a3ebbfe8eb5105be0651b2035fa60cdf Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Thu, 4 Oct 2012 23:50:15 +0200 Subject: [PATCH 11/29] Fix NullReferenceException in IncorrectExceptionParameterOrderingIssue. --- .../CodeIssues/IncorrectExceptionParameterOrderingIssue.cs | 4 ++-- .../CodeIssues/IncorrectExceptionParameterOrderingTests.cs | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/IncorrectExceptionParameterOrderingIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/IncorrectExceptionParameterOrderingIssue.cs index dc7ab72d0f..73e5051f52 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/IncorrectExceptionParameterOrderingIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/IncorrectExceptionParameterOrderingIssue.cs @@ -63,8 +63,8 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring return; var firstParam = parameters.FirstOrNullObject() as PrimitiveExpression; var secondParam = parameters.LastOrNullObject() as PrimitiveExpression; - if (firstParam == null || firstParam.Value.GetType() != typeof(string) || - secondParam == null || firstParam.Value.GetType() != typeof(string)) + if (firstParam == null || !(firstParam.Value is string) || + secondParam == null || !(secondParam.Value is string)) return; var type = context.Resolve(objectCreateExpression.Type) as TypeResolveResult; if (type == null) diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/IncorrectExceptionParameterOrderingTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/IncorrectExceptionParameterOrderingTests.cs index 883df7b1d2..1a50b304ca 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/IncorrectExceptionParameterOrderingTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/IncorrectExceptionParameterOrderingTests.cs @@ -79,6 +79,7 @@ class A throw new ArgumentException (""The parameter 'blah' can not be null"", ""blah""); throw new ArgumentOutOfRangeException (""blah"", ""The parameter 'blah' can not be null""); throw new DuplicateWaitObjectException (""blah"", ""The parameter 'blah' can not be null""); + throw new ArgumentOutOfRangeException (""blah"", 42); } }"; TestRefactoringContext context; From 75925d5aaf0077320f48468f3aadde2f736bbb1c Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Fri, 5 Oct 2012 00:07:12 +0200 Subject: [PATCH 12/29] Fix #119: LocalVariableOnlyAssignedIssue does not recognize usage through unary operator expression --- .../LocalVariableOnlyAssignedIssue.cs | 4 ++-- .../ParameterOnlyAssignedIssue.cs | 4 ++-- .../VariableOnlyAssignedIssue.cs | 5 +++++ .../LocalVariableOnlyAssignedIssueTests.cs | 20 +++++++++++++++++-- 4 files changed, 27 insertions(+), 6 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/LocalVariableOnlyAssignedIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/LocalVariableOnlyAssignedIssue.cs index d30d245654..7b66ba463e 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/LocalVariableOnlyAssignedIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/LocalVariableOnlyAssignedIssue.cs @@ -30,7 +30,7 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring { [IssueDescription ("Local variable is only assigned", - Description = "Local variable is assigned by its value is never used", + Description = "Local variable is assigned but its value is never used", Category = IssueCategories.CodeQualityIssues, Severity = Severity.Warning, IssueMarker = IssueMarker.Underline)] @@ -62,7 +62,7 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring if (!TestOnlyAssigned (ctx, decl.Parent, resolveResult.Variable)) return; AddIssue (variableInitializer.NameToken, - ctx.TranslateString ("Local variable is assigned by its value is never used")); + ctx.TranslateString ("Local variable is assigned but its value is never used")); } } } diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/ParameterOnlyAssignedIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/ParameterOnlyAssignedIssue.cs index 0d5ab0c8da..f3797fef4e 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/ParameterOnlyAssignedIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/ParameterOnlyAssignedIssue.cs @@ -28,7 +28,7 @@ using ICSharpCode.NRefactory.Semantics; namespace ICSharpCode.NRefactory.CSharp.Refactoring { [IssueDescription ("Parameter is only assigned", - Description = "Parameter is assigned by its value is never used.", + Description = "Parameter is assigned but its value is never used.", Category = IssueCategories.CodeQualityIssues, Severity = Severity.Warning, IssueMarker = IssueMarker.Underline)] @@ -60,7 +60,7 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring return; } AddIssue(parameterDeclaration.NameToken, - ctx.TranslateString("Parameter is assigned by its value is never used")); + ctx.TranslateString("Parameter is assigned but its value is never used")); } } } diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/VariableOnlyAssignedIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/VariableOnlyAssignedIssue.cs index 6c6d6820c2..126b59da0c 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/VariableOnlyAssignedIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/VariableOnlyAssignedIssue.cs @@ -66,11 +66,16 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring case UnaryOperatorType.Decrement: case UnaryOperatorType.PostDecrement: assignment = true; + if (!(parent.Parent is ExpressionStatement)) + nonAssignment = true; continue; } } else if (parent is DirectionExpression) { if (((DirectionExpression)parent).FieldDirection == FieldDirection.Out) { assignment = true; + // Using dummy variables is necessary for ignoring + // out-arguments, so we don't want to warn for those. + nonAssignment = true; continue; } } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/LocalVariableOnlyAssignedIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/LocalVariableOnlyAssignedIssueTests.cs index be61be332c..8c5ddf0ba3 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/LocalVariableOnlyAssignedIssueTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/LocalVariableOnlyAssignedIssueTests.cs @@ -85,13 +85,29 @@ class TestClass { i = 1; } + void TestMethod() + { + int tmp; + Test (out tmp); + } +}"; + // should not warn because ignoring out-arguments is legitimate + Test (input1, 0); + } + + [Test] + public void TestIncrement () + { + var input1 = @" +class TestClass +{ void TestMethod() { int i = 1; - Test (out i); + Test (i++); } }"; - Test (input1, 1); + Test (input1, 0); } } } From 9cdf7e71f9fd3247a15b4a73db37bea5cf358576 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Fri, 5 Oct 2012 00:18:36 +0200 Subject: [PATCH 13/29] Don't show 'method never returns' for iterators containing 'yield break;' --- .../Analysis/ReachabilityAnalysis.cs | 4 +++ .../CodeIssues/MethodNeverReturnsIssue.cs | 34 ++++++------------- .../MethodNeverReturnsIssueTests.cs | 14 ++++++++ 3 files changed, 28 insertions(+), 24 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Analysis/ReachabilityAnalysis.cs b/ICSharpCode.NRefactory.CSharp/Analysis/ReachabilityAnalysis.cs index ebb8d419d3..524966d7fb 100644 --- a/ICSharpCode.NRefactory.CSharp/Analysis/ReachabilityAnalysis.cs +++ b/ICSharpCode.NRefactory.CSharp/Analysis/ReachabilityAnalysis.cs @@ -78,6 +78,10 @@ namespace ICSharpCode.NRefactory.CSharp.Analysis } } + public IEnumerable ReachableStatements { + get { return reachableStatements; } + } + public bool IsReachable(Statement statement) { return reachableStatements.Contains(statement); diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/MethodNeverReturnsIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/MethodNeverReturnsIssue.cs index 68ae2e8529..44c08d3299 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/MethodNeverReturnsIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/MethodNeverReturnsIssue.cs @@ -43,8 +43,6 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring class GatherVisitor : GatherVisitorBase { - readonly ControlFlowGraphBuilder cfgBuilder = new ControlFlowGraphBuilder (); - public GatherVisitor(BaseRefactoringContext ctx) : base (ctx) { @@ -58,30 +56,18 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring if (methodDeclaration.Body.IsNull) return; - var cfg = cfgBuilder.BuildControlFlowGraph (methodDeclaration.Body, ctx.Resolver, - ctx.CancellationToken); - var stack = new Stack (); - var visitedNodes = new HashSet (); - stack.Push (cfg [0]); - while (stack.Count > 0) { - var node = stack.Pop (); - - // reach method's end - if (node.PreviousStatement == methodDeclaration.Body) - return; - // reach a return statement - if (node.NextStatement is ReturnStatement || - node.NextStatement is ThrowStatement) - return; - - foreach (var edge in node.Outgoing) { - if (visitedNodes.Add(edge.To)) - stack.Push(edge.To); + var reachability = ctx.CreateReachabilityAnalysis(methodDeclaration.Body); + bool hasReachableReturn = false; + foreach (var statement in reachability.ReachableStatements) { + if (statement is ReturnStatement || statement is ThrowStatement || statement is YieldBreakStatement) { + hasReachableReturn = true; + break; } } - - AddIssue (methodDeclaration.NameToken, - ctx.TranslateString ("Method never reaches its end or a 'return' statement.")); + if (!hasReachableReturn && !reachability.IsEndpointReachable(methodDeclaration.Body)) { + AddIssue (methodDeclaration.NameToken, + ctx.TranslateString ("Method never reaches its end or a 'return' statement.")); + } } } } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/MethodNeverReturnsIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/MethodNeverReturnsIssueTests.cs index 2e5e4f5c1f..58b4db6a5d 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/MethodNeverReturnsIssueTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/MethodNeverReturnsIssueTests.cs @@ -87,5 +87,19 @@ class TestClass }"; Test (input, 1); } + + [Test] + public void YieldBreak () + { + var input = @" +class TestClass +{ + System.Collections.Generic.IEnumerable TestMethod () + { + yield break; + } +}"; + Test (input, 0); + } } } From 3e42bc1e6ab4adc7a871996dc2485570dda91738 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Fri, 5 Oct 2012 00:23:24 +0200 Subject: [PATCH 14/29] Don't show 'Method with optional parameter is hidden by overload' issue if the overloads have different type parameters. --- .../MethodOverloadHidesOptionalParameterIssue.cs | 2 +- ...MethodOverloadHidesOptionalParameterIssueTests.cs | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/MethodOverloadHidesOptionalParameterIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/MethodOverloadHidesOptionalParameterIssue.cs index 3420a3ddc2..2911cee187 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/MethodOverloadHidesOptionalParameterIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/MethodOverloadHidesOptionalParameterIssue.cs @@ -64,7 +64,7 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring if (method.Parameters.Count == 0 || !method.Parameters.Last ().IsOptional) return; - var overloads = method.DeclaringType.GetMembers (m => m.Name == method.Name).OfType () + var overloads = method.DeclaringType.GetMethods(m => m.Name == method.Name && m.TypeParameters.Count == method.TypeParameters.Count) .ToArray (); var parameterNodes = methodDeclaration.Parameters.ToArray(); diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/MethodOverloadHidesOptionalParameterIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/MethodOverloadHidesOptionalParameterIssueTests.cs index 14df3a1dd3..5c72a57eea 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/MethodOverloadHidesOptionalParameterIssueTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/MethodOverloadHidesOptionalParameterIssueTests.cs @@ -68,6 +68,18 @@ class TestClass { void TestMethod (int a, int b = 1, int c = 1) { } +}"; + Test (input, 0); + } + + [Test] + public void TestNoIssue_Generics () + { + var input = @" +class TestClass +{ + void TestMethod (object obj) { } + void TestMethod (object obj, int arg = 0) { } }"; Test (input, 0); } From 912017d123135b2410a17c776c412780085ca7be Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Fri, 5 Oct 2012 00:40:33 +0200 Subject: [PATCH 15/29] RedundantWhereWithPredicateIssue: in addition to Any(), handle Count(),First(),FirstOrDefault(),Last(),LastOrDefault(),LongCount(),Single() and SingleOrDefault(). --- .../RedundantWhereWithPredicateIssue.cs | 62 +++++++++++++++---- .../RedundantWhereWithPredicateIssueTests.cs | 23 +++++++ 2 files changed, 72 insertions(+), 13 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantWhereWithPredicateIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantWhereWithPredicateIssue.cs index 45c930748f..3b255c029d 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantWhereWithPredicateIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantWhereWithPredicateIssue.cs @@ -6,7 +6,7 @@ using ICSharpCode.NRefactory.PatternMatching; namespace ICSharpCode.NRefactory.CSharp.Refactoring { - [IssueDescription("Any() should be used with predicate and Where() removed", + [IssueDescription("Any()/First()/etc. should be used with predicate and Where() removed", Description= "Detects redundant Where() with predicate calls followed by Any().", Category = IssueCategories.CodeQualityIssues, Severity = Severity.Hint)] @@ -15,11 +15,11 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring static readonly AstNode pattern = new InvocationExpression ( new MemberReferenceExpression ( - new NamedNode ("whereInvoke", - new InvocationExpression ( - new MemberReferenceExpression (new AnyNode ("target"), "Where"), - new AnyNode ())), - "Any")); + new NamedNode ("whereInvoke", + new InvocationExpression ( + new MemberReferenceExpression (new AnyNode ("target"), "Where"), + new AnyNode ())), + Pattern.AnyString)); public IEnumerable GetIssues(BaseRefactoringContext context) { @@ -41,11 +41,11 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring return; var anyResolve = ctx.Resolve (anyInvoke) as InvocationResolveResult; - if (anyResolve == null || anyResolve.Member.FullName != "System.Linq.Enumerable.Any") + if (anyResolve == null || !HasPredicateVersion(anyResolve.Member)) return; var whereInvoke = match.Get ("whereInvoke").Single (); var whereResolve = ctx.Resolve (whereInvoke) as InvocationResolveResult; - if (whereResolve == null || whereResolve.Member.FullName != "System.Linq.Enumerable.Where") + if (whereResolve == null || whereResolve.Member.Name != "Where" || !IsQueryExtensionClass(whereResolve.Member.DeclaringTypeDefinition)) return; if (whereResolve.Member.Parameters.Count != 2) return; @@ -53,11 +53,47 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring if (predResolve.Type.TypeParameterCount != 2) return; - AddIssue (anyInvoke, "Redundant Where() call with predicate followed by Any()", script => { - var arg = whereInvoke.Arguments.Single ().Clone (); - var target = match.Get ("target").Single ().Clone (); - script.Replace (anyInvoke, new InvocationExpression (new MemberReferenceExpression (target, "Any"), arg)); - }); + AddIssue ( + anyInvoke, string.Format("Redundant Where() call with predicate followed by {0}()", anyResolve.Member.Name), + script => { + var arg = whereInvoke.Arguments.Single ().Clone (); + var target = match.Get ("target").Single ().Clone (); + script.Replace (anyInvoke, new InvocationExpression (new MemberReferenceExpression (target, anyResolve.Member.Name), arg)); + }); + } + + bool IsQueryExtensionClass(ITypeDefinition typeDef) + { + if (typeDef == null || typeDef.Namespace != "System.Linq") + return false; + switch (typeDef.Name) { + case "Enumerable": + case "ParallelEnumerable": + case "Queryable": + return true; + default: + return false; + } + } + + bool HasPredicateVersion(IParameterizedMember member) + { + if (!IsQueryExtensionClass(member.DeclaringTypeDefinition)) + return false; + switch (member.Name) { + case "Any": + case "Count": + case "First": + case "FirstOrDefault": + case "Last": + case "LastOrDefault": + case "LongCount": + case "Single": + case "SingleOrDefault": + return true; + default: + return false; + } } } } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantWhereWithPredicateIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantWhereWithPredicateIssueTests.cs index 9c712e6726..fc40866ca6 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantWhereWithPredicateIssueTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantWhereWithPredicateIssueTests.cs @@ -66,5 +66,28 @@ public class X var issues = GetIssues (new RedundantWhereWithPredicateIssue (), input, out context); Assert.AreEqual (0, issues.Count); } + + [Test] + public void TestWhereCount() + { + var input = @"using System.Linq; +public class CSharpDemo { + public void Bla () { + int[] arr; + var bla = arr.Where (x => x < 4).Count (); + } +}"; + + TestRefactoringContext context; + var issues = GetIssues (new RedundantWhereWithPredicateIssue (), input, out context); + Assert.AreEqual (1, issues.Count); + CheckFix (context, issues, @"using System.Linq; +public class CSharpDemo { + public void Bla () { + int[] arr; + var bla = arr.Count (x => x < 4); + } +}"); + } } } From 81c09524d65a684aeac3f3232271fa6337e4fd43 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Fri, 5 Oct 2012 00:54:19 +0200 Subject: [PATCH 16/29] Reference to static member via derived type: ignore curiously recurring template pattern --- ...erenceToStaticMemberViaDerivedTypeIssue.cs | 27 +++++++++++++++++-- ...erenceToStaticMemberViaDerivedTypeTests.cs | 22 +++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ReferenceToStaticMemberViaDerivedTypeIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ReferenceToStaticMemberViaDerivedTypeIssue.cs index d050067f58..5342de7b4d 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ReferenceToStaticMemberViaDerivedTypeIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ReferenceToStaticMemberViaDerivedTypeIssue.cs @@ -89,6 +89,12 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring return; if (typeResolveResult.Type.Equals(member.DeclaringType)) return; + // check whether member.DeclaringType contains the original type + // (curiously recurring template pattern) + var v = new ContainsTypeVisitor(typeResolveResult.Type.GetDefinition()); + member.DeclaringType.AcceptVisitor(v); + if (v.IsContained) + return; AddIssue(issueAnchor, context.TranslateString("Static method invoked via derived type"), GetActions(context, targetExpression, member)); } @@ -96,14 +102,31 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring IEnumerable GetActions(BaseRefactoringContext context, Expression targetExpression, IMember member) { - var csResolver = context.Resolver.GetResolverStateBefore(targetExpression); - var builder = new TypeSystemAstBuilder(csResolver); + var builder = context.CreateTypeSytemAstBuilder(targetExpression); var newType = builder.ConvertType(member.DeclaringType); string description = string.Format("{0} '{1}'", context.TranslateString("Use base class"), newType.GetText()); yield return new CodeAction(description, script => { script.Replace(targetExpression, newType); }); } + + sealed class ContainsTypeVisitor : TypeVisitor + { + readonly ITypeDefinition searchedType; + internal bool IsContained; + + public ContainsTypeVisitor(ITypeDefinition searchedType) + { + this.searchedType = searchedType; + } + + public override IType VisitTypeDefinition(ITypeDefinition type) + { + if (type.Equals(searchedType)) + IsContained = true; + return base.VisitTypeDefinition(type); + } + } } #endregion } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ReferenceToStaticMemberViaDerivedTypeTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ReferenceToStaticMemberViaDerivedTypeTests.cs index c46e2b872e..f357818b34 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ReferenceToStaticMemberViaDerivedTypeTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ReferenceToStaticMemberViaDerivedTypeTests.cs @@ -299,6 +299,28 @@ class B : A var issues = GetIssues(new ReferenceToStaticMemberViaDerivedTypeIssue(), input, out context); Assert.AreEqual(0, issues.Count); } + + [Test] + public void IgnoresCuriouslyRecurringTemplatePattern() + { + var input = @" +class Base +{ + public static void F() { } +} +class Derived : Base {} +class Test +{ + void Main() + { + Derived.F(); + } +}"; + // do not suggest replacing 'Derived.F()' with 'Base.F()' + TestRefactoringContext context; + var issues = GetIssues(new ReferenceToStaticMemberViaDerivedTypeIssue(), input, out context); + Assert.AreEqual(0, issues.Count); + } } } From e272d518b6b2f50b09ecab5626e9ed584c92d6c9 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Fri, 5 Oct 2012 01:08:06 +0200 Subject: [PATCH 17/29] AssignmentMadeToSameVariableIssue: don't warn on "a += a;" --- .../AssignmentMadeToSameVariableIssue.cs | 2 ++ .../AssignmentMadeToSameVariableIssueTests.cs | 14 ++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AssignmentMadeToSameVariableIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AssignmentMadeToSameVariableIssue.cs index 7437e74f1c..055518a24c 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AssignmentMadeToSameVariableIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AssignmentMadeToSameVariableIssue.cs @@ -56,6 +56,8 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring { base.VisitAssignmentExpression (assignmentExpression); + if (assignmentExpression.Operator != AssignmentOperatorType.Assign) + return; if (!(assignmentExpression.Left is IdentifierExpression) && !(assignmentExpression.Left is MemberReferenceExpression)) return; diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/AssignmentMadeToSameVariableIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/AssignmentMadeToSameVariableIssueTests.cs index b167903d4c..46329fc0e0 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/AssignmentMadeToSameVariableIssueTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/AssignmentMadeToSameVariableIssueTests.cs @@ -209,6 +209,20 @@ class TestClass { nested.nested.a = nested.nested.a; } +}"; + Test (input, 0); + } + + [Test] + public void TestNoIssueWithCompoundOperator () + { + var input = @" +class TestClass +{ + void TestMethod (int a) + { + a += a; + } }"; Test (input, 0); } From 211c6a1b05ef652dabd68a24c73541dcbd1d8d6c Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Fri, 5 Oct 2012 01:37:43 +0200 Subject: [PATCH 18/29] Implemented ITypeParameter.EffectiveInterfaceSet --- .../TypeSystem/ITypeParameter.cs | 2 +- .../AbstractResolvedTypeParameter.cs | 85 ++++++++++++------- .../Implementation/DummyTypeParameter.cs | 2 +- 3 files changed, 57 insertions(+), 32 deletions(-) diff --git a/ICSharpCode.NRefactory/TypeSystem/ITypeParameter.cs b/ICSharpCode.NRefactory/TypeSystem/ITypeParameter.cs index 9d4b360635..7e0a89d8f8 100644 --- a/ICSharpCode.NRefactory/TypeSystem/ITypeParameter.cs +++ b/ICSharpCode.NRefactory/TypeSystem/ITypeParameter.cs @@ -107,7 +107,7 @@ namespace ICSharpCode.NRefactory.TypeSystem /// /// Gets the effective interface set of this type parameter. /// - IList EffectiveInterfaceSet { get; } + ICollection EffectiveInterfaceSet { get; } /// /// Gets if the type parameter has the 'new()' constraint. diff --git a/ICSharpCode.NRefactory/TypeSystem/Implementation/AbstractResolvedTypeParameter.cs b/ICSharpCode.NRefactory/TypeSystem/Implementation/AbstractResolvedTypeParameter.cs index 8b12e0e920..cede6c7a8b 100644 --- a/ICSharpCode.NRefactory/TypeSystem/Implementation/AbstractResolvedTypeParameter.cs +++ b/ICSharpCode.NRefactory/TypeSystem/Implementation/AbstractResolvedTypeParameter.cs @@ -94,48 +94,73 @@ namespace ICSharpCode.NRefactory.TypeSystem.Implementation public IType EffectiveBaseClass { get { - if (effectiveBaseClass == null) - effectiveBaseClass = CalculateEffectiveBaseClass(); + if (effectiveBaseClass == null) { + // protect against cyclic type parameters + using (var busyLock = BusyManager.Enter(this)) { + if (!busyLock.Success) + return SpecialType.UnknownType; // don't cache this error + effectiveBaseClass = CalculateEffectiveBaseClass(); + } + } return effectiveBaseClass; } } IType CalculateEffectiveBaseClass() { - // protect against cyclic type parameters - using (var busyLock = BusyManager.Enter(this)) { - if (!busyLock.Success) - return SpecialType.UnknownType; - - if (HasValueTypeConstraint) - return this.Compilation.FindType(KnownTypeCode.ValueType); - - List classTypeConstraints = new List(); - foreach (IType constraint in this.DirectBaseTypes) { - if (constraint.Kind == TypeKind.Class) { - classTypeConstraints.Add(constraint); - } else if (constraint.Kind == TypeKind.TypeParameter) { - IType baseClass = ((ITypeParameter)constraint).EffectiveBaseClass; - if (baseClass.Kind == TypeKind.Class) - classTypeConstraints.Add(baseClass); - } - } - if (classTypeConstraints.Count == 0) - return this.Compilation.FindType(KnownTypeCode.Object); - // Find the derived-most type in the resulting set: - IType result = classTypeConstraints[0]; - for (int i = 1; i < classTypeConstraints.Count; i++) { - if (classTypeConstraints[i].GetDefinition().IsDerivedFrom(result.GetDefinition())) - result = classTypeConstraints[i]; + if (HasValueTypeConstraint) + return this.Compilation.FindType(KnownTypeCode.ValueType); + + List classTypeConstraints = new List(); + foreach (IType constraint in this.DirectBaseTypes) { + if (constraint.Kind == TypeKind.Class) { + classTypeConstraints.Add(constraint); + } else if (constraint.Kind == TypeKind.TypeParameter) { + IType baseClass = ((ITypeParameter)constraint).EffectiveBaseClass; + if (baseClass.Kind == TypeKind.Class) + classTypeConstraints.Add(baseClass); } - return result; } + if (classTypeConstraints.Count == 0) + return this.Compilation.FindType(KnownTypeCode.Object); + // Find the derived-most type in the resulting set: + IType result = classTypeConstraints[0]; + for (int i = 1; i < classTypeConstraints.Count; i++) { + if (classTypeConstraints[i].GetDefinition().IsDerivedFrom(result.GetDefinition())) + result = classTypeConstraints[i]; + } + return result; } - public IList EffectiveInterfaceSet { + ICollection effectiveInterfaceSet; + + public ICollection EffectiveInterfaceSet { get { - throw new NotImplementedException(); + var result = LazyInit.VolatileRead(ref effectiveInterfaceSet); + if (result != null) { + return result; + } else { + // protect against cyclic type parameters + using (var busyLock = BusyManager.Enter(this)) { + if (!busyLock.Success) + return EmptyList.Instance; // don't cache this error + return LazyInit.GetOrSet(ref effectiveInterfaceSet, CalculateEffectiveInterfaceSet()); + } + } + } + } + + ICollection CalculateEffectiveInterfaceSet() + { + HashSet result = new HashSet(); + foreach (IType constraint in this.DirectBaseTypes) { + if (constraint.Kind == TypeKind.Interface) { + result.Add(constraint); + } else if (constraint.Kind == TypeKind.TypeParameter) { + result.UnionWith(((ITypeParameter)constraint).EffectiveInterfaceSet); + } } + return result; } public abstract bool HasDefaultConstructorConstraint { get; } diff --git a/ICSharpCode.NRefactory/TypeSystem/Implementation/DummyTypeParameter.cs b/ICSharpCode.NRefactory/TypeSystem/Implementation/DummyTypeParameter.cs index 22e7fc4fc8..f0ec26d0e1 100644 --- a/ICSharpCode.NRefactory/TypeSystem/Implementation/DummyTypeParameter.cs +++ b/ICSharpCode.NRefactory/TypeSystem/Implementation/DummyTypeParameter.cs @@ -189,7 +189,7 @@ namespace ICSharpCode.NRefactory.TypeSystem.Implementation get { return SpecialType.UnknownType; } } - IList ITypeParameter.EffectiveInterfaceSet { + ICollection ITypeParameter.EffectiveInterfaceSet { get { return EmptyList.Instance; } } From e62e9469a71db40f0328c38540bd601b2de1f9c1 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Fri, 5 Oct 2012 01:38:12 +0200 Subject: [PATCH 19/29] Simplified ExpressionIsNeverOfProvidedTypeIssue and fixed false positive when converting from System.Object to value types. --- .../ICSharpCode.NRefactory.CSharp.csproj | 3 +- .../ExpressionIsAlwaysOfProvidedTypeIssue.cs | 8 +- .../ExpressionIsNeverOfProvidedTypeIssue.cs | 32 ++++- .../Refactoring/TypeCompatibilityHelper.cs | 128 ------------------ ...pressionIsNeverOfProvidedTypeIssueTests.cs | 46 ++++--- 5 files changed, 64 insertions(+), 153 deletions(-) delete mode 100644 ICSharpCode.NRefactory.CSharp/Refactoring/TypeCompatibilityHelper.cs diff --git a/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj b/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj index d0dde5fa32..fe0c1ba882 100644 --- a/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj +++ b/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj @@ -333,7 +333,6 @@ - @@ -536,4 +535,4 @@ - + \ No newline at end of file diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsAlwaysOfProvidedTypeIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsAlwaysOfProvidedTypeIssue.cs index e5eb242e76..50770b0379 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsAlwaysOfProvidedTypeIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsAlwaysOfProvidedTypeIssue.cs @@ -44,11 +44,11 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring class GatherVisitor : GatherVisitorBase { - static CSharpConversions conversion; + readonly CSharpConversions conversions; public GatherVisitor (BaseRefactoringContext ctx) : base (ctx) { - conversion = new CSharpConversions(ctx.Compilation); + conversions = CSharpConversions.Get(ctx.Compilation); } public override void VisitIsExpression (IsExpression isExpression) @@ -58,8 +58,8 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring var type = ctx.Resolve (isExpression.Expression).Type; var providedType = ctx.ResolveType (isExpression.Type); - var foundConversion = conversion.ImplicitConversion(type, providedType); - if (foundConversion == Conversion.None) + var foundConversion = conversions.ImplicitConversion(type, providedType); + if (!foundConversion.IsValid) return; var action = new CodeAction (ctx.TranslateString ("Compare with 'null'"), diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsNeverOfProvidedTypeIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsNeverOfProvidedTypeIssue.cs index 7c28f3d1f9..ed10a12d8f 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsNeverOfProvidedTypeIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsNeverOfProvidedTypeIssue.cs @@ -25,6 +25,10 @@ // THE SOFTWARE. using System.Collections.Generic; +using System.Linq; +using ICSharpCode.NRefactory.CSharp.Resolver; +using ICSharpCode.NRefactory.Semantics; +using ICSharpCode.NRefactory.TypeSystem; namespace ICSharpCode.NRefactory.CSharp.Refactoring { @@ -42,21 +46,43 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring class GatherVisitor : GatherVisitorBase { + readonly CSharpConversions conversions; public GatherVisitor (BaseRefactoringContext ctx) : base (ctx) { + conversions = CSharpConversions.Get(ctx.Compilation); } public override void VisitIsExpression (IsExpression isExpression) { base.VisitIsExpression (isExpression); + var conversions = CSharpConversions.Get(ctx.Compilation); var exprType = ctx.Resolve (isExpression.Expression).Type; var providedType = ctx.ResolveType (isExpression.Type); - if (TypeCompatibilityHelper.CheckTypeCompatibility(exprType, providedType) == - TypeCompatibilityHelper.TypeCompatiblity.NeverOfProvidedType) - AddIssue (isExpression, ctx.TranslateString ("Given expression is never of the provided type")); + if (IsValidReferenceOrBoxingConversion(exprType, providedType)) + return; + + var exprTP = exprType as ITypeParameter; + var providedTP = providedType as ITypeParameter; + if (exprTP != null) { + if (IsValidReferenceOrBoxingConversion(exprTP.EffectiveBaseClass, providedType) + && exprTP.EffectiveInterfaceSet.All(i => IsValidReferenceOrBoxingConversion(i, providedType))) + return; + } + if (providedTP != null) { + if (IsValidReferenceOrBoxingConversion(exprType, providedTP.EffectiveBaseClass)) + return; + } + + AddIssue (isExpression, ctx.TranslateString ("Given expression is never of the provided type")); + } + + bool IsValidReferenceOrBoxingConversion(IType fromType, IType toType) + { + Conversion c = conversions.ExplicitConversion(fromType, toType); + return c.IsValid && (c.IsIdentityConversion || c.IsReferenceConversion || c.IsBoxingConversion || c.IsUnboxingConversion); } } } diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/TypeCompatibilityHelper.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/TypeCompatibilityHelper.cs deleted file mode 100644 index 98ae4e36fb..0000000000 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/TypeCompatibilityHelper.cs +++ /dev/null @@ -1,128 +0,0 @@ -// -// TypeCompatibilityHelper.cs -// -// Author: -// Mansheng Yang -// -// Copyright (c) 2012 Mansheng Yang -// -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files (the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: -// -// The above copyright notice and this permission notice shall be included in -// all copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -// THE SOFTWARE. - -using System.Collections.Generic; -using System.Linq; -using ICSharpCode.NRefactory.TypeSystem; - -namespace ICSharpCode.NRefactory.CSharp.Refactoring -{ - class TypeCompatibilityHelper - { - public enum TypeCompatiblity - { - MayOfProvidedType, - AlwaysOfProvidedType, - NeverOfProvidedType - } - - static bool CheckTypeParameterConstraints (IType type, IEnumerable baseTypes, - ITypeParameter typeParameter) - { - if (!typeParameter.DirectBaseTypes.All (t => baseTypes.Any (t2 => t2.Equals (t)))) - return false; - if (typeParameter.HasDefaultConstructorConstraint && - !type.GetConstructors (c => c.IsPublic && c.Parameters.Count == 0).Any ()) - return false; - return true; - } - - public static TypeCompatiblity CheckTypeCompatibility(IType exprType, IType providedType) - { - var exprBaseTypes = exprType.GetAllBaseTypes ().ToArray (); - - // providedType is a base type of exprType - if (exprBaseTypes.Any (t => t.Equals (providedType))) - return TypeCompatiblity.AlwaysOfProvidedType; - - if ((exprType.IsReferenceType == true && providedType.IsReferenceType == false) || - (exprType.IsReferenceType == false && providedType.IsReferenceType == true)) - return TypeCompatiblity.NeverOfProvidedType; - - var typeParameter = exprType as ITypeParameter; - var providedTypeParameter = providedType as ITypeParameter; - - if (typeParameter != null) { - // check if providedType can be a derived type - var providedBaseTypes = providedType.GetAllBaseTypes ().ToArray (); - var providedTypeDef = providedType.GetDefinition (); - // if providedType is sealed, check if it fullfills all the type parameter constraints, - // otherwise, only check if it is derived from EffectiveBaseClass - if (providedTypeParameter == null && (providedTypeDef == null || providedTypeDef.IsSealed)) { - if (CheckTypeParameterConstraints (providedType, providedBaseTypes, typeParameter)) - return TypeCompatiblity.MayOfProvidedType; - } else if (providedBaseTypes.Any (t => t.Equals (typeParameter.EffectiveBaseClass))) { - return TypeCompatiblity.MayOfProvidedType; - } - - // if providedType is also a type parameter, check if base classes are compatible - if (providedTypeParameter != null && - exprBaseTypes.Any (t => t.Equals (providedTypeParameter.EffectiveBaseClass))) - return TypeCompatiblity.MayOfProvidedType; - - return TypeCompatiblity.NeverOfProvidedType; - } - // check if exprType fullfills all the type parameter constraints - if (providedTypeParameter != null && - CheckTypeParameterConstraints (exprType, exprBaseTypes, providedTypeParameter)) - return TypeCompatiblity.MayOfProvidedType; - - switch (exprType.Kind) { - case TypeKind.Class: - var exprTypeDef = exprType.GetDefinition (); - if (exprTypeDef == null) - return TypeCompatiblity.MayOfProvidedType; - // exprType is sealed, but providedType is not a base type of it or it does not - // fullfill all the type parameter constraints - if (exprTypeDef.IsSealed) - break; - - // check if providedType can be a derived type - if (providedType.Kind == TypeKind.Interface || - providedType.GetAllBaseTypes ().Any (t => t.Equals (exprType))) - return TypeCompatiblity.MayOfProvidedType; - - if (providedTypeParameter != null && - exprBaseTypes.Any (t => t.Equals (providedTypeParameter.EffectiveBaseClass))) - return TypeCompatiblity.MayOfProvidedType; - - break; - - case TypeKind.Struct: - case TypeKind.Delegate: - case TypeKind.Enum: - case TypeKind.Array: - case TypeKind.Anonymous: - case TypeKind.Null: - break; - - default: - return TypeCompatiblity.MayOfProvidedType; - } - return TypeCompatiblity.NeverOfProvidedType; - } - } -} diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ExpressionIsNeverOfProvidedTypeIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ExpressionIsNeverOfProvidedTypeIssueTests.cs index fc73329b43..e3c006fe2f 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ExpressionIsNeverOfProvidedTypeIssueTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ExpressionIsNeverOfProvidedTypeIssueTests.cs @@ -92,6 +92,20 @@ class TestClass Test (input, 1); } + [Test] + public void TestObjectToInt () + { + var input = @" +sealed class TestClass +{ + void TestMethod (object x) + { + if (x is int) ; + } +}"; + Test (input, 0); + } + [Test] public void TestClassIsTypeParameter () { @@ -153,7 +167,8 @@ class TestClass if (x is T) ; } }"; - Test (input, 1); + // this is possible with T==object + Test (input, 0); } [Test] @@ -215,21 +230,6 @@ sealed class TestClass Test (input, 1); } - [Test] - public void TestTypeParameter5 () - { - var input = @" -sealed class TestClass -{ - public TestClass (int i) { } - void TestMethod (T x) where T : new() - { - if (x is TestClass) ; - } -}"; - Test (input, 1); - } - [Test] public void TestTypeParameterIsTypeParameter () { @@ -256,6 +256,20 @@ class TestClass { if (x is T2) ; } +}"; + Test (input, 0); + } + + [Test] + public void TestObjectArrayToStringArray () + { + var input = @" +sealed class TestClass +{ + void TestMethod (object[] x) + { + if (x is string[]) ; + } }"; Test (input, 0); } From be94aebdcc15b8b5f21e665bd86e82da4556f63b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20Kr=C3=BCger?= Date: Fri, 5 Oct 2012 06:40:03 +0200 Subject: [PATCH 20/29] [Refactoring] FormatText now specifies a formatting region. (speed purposes) --- ICSharpCode.NRefactory.CSharp/Refactoring/DocumentScript.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/DocumentScript.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/DocumentScript.cs index f472880a68..537ccf5c28 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/DocumentScript.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/DocumentScript.cs @@ -100,6 +100,7 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring var segment = GetSegment(node); var syntaxTree = SyntaxTree.Parse(currentDocument, "dummy.cs"); var formatter = new AstFormattingVisitor(FormattingOptions, currentDocument, Options); + formatter.FormattingRegion = new ICSharpCode.NRefactory.TypeSystem.DomRegion (node.StartLocation, node.EndLocation); syntaxTree.AcceptVisitor(formatter); formatter.ApplyChanges(segment.Offset, segment.Length); } From 8e1ed7e962cd43eb3c1c42e3f8ac02e3fa843e1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20Kr=C3=BCger?= Date: Fri, 5 Oct 2012 07:36:38 +0200 Subject: [PATCH 21/29] [Refactoring] Format text can now take a node list to format / fixed a formatting issue. --- .../Formatter/AstFormattingVisitor.cs | 17 +++++++++++------ .../CodeActions/IterateViaForeachAction.cs | 2 +- .../Refactoring/DocumentScript.cs | 17 +++++++++++------ .../Refactoring/Script.cs | 16 ++++++++++++---- .../CodeActions/ConvertSwitchToIfTests.cs | 11 ++++++++--- .../CSharp/CodeActions/MoveToOuterScopeTests.cs | 6 +++--- .../SplitDeclarationAndAssignmentTests.cs | 1 + 7 files changed, 47 insertions(+), 23 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Formatter/AstFormattingVisitor.cs b/ICSharpCode.NRefactory.CSharp/Formatter/AstFormattingVisitor.cs index 06c1f6e4b6..a9de1e7295 100644 --- a/ICSharpCode.NRefactory.CSharp/Formatter/AstFormattingVisitor.cs +++ b/ICSharpCode.NRefactory.CSharp/Formatter/AstFormattingVisitor.cs @@ -119,16 +119,20 @@ namespace ICSharpCode.NRefactory.CSharp protected override void VisitChildren (AstNode node) { - if (!FormattingRegion.IsEmpty) { - if (node.EndLocation < FormattingRegion.Begin || node.StartLocation > FormattingRegion.End) - return; - } - AstNode next; for (var child = node.FirstChild; child != null; child = next) { // Store next to allow the loop to continue // if the visitor removes/replaces child. next = child.NextSibling; + + if (!FormattingRegion.IsEmpty) { + if (child.EndLocation < FormattingRegion.Begin) { + continue; + } + if (child.StartLocation > FormattingRegion.End) { + break; + } + } child.AcceptVisitor (this); } } @@ -404,7 +408,8 @@ namespace ICSharpCode.NRefactory.CSharp void ForceSpace(int startOffset, int endOffset, bool forceSpace) { int lastNonWs = SearchLastNonWsChar(startOffset, endOffset); - AddChange(lastNonWs + 1, System.Math.Max(0, endOffset - lastNonWs - 1), forceSpace ? " " : ""); + if (lastNonWs >= 0) + AddChange(lastNonWs + 1, System.Math.Max(0, endOffset - lastNonWs - 1), forceSpace ? " " : ""); } // void ForceSpacesAfter (AstNode n, bool forceSpaces) // { diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/IterateViaForeachAction.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/IterateViaForeachAction.cs index 0e3fed4fe2..a29a9f62c5 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/IterateViaForeachAction.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeActions/IterateViaForeachAction.cs @@ -70,7 +70,7 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring var blockStatement = new BlockStatement(); blockStatement.Statements.Add(iterator); script.Replace(usingStatement.EmbeddedStatement, blockStatement); - script.FormatText(blockStatement); + script.FormatText((AstNode)blockStatement); } else if (usingStatement.EmbeddedStatement is BlockStatement) { var anchorNode = usingStatement.EmbeddedStatement.FirstChild; script.InsertAfter(anchorNode, iterator); diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/DocumentScript.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/DocumentScript.cs index 537ccf5c28..41af9dd2d2 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/DocumentScript.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/DocumentScript.cs @@ -17,8 +17,10 @@ // DEALINGS IN THE SOFTWARE. using System; +using System.Linq; using System.Diagnostics; using ICSharpCode.NRefactory.Editor; +using System.Collections.Generic; namespace ICSharpCode.NRefactory.CSharp.Refactoring { @@ -95,14 +97,17 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring return originalDocument.Version.MoveOffsetTo(currentDocument.Version, originalDocumentOffset); } - public override void FormatText(AstNode node) + public override void FormatText(IEnumerable nodes) { - var segment = GetSegment(node); var syntaxTree = SyntaxTree.Parse(currentDocument, "dummy.cs"); - var formatter = new AstFormattingVisitor(FormattingOptions, currentDocument, Options); - formatter.FormattingRegion = new ICSharpCode.NRefactory.TypeSystem.DomRegion (node.StartLocation, node.EndLocation); - syntaxTree.AcceptVisitor(formatter); - formatter.ApplyChanges(segment.Offset, segment.Length); + foreach (var node in nodes.OrderByDescending (n => n.StartLocation)) { + var segment = GetSegment(node); + var formatter = new AstFormattingVisitor(FormattingOptions, currentDocument, Options); + + formatter.FormattingRegion = new ICSharpCode.NRefactory.TypeSystem.DomRegion (node.StartLocation, node.EndLocation); + syntaxTree.AcceptVisitor(formatter); + formatter.ApplyChanges(segment.Offset, segment.Length); + } } protected override int GetIndentLevelAt(int offset) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/Script.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/Script.cs index 51897bf50e..75d92c1c7e 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/Script.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/Script.cs @@ -202,21 +202,28 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring CorrectFormatting (node, node); } + List nodesToFormat = new List (); + void CorrectFormatting(AstNode node, AstNode newNode) { if (node is Identifier || node is IdentifierExpression || node is CSharpTokenNode || node is AstType) return; if (node == null || node.Parent is BlockStatement) { - FormatText(newNode); + nodesToFormat.Add (newNode); } else { - FormatText((node.Parent != null && (node.Parent is Statement || node.Parent is Expression || node.Parent is VariableInitializer)) ? node.Parent : newNode); + nodesToFormat.Add ((node.Parent != null && (node.Parent is Statement || node.Parent is Expression || node.Parent is VariableInitializer)) ? node.Parent : newNode); } } public abstract void Remove (AstNode node, bool removeEmptyLine = true); - public abstract void FormatText (AstNode node); - + public abstract void FormatText (IEnumerable nodes); + + public void FormatText (params AstNode[] node) + { + FormatText ((IEnumerable)node); + } + public virtual void Select (AstNode node) { // default implementation: do nothing @@ -380,6 +387,7 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring public virtual void Dispose() { + FormatText (nodesToFormat); } public enum NewTypeContext { diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/ConvertSwitchToIfTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/ConvertSwitchToIfTests.cs index 5f2bc4180b..1af817801d 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/ConvertSwitchToIfTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/ConvertSwitchToIfTests.cs @@ -138,10 +138,15 @@ class TestClass { if (a == 0) { int b = 1; - } else if (a == 1 || a == 2) { - } else if (a == 3 || a == 4 || a == 5) { - } else { } + else + if (a == 1 || a == 2) { + } + else + if (a == 3 || a == 4 || a == 5) { + } + else { + } } }"); } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/MoveToOuterScopeTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/MoveToOuterScopeTests.cs index c9f40e5503..897d332420 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/MoveToOuterScopeTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/MoveToOuterScopeTests.cs @@ -88,7 +88,7 @@ class A int i = 2; while (true) { int j = 3; - } + } "); } @@ -116,10 +116,10 @@ class A } ", @" int j; - while (true) { + while (true) { int i = 2; j = i; - } + } "); } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/SplitDeclarationAndAssignmentTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/SplitDeclarationAndAssignmentTests.cs index b0eb3566e9..8d5749671a 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/SplitDeclarationAndAssignmentTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeActions/SplitDeclarationAndAssignmentTests.cs @@ -101,6 +101,7 @@ namespace ICSharpCode.NRefactory.CSharp.CodeActions " {" + Environment.NewLine + " int i;" + Environment.NewLine + " for (i = 1; i < 10; i++) {" + Environment.NewLine + + " " + Environment.NewLine + " }" + Environment.NewLine + " }" + Environment.NewLine + "}", result); From 810a1e8911ca7ffababe9bdc3bd8f267d8dbc39c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20Kr=C3=BCger?= Date: Fri, 5 Oct 2012 07:45:49 +0200 Subject: [PATCH 22/29] [Refactoring] Corrected formatting region. --- ICSharpCode.NRefactory.CSharp/Refactoring/DocumentScript.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/DocumentScript.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/DocumentScript.cs index 41af9dd2d2..7e4e9e0b36 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/DocumentScript.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/DocumentScript.cs @@ -104,7 +104,10 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring var segment = GetSegment(node); var formatter = new AstFormattingVisitor(FormattingOptions, currentDocument, Options); - formatter.FormattingRegion = new ICSharpCode.NRefactory.TypeSystem.DomRegion (node.StartLocation, node.EndLocation); + formatter.FormattingRegion = new ICSharpCode.NRefactory.TypeSystem.DomRegion ( + currentDocument.GetLocation (segment.Offset), + currentDocument.GetLocation (segment.EndOffset) + ); syntaxTree.AcceptVisitor(formatter); formatter.ApplyChanges(segment.Offset, segment.Length); } From 4d49d61ab5bb35fffb48db64cf127267c7de63a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20Kr=C3=BCger?= Date: Fri, 5 Oct 2012 08:26:38 +0200 Subject: [PATCH 23/29] [CodeIssues] Added type checking to string is null or empty issue. --- .../CodeIssues/StringIsNullOrEmptyIssue.cs | 5 ++++- .../StringIsNullOrEmptyInspectorTests.cs | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/StringIsNullOrEmptyIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/StringIsNullOrEmptyIssue.cs index a3cc5237a2..12016f7907 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/StringIsNullOrEmptyIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/StringIsNullOrEmptyIssue.cs @@ -31,7 +31,7 @@ using ICSharpCode.NRefactory.PatternMatching; namespace ICSharpCode.NRefactory.CSharp.Refactoring { /// - /// Checks for str == null && str == "" + /// Checks for str == null && str == " " /// Converts to: string.IsNullOrEmpty (str) /// [IssueDescription("Use string.IsNullOrEmpty", @@ -124,6 +124,9 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring } if (m.Success) { var str = m.Get("str").Single(); + var def = ctx.Resolve (str).Type.GetDefinition (); + if (def == null || def.KnownTypeCode != ICSharpCode.NRefactory.TypeSystem.KnownTypeCode.String) + return; AddIssue(binaryOperatorExpression, ctx.TranslateString("Use string.IsNullOrEmpty"), script => { Expression expr = new PrimitiveType ("string").Invoke("IsNullOrEmpty", str.Clone()); if (isNegated) diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/StringIsNullOrEmptyInspectorTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/StringIsNullOrEmptyInspectorTests.cs index d256ca5682..28f6431a58 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/StringIsNullOrEmptyInspectorTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/StringIsNullOrEmptyInspectorTests.cs @@ -499,5 +499,23 @@ namespace ICSharpCode.NRefactory.CSharp.CodeIssues } }"); } + + [Test] + public void TestArrays () + { + var input = @"class Foo +{ + void Bar () + { + int[] foo = new int[10]; + if (foo == null || foo.Length == 0) { + } + } +}"; + + TestRefactoringContext context; + var issues = GetIssues (new StringIsNullOrEmptyIssue (), input, out context); + Assert.AreEqual (0, issues.Count); + } } } From 28c635f11def63e11d3961330293305dd523397a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20Kr=C3=BCger?= Date: Fri, 5 Oct 2012 09:15:56 +0200 Subject: [PATCH 24/29] [CodeIssues] Removed LocalVariableOnlyAssignedIssue - it's redundant and handled by RedundantAssignmentIssue --- .../ICSharpCode.NRefactory.CSharp.csproj | 1 - .../LocalVariableOnlyAssignedIssue.cs | 69 ----------- .../LocalVariableOnlyAssignedIssueTests.cs | 113 ------------------ .../ICSharpCode.NRefactory.Tests.csproj | 1 - 4 files changed, 184 deletions(-) delete mode 100644 ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/LocalVariableOnlyAssignedIssue.cs delete mode 100644 ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/LocalVariableOnlyAssignedIssueTests.cs diff --git a/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj b/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj index fe0c1ba882..042bddaa6d 100644 --- a/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj +++ b/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj @@ -320,7 +320,6 @@ - diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/LocalVariableOnlyAssignedIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/LocalVariableOnlyAssignedIssue.cs deleted file mode 100644 index 7b66ba463e..0000000000 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableOnlyAssignedIssues/LocalVariableOnlyAssignedIssue.cs +++ /dev/null @@ -1,69 +0,0 @@ -// -// LocalVariableOnlyAssignedIssue.cs -// -// Author: -// Mansheng Yang -// -// Copyright (c) 2012 Mansheng Yang -// -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files (the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: -// -// The above copyright notice and this permission notice shall be included in -// all copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -// THE SOFTWARE. - -using ICSharpCode.NRefactory.Semantics; - -namespace ICSharpCode.NRefactory.CSharp.Refactoring -{ - - [IssueDescription ("Local variable is only assigned", - Description = "Local variable is assigned but its value is never used", - Category = IssueCategories.CodeQualityIssues, - Severity = Severity.Warning, - IssueMarker = IssueMarker.Underline)] - public class LocalVariableOnlyAssignedIssue : VariableOnlyAssignedIssue - { - internal override GatherVisitorBase GetGatherVisitor (BaseRefactoringContext ctx) - { - return new GatherVisitor (ctx); - } - - class GatherVisitor : GatherVisitorBase - { - public GatherVisitor (BaseRefactoringContext ctx) - : base (ctx) - { - } - - public override void VisitVariableInitializer (VariableInitializer variableInitializer) - { - base.VisitVariableInitializer (variableInitializer); - - var decl = variableInitializer.Parent as VariableDeclarationStatement; - if (decl == null) - return; - - var resolveResult = ctx.Resolve (variableInitializer) as LocalResolveResult; - if (resolveResult == null) - return; - if (!TestOnlyAssigned (ctx, decl.Parent, resolveResult.Variable)) - return; - AddIssue (variableInitializer.NameToken, - ctx.TranslateString ("Local variable is assigned but its value is never used")); - } - } - } -} diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/LocalVariableOnlyAssignedIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/LocalVariableOnlyAssignedIssueTests.cs deleted file mode 100644 index 8c5ddf0ba3..0000000000 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/LocalVariableOnlyAssignedIssueTests.cs +++ /dev/null @@ -1,113 +0,0 @@ -// -// LocalVariableOnlyAssignedIssueTests.cs -// -// Author: -// Mansheng Yang -// -// Copyright (c) 2012 Mansheng Yang -// -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files (the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: -// -// The above copyright notice and this permission notice shall be included in -// all copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -// THE SOFTWARE. - -using ICSharpCode.NRefactory.CSharp.Refactoring; -using NUnit.Framework; - -namespace ICSharpCode.NRefactory.CSharp.CodeIssues -{ - [TestFixture] - public class LocalVariableOnlyAssignedIssueTests : InspectionActionTestBase - { - [Test] - public void TestUnusedValue () - { - var input1 = @" -class TestClass -{ - void TestMethod() - { - int i = 1; - i++; - } -}"; - Test (input1, 1); - - var input2 = @" -class TestClass -{ - void TestMethod() - { - int i; - i = 1; - } -}"; - Test (input2, 1); - } - - [Test] - public void TestUsedValue () - { - var input = @" -class TestClass -{ - int TestMethod() - { - int i; - i = 1; - int j = i + 1; - return j; - } -}"; - Test (input, 0); - } - - [Test] - public void TestOutArgument () - { - var input1 = @" -class TestClass -{ - void Test (out int i) - { - i = 1; - } - void TestMethod() - { - int tmp; - Test (out tmp); - } -}"; - // should not warn because ignoring out-arguments is legitimate - Test (input1, 0); - } - - [Test] - public void TestIncrement () - { - var input1 = @" -class TestClass -{ - void TestMethod() - { - int i = 1; - Test (i++); - } -}"; - Test (input1, 0); - } - } -} diff --git a/ICSharpCode.NRefactory.Tests/ICSharpCode.NRefactory.Tests.csproj b/ICSharpCode.NRefactory.Tests/ICSharpCode.NRefactory.Tests.csproj index f4296dd82b..1f3e89882d 100644 --- a/ICSharpCode.NRefactory.Tests/ICSharpCode.NRefactory.Tests.csproj +++ b/ICSharpCode.NRefactory.Tests/ICSharpCode.NRefactory.Tests.csproj @@ -110,7 +110,6 @@ - From a50b205d6b15b973e43e1b00ae9a2f1ca70de102 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20Kr=C3=BCger?= Date: Fri, 5 Oct 2012 09:45:29 +0200 Subject: [PATCH 25/29] [CodeIssues] Fixed unused parameters for methods used as delegates and event handlers. --- .../ParameterNotUsedIssue.cs | 47 +++++++++++++++++-- .../CodeIssues/ParameterNotUsedIssueTests.cs | 16 +++++++ 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableNotUsedIssues/ParameterNotUsedIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableNotUsedIssues/ParameterNotUsedIssue.cs index cb9a51d76d..adf2b0cc69 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableNotUsedIssues/ParameterNotUsedIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableNotUsedIssues/ParameterNotUsedIssue.cs @@ -28,6 +28,8 @@ using ICSharpCode.NRefactory.Semantics; using System.Linq; using ICSharpCode.NRefactory.TypeSystem; using System.Collections.Generic; +using ICSharpCode.NRefactory.CSharp.Resolver; +using System; namespace ICSharpCode.NRefactory.CSharp.Refactoring { @@ -41,15 +43,48 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring #region ICodeIssueProvider implementation public IEnumerable GetIssues(BaseRefactoringContext context) { - return new GatherVisitor (context).GetIssues (); + var delegateVisitor = new GetDelgateUsagesVisitor (context); + context.RootNode.AcceptVisitor (delegateVisitor); + + return new GatherVisitor (context, delegateVisitor).GetIssues (); } #endregion + // Collect all methods that are used as delegate + class GetDelgateUsagesVisitor : DepthFirstAstVisitor + { + BaseRefactoringContext ctx; + public readonly List UsedMethods = new List (); + + public GetDelgateUsagesVisitor(BaseRefactoringContext ctx) + { + this.ctx = ctx; + } + + public override void VisitIdentifierExpression(IdentifierExpression identifierExpression) + { + var mgr = ctx.Resolve (identifierExpression) as MethodGroupResolveResult; + if (mgr != null) + UsedMethods.AddRange (mgr.Methods); + base.VisitIdentifierExpression(identifierExpression); + } + + public override void VisitMemberReferenceExpression(MemberReferenceExpression memberReferenceExpression) + { + var mgr = ctx.Resolve (memberReferenceExpression) as MethodGroupResolveResult; + if (mgr != null) + UsedMethods.AddRange (mgr.Methods); + base.VisitMemberReferenceExpression(memberReferenceExpression); + } + } + class GatherVisitor : GatherVisitorBase { - public GatherVisitor (BaseRefactoringContext ctx) + GetDelgateUsagesVisitor usedDelegates; + public GatherVisitor (BaseRefactoringContext ctx, GetDelgateUsagesVisitor usedDelegates) : base (ctx) { + this.usedDelegates = usedDelegates; } public override void VisitMethodDeclaration(MethodDeclaration methodDeclaration) @@ -66,13 +101,15 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring return; if (member.ImplementedInterfaceMembers.Any ()) return; - - base.VisitMethodDeclaration(methodDeclaration); + if (usedDelegates.UsedMethods.Any (m => m.Region.Begin == methodDeclaration.StartLocation)) + return; + foreach (var parameter in methodDeclaration.Parameters) + parameter.AcceptVisitor (this); } public override void VisitParameterDeclaration (ParameterDeclaration parameterDeclaration) { - base.VisitParameterDeclaration (parameterDeclaration); + base.VisitParameterDeclaration (parameterDeclaration); if (!(parameterDeclaration.Parent is MethodDeclaration)) return; diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterNotUsedIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterNotUsedIssueTests.cs index e939c3cfa0..5b47df9c4e 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterNotUsedIssueTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterNotUsedIssueTests.cs @@ -114,5 +114,21 @@ class TestClass { }"; Test (input, 0); } + + + [Test] + public void TestMethodUsedAsDelegateMethod () + { + var input = @"using System; +class TestClass { + public event EventHandler FooEvt; + void TestMethod () + { + FooEvt += FooBar; + } + void FooBar (object sender, EventArgs e) {} +}"; + Test (input, 0); + } } } From fe815dc2a05c27ef5224a4253bd0840b9120c69b Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Fri, 5 Oct 2012 02:09:09 +0200 Subject: [PATCH 26/29] Fixed false positive that caused a 'is operator always returns true' warning even though it returns false. (e.g. 'intVar is double') --- .../ExpressionIsAlwaysOfProvidedTypeIssue.cs | 9 ++++++++- .../ExpressionIsAlwaysOfProvidedTypeIssueTests.cs | 14 ++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsAlwaysOfProvidedTypeIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsAlwaysOfProvidedTypeIssue.cs index 50770b0379..4fbe64f46e 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsAlwaysOfProvidedTypeIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsAlwaysOfProvidedTypeIssue.cs @@ -27,6 +27,7 @@ using System.Collections.Generic; using ICSharpCode.NRefactory.CSharp.Resolver; using ICSharpCode.NRefactory.Semantics; +using ICSharpCode.NRefactory.TypeSystem; namespace ICSharpCode.NRefactory.CSharp.Refactoring { @@ -59,7 +60,7 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring var providedType = ctx.ResolveType (isExpression.Type); var foundConversion = conversions.ImplicitConversion(type, providedType); - if (!foundConversion.IsValid) + if (!IsValidReferenceOrBoxingConversion(type, providedType)) return; var action = new CodeAction (ctx.TranslateString ("Compare with 'null'"), @@ -68,6 +69,12 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring AddIssue (isExpression, ctx.TranslateString ("Given expression is always of the provided type. " + "Consider comparing with 'null' instead"), new [] { action }); } + + bool IsValidReferenceOrBoxingConversion(IType fromType, IType toType) + { + Conversion c = conversions.ImplicitConversion(fromType, toType); + return c.IsValid && (c.IsIdentityConversion || c.IsReferenceConversion || c.IsBoxingConversion); + } } } } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ExpressionIsAlwaysOfProvidedTypeIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ExpressionIsAlwaysOfProvidedTypeIssueTests.cs index bd7db047ff..b230d249ae 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ExpressionIsAlwaysOfProvidedTypeIssueTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ExpressionIsAlwaysOfProvidedTypeIssueTests.cs @@ -90,5 +90,19 @@ class TestClass }"; Test (input, 1, output); } + + [Test] + public void IntIsNotDouble () + { + var input = @" +sealed class TestClass +{ + void TestMethod (int x) + { + if (x is double) ; + } +}"; + Test (input, 0); + } } } From e6bc300e1eeb3378a7c71411a202623d385f3877 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Fri, 5 Oct 2012 02:23:55 +0200 Subject: [PATCH 27/29] Don't show "expression is never of the provided type" warning if the type could not be resolved. --- .../ExpressionIsNeverOfProvidedTypeIssue.cs | 2 ++ ...pressionIsNeverOfProvidedTypeIssueTests.cs | 28 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsNeverOfProvidedTypeIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsNeverOfProvidedTypeIssue.cs index ed10a12d8f..543b2bdb61 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsNeverOfProvidedTypeIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ExpressionIsNeverOfProvidedTypeIssue.cs @@ -61,6 +61,8 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring var exprType = ctx.Resolve (isExpression.Expression).Type; var providedType = ctx.ResolveType (isExpression.Type); + if (exprType.Kind == TypeKind.Unknown || providedType.Kind == TypeKind.Unknown) + return; if (IsValidReferenceOrBoxingConversion(exprType, providedType)) return; diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ExpressionIsNeverOfProvidedTypeIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ExpressionIsNeverOfProvidedTypeIssueTests.cs index e3c006fe2f..1270a20c3f 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ExpressionIsNeverOfProvidedTypeIssueTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ExpressionIsNeverOfProvidedTypeIssueTests.cs @@ -270,6 +270,34 @@ sealed class TestClass { if (x is string[]) ; } +}"; + Test (input, 0); + } + + [Test] + public void UnknownExpression() + { + var input = @" +sealed class TestClass +{ + void TestMethod () + { + if (unknown is string) ; + } +}"; + Test (input, 0); + } + + [Test] + public void UnknownType() + { + var input = @" +sealed class TestClass +{ + void TestMethod (int x) + { + if (x is unknown) ; + } }"; Test (input, 0); } From 99bbb11ded6e075e5d98a5df04d18c4f9bdfa7d4 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Fri, 5 Oct 2012 14:23:02 +0200 Subject: [PATCH 28/29] Add 'ResultOfAsyncCallShouldNotBeIgnored' issue. --- .../ICSharpCode.NRefactory.CSharp.csproj | 1 + ...esultOfAsyncCallShouldNotBeIgnoredIssue.cs | 58 +++++++++++++++++++ .../Implementation/KnownTypeCache.cs | 5 +- 3 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ResultOfAsyncCallShouldNotBeIgnoredIssue.cs diff --git a/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj b/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj index 042bddaa6d..47c1b00f18 100644 --- a/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj +++ b/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj @@ -316,6 +316,7 @@ + diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ResultOfAsyncCallShouldNotBeIgnoredIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ResultOfAsyncCallShouldNotBeIgnoredIssue.cs new file mode 100644 index 0000000000..48dc4e9285 --- /dev/null +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ResultOfAsyncCallShouldNotBeIgnoredIssue.cs @@ -0,0 +1,58 @@ +// Copyright (c) AlphaSierraPapa for the SharpDevelop Team +// +// Permission is hereby granted, free of charge, to any person obtaining a copy of this +// software and associated documentation files (the "Software"), to deal in the Software +// without restriction, including without limitation the rights to use, copy, modify, merge, +// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons +// to whom the Software is furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all copies or +// substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, +// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR +// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE +// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +// DEALINGS IN THE SOFTWARE. + +using System; +using System.Collections.Generic; +using ICSharpCode.NRefactory.Semantics; +using ICSharpCode.NRefactory.TypeSystem; + +namespace ICSharpCode.NRefactory.CSharp.Refactoring +{ + [IssueDescription("Result of async call is ignored", + Description = "Warns when the task returned by an async call is ignored, which causes exceptions" + + " thrown by the call to be silently ignored.", + Category = IssueCategories.CodeQualityIssues, + Severity = Severity.Warning)] + public class ResultOfAsyncCallShouldNotBeIgnoredIssue : ICodeIssueProvider + { + public IEnumerable GetIssues(BaseRefactoringContext context) + { + return new GatherVisitor().GetIssues(); + } + + sealed class GatherVisitor : GatherVisitorBase + { + public GatherVisitor(BaseRefactoringContext ctx) + : base(ctx) + { + } + + public override void VisitExpressionStatement(ExpressionStatement expressionStatement) + { + base.VisitExpressionStatement(expressionStatement); + var invocation = expressionStatement.Expression as InvocationExpression; + if (invocation == null) + return; + var rr = ctx.Resolve(invocation) as InvocationResolveResult; + if (rr != null && (rr.Type.IsKnownType(KnownTypeCode.Task) || rr.Type.IsKnownType(KnownTypeCode.TaskOfT))) { + AddIssue(invocation, "Exceptions in async call will be silently ignored because the returned task is unused"); + } + } + } + } +} diff --git a/ICSharpCode.NRefactory/TypeSystem/Implementation/KnownTypeCache.cs b/ICSharpCode.NRefactory/TypeSystem/Implementation/KnownTypeCache.cs index adcd1a8e34..c28e553a5e 100644 --- a/ICSharpCode.NRefactory/TypeSystem/Implementation/KnownTypeCache.cs +++ b/ICSharpCode.NRefactory/TypeSystem/Implementation/KnownTypeCache.cs @@ -48,12 +48,13 @@ namespace ICSharpCode.NRefactory.TypeSystem.Implementation KnownTypeReference typeRef = KnownTypeReference.Get(typeCode); if (typeRef == null) return SpecialType.UnknownType; + var typeName = new TopLevelTypeName(typeRef.Namespace, typeRef.Name, typeRef.TypeParameterCount); foreach (IAssembly asm in compilation.Assemblies) { - var typeDef = asm.GetTypeDefinition(new TopLevelTypeName(typeRef.Namespace, typeRef.Name, typeRef.TypeParameterCount)); + var typeDef = asm.GetTypeDefinition(typeName); if (typeDef != null) return typeDef; } - return new UnknownType(typeRef.Namespace, typeRef.Name, typeRef.TypeParameterCount); + return new UnknownType(typeName); } } } From 536b9b0b1cbed852886ac6bf63f44976d7c00d8f Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Fri, 5 Oct 2012 15:07:56 +0200 Subject: [PATCH 29/29] Add issue provider for missing StringComparison argument to string.StartsWith/EndsWith/IndexOf/LastIndexOf calls. --- .../ICSharpCode.NRefactory.CSharp.csproj | 1 + .../MissingStringComparisonIssue.cs | 106 ++++++++++++++++++ ...esultOfAsyncCallShouldNotBeIgnoredIssue.cs | 2 +- .../MissingStringComparisonIssueTests.cs | 93 +++++++++++++++ .../ICSharpCode.NRefactory.Tests.csproj | 1 + 5 files changed, 202 insertions(+), 1 deletion(-) create mode 100644 ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/MissingStringComparisonIssue.cs create mode 100644 ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/MissingStringComparisonIssueTests.cs diff --git a/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj b/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj index 47c1b00f18..8d788bbac9 100644 --- a/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj +++ b/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj @@ -301,6 +301,7 @@ + diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/MissingStringComparisonIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/MissingStringComparisonIssue.cs new file mode 100644 index 0000000000..b037f8e2d1 --- /dev/null +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/MissingStringComparisonIssue.cs @@ -0,0 +1,106 @@ +// +// MissingStringComparisonIssue.cs +// +// Author: +// Daniel Grunwald +// +// Copyright (c) 2012 Daniel Grunwald +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +using System; +using System.Collections.Generic; +using System.Linq; +using ICSharpCode.NRefactory.Semantics; +using ICSharpCode.NRefactory.TypeSystem; +using ICSharpCode.NRefactory.TypeSystem.Implementation; + +namespace ICSharpCode.NRefactory.CSharp.Refactoring +{ + [IssueDescription("Missing StringComparison argument", + Description = "Warns when a culture-aware comparison is used by default.", + Category = IssueCategories.CodeQualityIssues, + Severity = Severity.Warning)] + public class MissingStringComparisonIssue : ICodeIssueProvider + { + public IEnumerable GetIssues(BaseRefactoringContext context) + { + return new GatherVisitor(context).GetIssues(); + } + + class GatherVisitor : GatherVisitorBase + { + public GatherVisitor(BaseRefactoringContext ctx) : base(ctx) + { + } + + public override void VisitInvocationExpression(InvocationExpression invocationExpression) + { + base.VisitInvocationExpression(invocationExpression); + + MemberReferenceExpression mre = invocationExpression.Target as MemberReferenceExpression; + if (mre == null) + return; + switch (mre.MemberName) { + case "StartsWith": + case "EndsWith": + if (invocationExpression.Arguments.Count != 1) + return; + break; + case "IndexOf": + case "LastIndexOf": + break; + default: + return; + } + + var rr = ctx.Resolve(invocationExpression) as InvocationResolveResult; + if (rr == null || rr.IsError) { + // Not an invocation resolve result - e.g. could be a UnknownMemberResolveResult instead + return; + } + if (!(rr.Member.DeclaringTypeDefinition != null && rr.Member.DeclaringTypeDefinition.KnownTypeCode == KnownTypeCode.String)) { + // Not a string operation + return; + } + IParameter firstParameter = rr.Member.Parameters.FirstOrDefault(); + if (firstParameter == null || !firstParameter.Type.IsKnownType(KnownTypeCode.String)) + return; // First parameter not a string + IParameter lastParameter = rr.Member.Parameters.Last(); + if (lastParameter.Type.Name == "StringComparison") + return; // already specifying a string comparison + AddIssue(invocationExpression.LParToken.StartLocation, invocationExpression.RParToken.EndLocation, + mre.MemberName + "() call is missing StringComparison argument", + new [] { + new CodeAction("Use ordinal comparison", script => AddArgument(script, invocationExpression, "Ordinal")), + new CodeAction("Use culture-aware comparison", script => AddArgument(script, invocationExpression, "CurrentCulture")), + }); + } + + void AddArgument(Script script, InvocationExpression invocationExpression, string stringComparison) + { + var astBuilder = ctx.CreateTypeSytemAstBuilder(invocationExpression); + var newArgument = astBuilder.ConvertType(new TopLevelTypeName("System", "StringComparison")).Member(stringComparison); + var copy = (InvocationExpression)invocationExpression.Clone(); + copy.Arguments.Add(newArgument); + script.Replace(invocationExpression, copy); + } + } + } +} diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ResultOfAsyncCallShouldNotBeIgnoredIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ResultOfAsyncCallShouldNotBeIgnoredIssue.cs index 48dc4e9285..d4a40e3f2b 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ResultOfAsyncCallShouldNotBeIgnoredIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ResultOfAsyncCallShouldNotBeIgnoredIssue.cs @@ -32,7 +32,7 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring { public IEnumerable GetIssues(BaseRefactoringContext context) { - return new GatherVisitor().GetIssues(); + return new GatherVisitor(context).GetIssues(); } sealed class GatherVisitor : GatherVisitorBase diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/MissingStringComparisonIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/MissingStringComparisonIssueTests.cs new file mode 100644 index 0000000000..25fcae8768 --- /dev/null +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/MissingStringComparisonIssueTests.cs @@ -0,0 +1,93 @@ +// +// Author: +// Daniel Grunwald +// +// Copyright (c) 2012 Daniel Grunwald +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +using System; +using ICSharpCode.NRefactory.CSharp.Refactoring; +using NUnit.Framework; + +namespace ICSharpCode.NRefactory.CSharp.CodeIssues +{ + [TestFixture] + public class MissingStringComparisonIssueTests : InspectionActionTestBase + { + const string stringIndexOfStringCalls = @"using System; +using System.Collections.Generic; +class Test { + public void StringIndexOfStringCalls(List list) + { + list[0].IndexOf("".com""); + list[0].IndexOf("".com"", 0); + list[0].IndexOf("".com"", 0, 5); + list[0].IndexOf(list[1], 0, 10); + } +}"; + const string stringIndexOfStringCallsWithComparison = @"using System; +using System.Collections.Generic; +class Test { + public void StringIndexOfStringCalls(List list) + { + list [0].IndexOf ("".com"", StringComparison.Ordinal); + list [0].IndexOf ("".com"", 0, StringComparison.Ordinal); + list [0].IndexOf ("".com"", 0, 5, StringComparison.Ordinal); + list [0].IndexOf (list [1], 0, 10, StringComparison.Ordinal); + } +}"; + + [Test] + public void IndexOfStringCalls() + { + Test(stringIndexOfStringCalls, 4, stringIndexOfStringCallsWithComparison); + } + + [Test] + public void IndexOfStringCallsAlreadyWithComparison() + { + Test(stringIndexOfStringCallsWithComparison, 0); + } + + [Test] + public void StringIndexOfChar() + { + string program = @"using System; +class Test { + void M(string text) { + text.IndexOf('.'); + } +}"; + Test(program, 0); + } + + [Test] + public void ListIndexOf() + { + string program = @"using System.Collections.Generic; +class Test { + void M(List list) { + list.IndexOf("".com""); + } +}"; + Test(program, 0); + } + } +} diff --git a/ICSharpCode.NRefactory.Tests/ICSharpCode.NRefactory.Tests.csproj b/ICSharpCode.NRefactory.Tests/ICSharpCode.NRefactory.Tests.csproj index 1f3e89882d..b68e0e5343 100644 --- a/ICSharpCode.NRefactory.Tests/ICSharpCode.NRefactory.Tests.csproj +++ b/ICSharpCode.NRefactory.Tests/ICSharpCode.NRefactory.Tests.csproj @@ -112,6 +112,7 @@ +