Author: spouliot
Date: 2008-01-24 18:40:30 -0500 (Thu, 24 Jan 2008)
New Revision: 93863
Removed:
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ImplementingEqualsButNotGetHashCodeAndViceVersaRule.cs
Modified:
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ChangeLog
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/Gendarme.Rules.BadPractice.xml.in
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/Makefile.am
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ToStringReturnsNullRule.cs
Log:
2008-01-24 Sebastien Pouliot <[EMAIL PROTECTED]>
* ImplementingEqualsButNotGetHashCodeAndViceVersaRule.cs: Rule
moves (with tests) into Design since it affects the API.
* ToStringReturnsNullRule.cs: Simplification to avoid uncommon cases
that added *LOTS* of false positives while still missing some
real defects. Now a rock-ified ITypeRule with much lower memory
requirements.
* Gendarme.Rules.BadPractice.xml.in: Remove (moved) definition.
* Makefile.am: Remove (moved) rule and tests
Modified: trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ChangeLog
===================================================================
--- trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ChangeLog
2008-01-24 23:36:42 UTC (rev 93862)
+++ trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ChangeLog
2008-01-24 23:40:30 UTC (rev 93863)
@@ -1,3 +1,14 @@
+2008-01-24 Sebastien Pouliot <[EMAIL PROTECTED]>
+
+ * ImplementingEqualsButNotGetHashCodeAndViceVersaRule.cs: Rule
+ moves (with tests) into Design since it affects the API.
+ * ToStringReturnsNullRule.cs: Simplification to avoid uncommon cases
+ that added *LOTS* of false positives while still missing some
+ real defects. Now a rock-ified ITypeRule with much lower memory
+ requirements.
+ * Gendarme.Rules.BadPractice.xml.in: Remove (moved) definition.
+ * Makefile.am: Remove (moved) rule and tests
+
2008-01-23 Sebastien Pouliot <[EMAIL PROTECTED]>
* ConstructorShouldNotCallVirtualMethodsRule.cs: Add recursive
Modified:
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/Gendarme.Rules.BadPractice.xml.in
===================================================================
---
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/Gendarme.Rules.BadPractice.xml.in
2008-01-24 23:36:42 UTC (rev 93862)
+++
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/Gendarme.Rules.BadPractice.xml.in
2008-01-24 23:40:30 UTC (rev 93863)
@@ -1,10 +1,4 @@
<rules>
- <rule Name="ImplementingEqualsButNotGetHashCodeAndViceVersaRule"
-
Type="Gendarme.Rules.BadPractice.ImplementingEqualsButNotGetHashCodeAndViceVersaRule,
Gendarme.Rules.BadPractice, [EMAIL PROTECTED]@, Culture=neutral,
PublicKeyToken=null"
-
Uri="http://groups.google.com/group/gendarme/web/rules-documentation#ImplementingEqualsButNotGetHashCodeAndViceVersaRule";
>
- <problem>Only one method of Object.Equals (Object) and
Object.GetHashCode () is implemented in '{0}'.</problem>
- <solution>Implement the other method also.</solution>
- </rule>
<rule Name="CloneMethodShouldNotReturnNullRule"
Type="Gendarme.Rules.BadPractice.CloneMethodShouldNotReturnNullRule,
Gendarme.Rules.BadPractice, [EMAIL PROTECTED]@, Culture=neutral,
PublicKeyToken=null"
Uri="http://groups.google.com/group/gendarme/web/rules-documentation#CloneMethodShouldNotReturnNullRule";
>
Deleted:
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ImplementingEqualsButNotGetHashCodeAndViceVersaRule.cs
===================================================================
---
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ImplementingEqualsButNotGetHashCodeAndViceVersaRule.cs
2008-01-24 23:36:42 UTC (rev 93862)
+++
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ImplementingEqualsButNotGetHashCodeAndViceVersaRule.cs
2008-01-24 23:40:30 UTC (rev 93863)
@@ -1,75 +0,0 @@
-//
-//
Gendarme.Rules.BadPractice.ImplementingEqualsButNotGetHashCodeAndViceVersaRule
-//
-// Authors:
-// Nidhi Rawal <[EMAIL PROTECTED]>
-//
-// Copyright (c) <2007> Nidhi Rawal
-//
-// 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 Mono.Cecil;
-
-using Gendarme.Framework;
-
-namespace Gendarme.Rules.BadPractice {
-
- public class ImplementingEqualsButNotGetHashCodeAndViceVersaRule:
ITypeRule {
-
- bool equals = false;
- bool getHashCode = false;
-
- private void EqualsOrGetHashCode (TypeDefinition type)
- {
- equals = false;
- getHashCode = false;
-
- foreach (MethodDefinition method in type.Methods) {
- if (method.Name == "Equals" &&
method.Parameters.Count == 1)
- equals = true;
- else if (method.Name == "GetHashCode" &&
method.Parameters.Count == 0)
- getHashCode = true;
- }
- }
-
- public MessageCollection CheckType (TypeDefinition type, Runner
runner)
- {
- EqualsOrGetHashCode (type);
-
- // if we have Equals but no GetHashCode method
- if (equals && !getHashCode) {
- Location location = new Location (type);
- Message message = new Message ("Implements
Object.Equals (Object) but does not implement Object.GetHashCode ()", location,
MessageType.Error);
- return new MessageCollection (message);
- }
-
- // if we have GetHashCode but no Equals method
- if (!equals && getHashCode) {
- Location location = new Location (type);
- Message message = new Message ("Implements
Object.GetHashCode () but does not implement Object.Equals (Object)", location,
MessageType.Error);
- return new MessageCollection (message);
- }
-
- // we either have both Equals and GetHashCode or none
(both fine)
- return runner.RuleSuccess;
- }
- }
-}
Modified: trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/Makefile.am
===================================================================
--- trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/Makefile.am
2008-01-24 23:36:42 UTC (rev 93862)
+++ trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/Makefile.am
2008-01-24 23:40:30 UTC (rev 93863)
@@ -10,8 +10,7 @@
badpractice_rules_sources_in = ../../AssemblyInfo.cs.in
badpractice_rules_generated_sources = $(badpractice_rules_sources_in:.in=)
badpractice_rules_sources = CloneMethodShouldNotReturnNullRule.cs
EqualShouldHandleNullArgRule.cs \
- ImplementingEqualsButNotGetHashCodeAndViceVersaRule.cs
ToStringReturnsNullRule.cs GetEntryAssemblyMayReturnNullRule.cs \
- ConstructorShouldNotCallVirtualMethodsRule.cs
+ ToStringReturnsNullRule.cs GetEntryAssemblyMayReturnNullRule.cs
ConstructorShouldNotCallVirtualMethodsRule.cs
badpractice_rules_build_sources = $(addprefix $(srcdir)/,
$(badpractice_rules_sources))
badpractice_rules_build_sources += $(badpractice_rules_generated_sources)
@@ -35,8 +34,7 @@
done;
badpractice_test_sources = CloneMethodShouldNotReturnNullTest.cs
EqualShouldHandleNullArgTest.cs \
- ImplementingEqualsButNotGetHashCodeAndViceVersaTest.cs
ToStringReturnsNullTest.cs GetEntryAssemblyMayReturnNullTest.cs \
- ConstructorShouldNotCallVirtualMethodsTest.cs
+ ToStringReturnsNullTest.cs GetEntryAssemblyMayReturnNullTest.cs
ConstructorShouldNotCallVirtualMethodsTest.cs
badpractice_test_build_sources = $(addprefix $(srcdir)/Test/,
$(badpractice_test_sources))
Modified:
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ToStringReturnsNullRule.cs
===================================================================
---
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ToStringReturnsNullRule.cs
2008-01-24 23:36:42 UTC (rev 93862)
+++
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ToStringReturnsNullRule.cs
2008-01-24 23:40:30 UTC (rev 93863)
@@ -3,8 +3,10 @@
//
// Authors:
// Nidhi Rawal <[EMAIL PROTECTED]>
+// Sebastien Pouliot <[EMAIL PROTECTED]>
//
// Copyright (c) <2007> Nidhi Rawal
+// Copyright (C) 2008 Novell, Inc (http://www.novell.com)
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to
deal
@@ -29,134 +31,74 @@
using Mono.Cecil;
using Mono.Cecil.Cil;
+
using Gendarme.Framework;
+using Gendarme.Framework.Rocks;
namespace Gendarme.Rules.BadPractice {
- public class ToStringReturnsNullRule: IMethodRule {
+ public class ToStringReturnsNullRule: ITypeRule {
- private static bool IsOverridenToString (MethodDefinition
method)
+ public MessageCollection CheckType (TypeDefinition type, Runner
runner)
{
- return method.Name == "ToString" && method.IsVirtual;
- }
-
- public MessageCollection CheckMethod (MethodDefinition method,
Runner runner)
- {
- if (!method.HasBody)
+ // rules applies to types that overrides
System.Object.Equals(object)
+ MethodDefinition method = type.GetMethod
(MethodSignatures.ToString);
+ if ((method == null) || !method.HasBody)
return runner.RuleSuccess;
- if (!IsOverridenToString (method))
+ // rule applies
+
+ if (!CheckIfMethodReturnsNull (method))
return runner.RuleSuccess;
- int offset = 0;
- bool nullReturned = false;
- foreach (Instruction ins in method.Body.Instructions) {
- if (ins.OpCode == OpCodes.Ret) {
- Instruction prevIns = ins.Previous;
- if (prevIns.OpCode != OpCodes.Ldnull) {
- string opCodeSt;
- if (prevIns.OpCode ==
OpCodes.Call || prevIns.OpCode == OpCodes.Callvirt)
- opCodeSt = ReturnSt
(method, prevIns.Previous);
- else
- opCodeSt = ReturnSt
(method, prevIns);
- if (opCodeSt == null &&
(prevIns.Operand == null || prevIns.Operand.ToString () != "System.String
System.Convert::ToString(System.Object)"))
- nullReturned = true;
- if (MethodReturnsNull (method,
opCodeSt)) {
- nullReturned = true;
- offset = ins.Offset;
- }
- }
- else {
- nullReturned = true;
- offset = ins.Offset;
- }
- }
- }
-
- if (nullReturned) {
- Location location = new Location (method,
offset);
- Message message = new Message ("ToString ()
seems to returns null in some condition", location, MessageType.Error);
- return new MessageCollection (message);
- }
-
- return runner.RuleSuccess;
+ // FIXME: location
+ Location location = new Location (method, -1);
+ Message message = new Message ("Method 'ToString()'
seems to returns null under some conditions.", location, MessageType.Error);
+ return new MessageCollection (message);
}
- private static string ReturnSt (MethodDefinition method,
Instruction instruc)
+ private static bool CheckIfMethodReturnsNull (MethodDefinition
method)
{
- Hashtable opCodes = InitializeHashTable (method);
- string stOpCode = null;
- switch (instruc.OpCode.Code) {
- case Code.Ldloc_0:
- stOpCode = opCodes.ContainsKey (0)? opCodes
[0].ToString () : null;
- break;
- case Code.Ldloc_1:
- stOpCode = opCodes.ContainsKey (1)? opCodes
[1].ToString () : null;
- break;
- case Code.Ldloc_2:
- stOpCode = opCodes.ContainsKey (2)? opCodes
[2].ToString () : null;
- break;
- case Code.Ldloc_3:
- stOpCode = opCodes.ContainsKey (3)? opCodes
[3].ToString () : null;
- break;
- case Code.Ldloc_S:
- string [] s = instruc.Operand.ToString ().Split
('_');
- int i = Convert.ToInt32 (s [1]);
- stOpCode = opCodes.ContainsKey (i) ? opCodes
[i].ToString () : null;
- break;
- case Code.Ldfld:
- TypeDefinition type = (TypeDefinition)
method.DeclaringType;
- foreach (MethodDefinition ctor in
type.Constructors)
- foreach (Instruction ctorins in
ctor.Body.Instructions)
- if (ctorins.Operand !=null &&
ctorins.Operand.ToString () == instruc.Operand.ToString ())
- stOpCode =
ctorins.Operand.ToString ();
- break;
- case Code.Ldsfld:
- stOpCode = String.Empty;
- break;
- }
- return stOpCode;
- }
-
- private static Hashtable InitializeHashTable (MethodDefinition
method)
- {
- int count = 0;
- Hashtable hash = new Hashtable ();
-
foreach (Instruction ins in method.Body.Instructions) {
- if (ins.OpCode.Code.ToString().Substring(0,2)
== "St") {
- if (ins.OpCode.Code == Code.Stloc_S) {
- hash.Add (count,
ins.Operand.ToString ());
- count ++;
+ switch (ins.OpCode.Code) {
+ case Code.Ret:
+ switch (ins.Previous.OpCode.Code) {
+ case Code.Ldarg_0:
+ // very special case where
String.ToString() return "this"
+ return false;
+ case Code.Ldloc_0:
+ case Code.Ldloc_1:
+ case Code.Ldloc_2:
+ case Code.Ldloc_S:
+ // FIXME: we could detect some
NULL in there
+ break;
+ case Code.Newobj:
+ // we're sure it's not null,
e.g. new string ('!', 2)
+ return false;
+ case Code.Ldnull:
+ // we're sure it's null
+ return true;
+ case Code.Ldstr:
+ return (ins.Previous.Operand ==
null);
+ case Code.Ldfld:
+ case Code.Ldsfld:
+ // we could be sure for
read-only fields with
+ // if
((ins.Previous.Operand as FieldDefinition).IsInitOnly)
+ // but since it's unlikely we
can track null in them...
+ // ...better not return false
positives
+ return false;
+ case Code.Call:
+ case Code.Callvirt:
+ // note: calling other
ToString() is safe since the rule applies to them too
+ // FIXME: we could check if the
method can possibly return a null string
+ break;
+ default:
+ throw new NotSupportedException
(String.Format ("{0} inside {1}", ins.Previous.OpCode.Code, method));
}
- else {
- hash.Add (count,
ins.OpCode.Code.ToString ());
- count ++;
- }
+ break;
}
}
- return hash;
+ return false;
}
-
- public bool MethodReturnsNull (MethodDefinition method, string
code)
- {
- bool isNull = false;
- Instruction prevInstruc;
- foreach (Instruction ins in method.Body.Instructions) {
- prevInstruc = ins.Previous;
- if (ins.Operand != null && ins.Operand.ToString
() == null && ins.OpCode == OpCodes.Stfld)
- isNull = false;
- else if (ins.OpCode.Code.ToString () == code ||
(ins.OpCode.ToString ().StartsWith ("st") && ins.Operand !=null &&
ins.Operand.ToString () == code)) {
- if (prevInstruc.OpCode.Code ==
Code.Ldnull)
- isNull = true;
- else if (prevInstruc.OpCode.Code ==
Code.Call || prevInstruc.OpCode.Code == Code.Callvirt) {
- code = ReturnSt (method,
prevInstruc.Previous);
- isNull = MethodReturnsNull
(method, code);
- }
- }
- }
- return isNull;
- }
-
}
}
_______________________________________________
Mono-patches maillist - [email protected]
http://lists.ximian.com/mailman/listinfo/mono-patches
