Author: spouliot
Date: 2008-01-23 19:02:07 -0500 (Wed, 23 Jan 2008)
New Revision: 93746
Modified:
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ChangeLog
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ConstructorShouldNotCallVirtualMethodsRule.cs
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/EqualShouldHandleNullArgRule.cs
Log:
2008-01-23 Sebastien Pouliot <[EMAIL PROTECTED]>
* ConstructorShouldNotCallVirtualMethodsRule.cs: Add recursive
checks for non-virtual calls (as they can lead to virtual calls). Add
check for final methods. Add check for calling 'this' sub-classes
(versus calling them on other instances, other types)
* EqualShouldHandleNullArgRule.cs: Change to ITypeRule (and locate
Equals) and to use rocks. Reimplemented HandlesNullArg based on more
extensive unit tests. False positives are mostly history :)
Modified: trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ChangeLog
===================================================================
--- trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ChangeLog
2008-01-24 00:01:57 UTC (rev 93745)
+++ trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ChangeLog
2008-01-24 00:02:07 UTC (rev 93746)
@@ -1,3 +1,13 @@
+2008-01-23 Sebastien Pouliot <[EMAIL PROTECTED]>
+
+ * ConstructorShouldNotCallVirtualMethodsRule.cs: Add recursive
+ checks for non-virtual calls (as they can lead to virtual calls). Add
+ check for final methods. Add check for calling 'this' sub-classes
+ (versus calling them on other instances, other types)
+ * EqualShouldHandleNullArgRule.cs: Change to ITypeRule (and locate
+ Equals) and to use rocks. Reimplemented HandlesNullArg based on more
+ extensive unit tests. False positives are mostly history :)
+
2008-01-14 Sebastien Pouliot <[EMAIL PROTECTED]>
* ConstructorShouldNotCallVirtualMethodsRule.cs: New. Rule to
Modified:
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ConstructorShouldNotCallVirtualMethodsRule.cs
===================================================================
---
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ConstructorShouldNotCallVirtualMethodsRule.cs
2008-01-24 00:01:57 UTC (rev 93745)
+++
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/ConstructorShouldNotCallVirtualMethodsRule.cs
2008-01-24 00:02:07 UTC (rev 93746)
@@ -3,8 +3,10 @@
//
// Authors:
// Daniel Abramov <[EMAIL PROTECTED]>
+// Sebastien Pouliot <[EMAIL PROTECTED]>
//
// Copyright (C) 2008 Daniel Abramov
+// 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
@@ -26,6 +28,7 @@
using System;
using System.Collections.Generic;
+using System.Linq;
using Mono.Cecil;
using Mono.Cecil.Cil;
@@ -37,19 +40,20 @@
public class ConstructorShouldNotCallVirtualMethodsRule : ITypeRule {
- public MessageCollection CheckType (TypeDefinition
typeDefinition, Runner runner)
+ public MessageCollection CheckType (TypeDefinition type, Runner
runner)
{
// sealed classes are ok
- if (typeDefinition.IsSealed)
+ if (type.IsSealed)
return runner.RuleSuccess;
- // no constructors - ok
- if (typeDefinition.Constructors.Count == 0)
- return runner.RuleSuccess;
+ MessageCollection messages = runner.RuleSuccess;
- MessageCollection messages = runner.RuleSuccess;
// check each constructor
- foreach (MethodDefinition constructor in
typeDefinition.Constructors) {
+ foreach (MethodDefinition constructor in
type.Constructors) {
+ // early checks to avoid stack creation
+ if (constructor.IsStatic ||
!constructor.HasBody)
+ continue;
+
CheckConstructor (constructor, ref messages);
}
return messages;
@@ -57,25 +61,97 @@
private static void CheckConstructor (MethodDefinition
constructor, ref MessageCollection messages)
{
- if (!constructor.HasBody)
+ Stack<string> stack = new Stack<string> ();
+ CheckMethod (constructor, ref messages, stack);
+ }
+
+ private static bool IsSubsclass (TypeReference sub,
TypeReference type)
+ {
+ while (type != null) {
+ if (sub == type)
+ return true;
+ TypeDefinition td = (type as TypeDefinition);
+ type = (td == null) ? null : td.BaseType;
+ }
+ return false;
+ }
+
+ private static bool IsCallFromInstance (Instruction ins, int
parameters)
+ {
+ while ((ins != null) && (parameters >= 0)) {
+ if ((ins.OpCode.Code == Code.Ldarg_0) &&
(parameters == 0))
+ return true; // this
+
+ switch (ins.OpCode.StackBehaviourPush) {
+ case StackBehaviour.Push0:
+ case StackBehaviour.Push1:
+ case StackBehaviour.Push1_push1:
+ case StackBehaviour.Pushi:
+ case StackBehaviour.Pushi8:
+ case StackBehaviour.Pushr4:
+ case StackBehaviour.Pushr8:
+ case StackBehaviour.Pushref:
+ parameters--;
+ break;
+ case StackBehaviour.Varpush:
+ // call[i|virt]
+ MethodReference mr = (ins.Operand as
MethodReference);
+ if (mr != null) {
+ if (mr.HasThis)
+ parameters++;
+ parameters +=
mr.Parameters.Count;
+ if
(mr.ReturnType.ReturnType.FullName != "System.Void")
+ parameters--;
+ }
+ break;
+ }
+ ins = ins.Previous;
+ }
+ return false;
+ }
+
+ private static void CheckMethod (MethodDefinition method, ref
MessageCollection messages, Stack<string> stack)
+ {
+ if (!method.HasBody)
return;
+ string method_name = method.ToString ();
+ // check to avoid constructors calling recursive methods
+ if (stack.Contains (method_name))
+ return;
+
// check constructor for virtual method calls
- foreach (Instruction current in
constructor.Body.Instructions) {
+ foreach (Instruction current in
method.Body.Instructions) {
switch (current.OpCode.Code) {
- // not sure what instruction will be used
- // so better check them all
case Code.Call:
case Code.Calli:
case Code.Callvirt:
+ // we recurse into normal calls since
they might be calling virtual methods
MethodDefinition md = (current.Operand
as MethodDefinition);
- if (md != null && md.HasThis &&
md.IsVirtual) {
- Location loc = new Location
(constructor, current.Offset);
- Message msg = new Message
("Calling a virtual method from the constructor of a non-sealed class is a bad
practice.", loc, MessageType.Error);
+ if (md == null || md.IsConstructor ||
!md.HasThis)
+ continue;
+
+ // check that the method is it this
class or a subclass
+ if (!IsSubsclass (md.DeclaringType,
method.DeclaringType))
+ continue;
+
+ // check that we're not calling the
method on another object
+ if (!IsCallFromInstance
(current.Previous, md.Parameters.Count))
+ continue;
+
+ if (md.IsVirtual && !md.IsFinal) {
+ Location loc = new Location
(method, current.Offset);
+ string s = stack.Count == 0 ?
method.ToString () : stack.Aggregate ((a1, a2) => a1 + ", " +
Environment.NewLine + a2);
+ s = String.Format ("Calling a
virtual method, '{0}', from {1} of a non-sealed class is a bad practice.", md,
s);
+ Message msg = new Message (s,
loc, MessageType.Error);
if (messages == null)
messages = new
MessageCollection (msg);
else
messages.Add (msg);
+ } else {
+ stack.Push (method_name);
+ CheckMethod (md, ref messages,
stack);
+ stack.Pop ();
}
break;
}
Modified:
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/EqualShouldHandleNullArgRule.cs
===================================================================
---
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/EqualShouldHandleNullArgRule.cs
2008-01-24 00:01:57 UTC (rev 93745)
+++
trunk/cecil/gendarme/rules/Gendarme.Rules.BadPractice/EqualShouldHandleNullArgRule.cs
2008-01-24 00:02:07 UTC (rev 93746)
@@ -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
@@ -24,64 +26,105 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.
-using System;
-using System.Collections;
-
using Mono.Cecil;
using Mono.Cecil.Cil;
+
using Gendarme.Framework;
+using Gendarme.Framework.Rocks;
namespace Gendarme.Rules.BadPractice {
- public class EqualShouldHandleNullArgRule: IMethodRule {
+ public class EqualShouldHandleNullArgRule: ITypeRule {
- // copy-paste from
gendarme\rules\Gendarme.Rules.Correctness\CallingEqualsWithNullArgRule.cs
- private static bool IsEquals (MethodReference md)
- {
- if ((md == null) || (md.Name != "Equals"))
- return false;
+ private const string Message = "The overridden method
Object.Equals (Object) does not return false if null value is found";
- return (md.ReturnType.ReturnType.FullName ==
"System.Boolean");
- }
-
- public MessageCollection CheckMethod (MethodDefinition method,
Runner runner)
+ public MessageCollection CheckType (TypeDefinition type, Runner
runner)
{
- // rule applies only if a body is available (e.g. not
for pinvokes...)
- if (!method.HasBody)
+ // rules applies to types that overrides
System.Object.Equals(object)
+ MethodDefinition method = type.GetMethod
(MethodSignatures.Equals);
+ if ((method == null) || !method.HasBody)
return runner.RuleSuccess;
- // rules applies for Equals overrides
- if (!IsEquals (method) || !method.IsVirtual)
+ // rule applies
+
+ // scan IL to see if null is checked and false returned
+ if (HandlesNullArg (method))
return runner.RuleSuccess;
-
- foreach (ParameterDefinition param in
method.Parameters) {
- if (param.ParameterType.FullName ==
"System.Object") {
- if (!HandlesNullArg (method)) {
- Location location = new
Location (method);
- Message message = new Message
("The overridden method Object.Equals (Object) does not return false if null
value is found", location, MessageType.Error);
- return new MessageCollection
(message);
- }
- }
- }
- return runner.RuleSuccess;
+
+ Location location = new Location (method);
+ Message msg = new Message (Message, location,
MessageType.Error);
+ return new MessageCollection (msg);
}
- // FIXME: this logic seems to work only on mono generated
assemblies
- // and can fail on VS.NET (e.g. debug)
+ // note: not perfect, in particular when calls to other methods
are used
private static bool HandlesNullArg (MethodDefinition method)
{
- Instruction prevIns;
- foreach (Instruction ins in method.Body.Instructions) {
- prevIns = ins.Previous;
- if (ins.OpCode == OpCodes.Ret && prevIns.OpCode
== OpCodes.Ldc_I4_0) {
- // added check for case where Equals
simpli returns true (or false)
- if (prevIns.Previous == null)
- return false;
- else if (prevIns.Previous.OpCode ==
OpCodes.Brtrue)
+ bool this_used = false;
+ bool object_used = false;
+ bool null_check = false;
+ bool return_value = false;
+
+ int n = 500; // avoid endless loop
+ Instruction ins = method.Body.Instructions [0];
+ while ((ins != null) && (n-- > 0)) {
+ switch (ins.OpCode.Code) {
+ case Code.Ldarg_0:
+ this_used = true;
+ break;
+ case Code.Ldarg_1:
+ // object parameter is used
+ object_used = true;
+ break;
+ case Code.Ldc_I4_0:
+ // it's possible that Equals returns
false (without any more checks)
+ return_value = false;
+ break;
+ case Code.Ldc_I4_1:
+ // it's possible that Equals returns
true (without any more checks)
+ return_value = true;
+ break;
+ case Code.Isinst:
+ if (object_used)
+ null_check = true;
+ break;
+ case Code.Brtrue:
+ case Code.Brtrue_S:
+ // this could be the null check after
Ldarg_1
+ if (object_used && !null_check) {
+ null_check = true;
+ // we do not branch since
ldarg_1 is null
+ }
+ break;
+ case Code.Brfalse:
+ case Code.Brfalse_S:
+ if (object_used || null_check) {
+ // this could be the null check
after Ldarg_1
+ if (!null_check)
+ null_check =
(ins.Previous.OpCode.Code == Code.Ldarg_1);
+ ins = (Instruction) ins.Operand;
+ continue;
+ }
+ break;
+ case Code.Ceq:
+ // if (this == obj), this cannot be null
+ if (this_used && object_used)
+ null_check = true;
+ break;
+ case Code.Ret:
+ // if we return the value from a call
+ if (ins.Previous.OpCode.FlowControl ==
FlowControl.Call)
return true;
+ ins = null;
+ continue;
}
+ ins = ins.Next;
}
- return false;
+ // case #1: object was not used
+ if (!object_used)
+ return !return_value;
+ else
+ return (null_check && !return_value);
+ // not sure, but we don't want to bury results with
false-negative
}
}
}
_______________________________________________
Mono-patches maillist - [email protected]
http://lists.ximian.com/mailman/listinfo/mono-patches
