diff --git a/gendarme/framework/Gendarme.Framework.Rocks/InstructionRocks.cs b/gendarme/framework/Gendarme.Framework.Rocks/InstructionRocks.cs
index aef546a2d..770d55508 100644
--- a/gendarme/framework/Gendarme.Framework.Rocks/InstructionRocks.cs
+++ b/gendarme/framework/Gendarme.Framework.Rocks/InstructionRocks.cs
@@ -511,6 +511,20 @@ public static bool IsStoreLocal (this Instruction self)
return OpCodeBitmask.StoreLocal.Get (self.OpCode.Code);
}
+ ///
+ /// Return true if the Instruction is a store of a field (stfld).
+ ///
+ /// The Instruction on which the extension method can be called.
+ /// True if the instruction is a store field, False otherwise
+ public static bool IsStoreField (this Instruction self)
+ {
+ if (self == null)
+ return false;
+
+ return self.OpCode.Code == Code.Stfld;
+ }
+
+
///
/// Return the instruction that match the current instruction. This is computed by
/// substracting push and adding pop counts until the total becomes zero.
diff --git a/gendarme/framework/Gendarme.Framework.Rocks/ModuleRocks.cs b/gendarme/framework/Gendarme.Framework.Rocks/ModuleRocks.cs
index 3aad11713..2527dc7a0 100644
--- a/gendarme/framework/Gendarme.Framework.Rocks/ModuleRocks.cs
+++ b/gendarme/framework/Gendarme.Framework.Rocks/ModuleRocks.cs
@@ -85,7 +85,7 @@ public static void LoadDebuggingSymbols (this ModuleDefinition self)
// so we start by looking for it's debugging symbol file
if (File.Exists (symbol_name)) {
// "always" if we can find Mono.Cecil.Mdb
- reader_type = Type.GetType ("Mono.Cecil.Mdb.MdbReaderProvider, Mono.Cecil.Mdb, Version=0.9.4.0, Culture=neutral, PublicKeyToken=0738eb9f132ed756");
+ reader_type = Type.GetType ("Mono.Cecil.Mdb.MdbReaderProvider, Mono.Cecil.Mdb, Version=0.9.5.0, Culture=neutral, PublicKeyToken=0738eb9f132ed756");
// load the assembly from the current folder if
// it is here, or fallback to the gac
}
diff --git a/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs b/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs
index 6d1c73dbe..f4b484a17 100644
--- a/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs
@@ -37,6 +37,7 @@
using Gendarme.Framework.Rocks;
using Gendarme.Framework.Engines;
using Gendarme.Framework.Helpers;
+using System.Diagnostics;
namespace Gendarme.Rules.Correctness {
@@ -119,6 +120,7 @@ public sealed class EnsureLocalDisposalRule : Rule, IMethodRule {
OpCodeBitmask callsAndNewobjBitmask = BuildCallsAndNewobjOpCodeBitmask ();
Bitmask locals = new Bitmask ();
+ Dictionary localsReferencingOtherLocals = null;
static bool IsDispose (MethodReference call)
{
@@ -129,9 +131,8 @@ static bool IsDispose (MethodReference call)
static bool DoesReturnDisposable (MethodReference call)
{
- //ignore properties (likely not the place where the IDisposable is *created*)
MethodDefinition method = call.Resolve ();
- if ((method == null) || call.IsProperty ())
+ if ((method == null) || (call.IsProperty () && !method.IsGetter))
return false;
if (method.IsConstructor) {
@@ -168,11 +169,23 @@ static bool IsInsideFinallyBlock (MethodDefinition method, Instruction ins)
return false;
}
+ void ClearVariable (int index)
+ {
+ if (locals.Get ((ulong)index)) {
+ locals.Clear ((ulong)index);
+ Debug.Assert (localsReferencingOtherLocals == null || !localsReferencingOtherLocals.ContainsKey (index));
+ } else {
+ int referencedIndex;
+ if (localsReferencingOtherLocals != null && localsReferencingOtherLocals.TryGetValue (index, out referencedIndex))
+ ClearVariable (referencedIndex);
+ }
+ }
+
void Clear (MethodDefinition method, Instruction ins)
{
VariableDefinition v = ins.GetVariable (method);
if (v != null)
- locals.Clear ((ulong) v.Index);
+ ClearVariable (v.Index);
}
void CheckForReturn (MethodDefinition method, Instruction ins)
@@ -204,10 +217,13 @@ void CheckDisposeCalls (MethodDefinition method, Instruction ins)
ulong index = v == null ? UInt64.MaxValue : (ulong) v.Index;
if (v != null && locals.Get (index)) {
if (!IsInsideFinallyBlock (method, ins)) {
- string msg = String.Format (CultureInfo.InvariantCulture,
- "Local {0}is not guaranteed to be disposed of.",
- GetFriendlyNameOrEmpty (v));
- Runner.Report (method, Severity.Medium, Confidence.Normal, msg);
+ if (!v.VariableType.Equals (method.ReturnType) &&
+ !v.VariableType.Inherits (method.ReturnType.Namespace, method.ReturnType.Name)) {
+ string msg = String.Format (CultureInfo.InvariantCulture,
+ "Local {0}is not guaranteed to be disposed of.",
+ GetFriendlyNameOrEmpty (v));
+ Runner.Report (method, Severity.Medium, Confidence.Normal, msg);
+ }
}
locals.Clear (index);
}
@@ -228,6 +244,18 @@ bool CheckCallsToOtherInstances (MethodDefinition method, Instruction ins, Metho
return true;
}
+ bool AssignedToLocalVarAndReturn (MethodDefinition method, Instruction ins)
+ {
+ var nextIns = ins.Next;
+ if (nextIns.OpCode.Code != Code.Br && nextIns.OpCode.Code != Code.Br_S)
+ return false;
+
+ var targetIns = nextIns.Operand as Instruction;
+
+ return (targetIns != null && targetIns.IsLoadLocal () &&
+ targetIns.Next != null && targetIns.Next.OpCode.Code == Code.Ret);
+ }
+
void CheckReassignment (MethodDefinition method, Instruction ins)
{
VariableDefinition v = ins.GetVariable (method);
@@ -238,8 +266,55 @@ void CheckReassignment (MethodDefinition method, Instruction ins)
GetFriendlyNameOrEmpty (v));
Runner.Report (method, ins, Severity.High, Confidence.Normal, msg);
} else {
+ if (AssignedToLocalVarAndReturn (method, ins))
+ return;
+
locals.Set (index);
+ if (localsReferencingOtherLocals != null && localsReferencingOtherLocals.ContainsKey ((int)index))
+ localsReferencingOtherLocals.Remove ((int)index);
+ }
+ }
+
+ int GetFirstVariableHoldingObject (int index)
+ {
+ int referencedVar;
+ if (localsReferencingOtherLocals.TryGetValue (index, out referencedVar))
+ return GetFirstVariableHoldingObject (referencedVar);
+
+ Debug.Assert (locals.Get ((ulong)index));
+ return index;
+ }
+
+ void CheckForAssignmentReferencingOtherVariable (MethodDefinition method, Instruction ins)
+ {
+ if (!ins.IsStoreLocal () && !ins.IsStoreField ())
+ return;
+
+ var prevIns = ins.Previous;
+ if (prevIns == null || !prevIns.IsLoadLocal ())
+ return;
+
+ var vLoad = prevIns.GetVariable (method);
+ if (vLoad == null)
+ return;
+
+ if (ins.IsStoreField ()) {
+ ClearVariable (vLoad.Index);
+ return;
}
+
+ var vStore = ins.GetVariable (method);
+ if (vStore == null)
+ return;
+
+ if (locals.Get ((ulong)vStore.Index))
+ return;
+
+ if (localsReferencingOtherLocals == null)
+ localsReferencingOtherLocals = new Dictionary ();
+ localsReferencingOtherLocals [vStore.Index] = GetFirstVariableHoldingObject (vLoad.Index);
+ if (localsReferencingOtherLocals [vStore.Index] == vStore.Index)
+ localsReferencingOtherLocals.Remove (vStore.Index);
}
public RuleResult CheckMethod (MethodDefinition method)
@@ -255,6 +330,7 @@ public RuleResult CheckMethod (MethodDefinition method)
bool return_idisposable = DoesReturnDisposable (method);
locals.ClearAll ();
+ localsReferencingOtherLocals = null;
foreach (Instruction ins in method.Body.Instructions) {
Code code = ins.OpCode.Code;
@@ -267,8 +343,10 @@ public RuleResult CheckMethod (MethodDefinition method)
CheckForOutParameters (method, ins);
continue;
default:
- if (!callsAndNewobjBitmask.Get (code))
+ if (!callsAndNewobjBitmask.Get (code)) {
+ CheckForAssignmentReferencingOtherVariable (method, ins);
continue;
+ }
break;
}
@@ -293,8 +371,9 @@ public RuleResult CheckMethod (MethodDefinition method)
Code nextCode = nextInstruction.OpCode.Code;
if (nextCode == Code.Pop || OpCodeBitmask.Calls.Get (nextCode)) {
+ // We ignore properties (likely not the place where the IDisposable is *created*)
// We ignore setter because it is an obvious share of the IDisposable
- if (!IsSetter (nextInstruction.Operand as MethodReference))
+ if (!call.IsProperty () && !IsSetter (nextInstruction.Operand as MethodReference))
ReportCall (method, ins, call);
} else if (nextInstruction.IsStoreLocal ()) {
// make sure we're not re-assigning over a non-disposed IDisposable
diff --git a/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs
index e375d1d0c..aaf5e8099 100644
--- a/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs
+++ b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs
@@ -439,5 +439,212 @@ public void ReturnValue ()
#endif
AssertRuleSuccess ("ReturnIDisposable2");
}
+
+ #region Bug #18498
+ abstract class Image : IDisposable
+ {
+ public void Dispose ()
+ {
+ }
+ }
+
+ class Bitmap : Image
+ {
+ }
+
+ Image ReturnsReference ()
+ {
+ Bitmap bmp = new Bitmap ();
+ Image img = (Image)bmp;
+ return img;
+ }
+
+ Image AlsoReturnsReference ()
+ {
+ var bmp = new Bitmap ();
+ var img = (Image)bmp;
+ var img2 = img;
+ return img2;
+ }
+
+ Image MissesDispose ()
+ {
+ var bmp = new Bitmap ();
+ var img = (Image)bmp;
+ img = new Bitmap ();
+ return img;
+ }
+
+ Image AlsoMissesDispose ()
+ {
+ var bmp = new Bitmap ();
+ var img = new Bitmap ();
+ bmp = img;
+ return bmp;
+ }
+
+
+ [Test]
+ public void ReturnsReference_Bug18498 ()
+ {
+ AssertRuleSuccess ("ReturnsReference");
+ }
+
+ [Test]
+ public void ReturnReference2_Bug18498 ()
+ {
+ AssertRuleSuccess ("AlsoReturnsReference");
+ }
+
+ [Test]
+ public void MissingDispose_Bug18498 ()
+ {
+ AssertRuleFailure ("MissesDispose");
+ }
+
+ [Test]
+ public void MissingDispose2_Bug18498 ()
+ {
+ AssertRuleFailure ("AlsoMissesDispose");
+ }
+ #endregion
+
+ Image PropertyReturningObj {
+ get {
+ using (var bmp = new Bitmap ()) {
+ var img = new Bitmap ();
+ return img;
+ }
+ }
+ }
+
+ [Test]
+ public void PropertyReturningReference ()
+ {
+ AssertRuleSuccess ("get_PropertyReturningObj");
+ }
+
+ Image ReturnObjectWithIfs ()
+ {
+ Bitmap obj = null;
+ int a = new Random ().Next (10);
+ if (a == 1)
+ obj = new Bitmap ();
+ else if (a == 2)
+ obj = new Bitmap ();
+ else if (a == 3)
+ obj = new Bitmap ();
+ return obj;
+ }
+
+ [Test]
+ public void MethodThatConditionallyReturnsObjects ()
+ {
+ AssertRuleSuccess ("ReturnObjectWithIfs");
+ }
+
+ Bitmap ReturnObjectWithSwitch ()
+ {
+ Bitmap obj = null;
+ switch (new Random ().Next (10))
+ {
+ case 1:
+ obj = new Bitmap();
+ break;
+ case 2:
+ obj = new Bitmap();
+ break;
+ default:
+ obj = new Bitmap();
+ break;
+ }
+ return obj;
+ }
+
+ [Test]
+ public void MethodThatConditionallyReturnsObjectsThroughSwitch ()
+ {
+ AssertRuleSuccess ("ReturnObjectWithSwitch");
+ }
+
+ Bitmap ReturnObjectAndCallsDisposeInTryCatchBlock ()
+ {
+ Bitmap obj = null;
+ try {
+ obj = new Bitmap ();
+ } catch (Exception) {
+ if (obj != null)
+ obj.Dispose ();
+ throw;
+ }
+
+ return obj;
+ }
+
+ void CallsDisposeInTryCatchBlock ()
+ {
+ Bitmap obj = null;
+ try {
+ obj = new Bitmap ();
+ } catch (Exception) {
+ if (obj != null)
+ obj.Dispose ();
+ throw;
+ }
+ }
+
+ [Test]
+ public void MethodReturnsObjectAndCallsDisposeInTryCatchBlock ()
+ {
+ AssertRuleSuccess ("ReturnObjectAndCallsDisposeInTryCatchBlock");
+ }
+
+ [Test]
+ public void MethodCallsDisposeInTryCatchBlock ()
+ {
+ AssertRuleFailure ("CallsDisposeInTryCatchBlock");
+ }
+
+ class TestableClass: IDisposable
+ {
+ Stream stream;
+
+ public void AssignFieldFromLocalVariableWithInitializer ()
+ {
+ var s = new MemoryStream {
+ ReadTimeout = 10
+ };
+
+ stream = s;
+ }
+
+ public void AssignFieldWithInitializer ()
+ {
+ stream = new MemoryStream {
+ ReadTimeout = 10
+ };
+ }
+
+ #region IDisposable implementation
+
+ public void Dispose ()
+ {
+ stream.Dispose ();
+ }
+
+ #endregion
+ }
+
+ [Test]
+ public void FieldAssignedFromLocalVariableWithInitializer ()
+ {
+ AssertRuleSuccess ("AssignFieldFromLocalVariableWithInitializer");
+ }
+
+ [Test]
+ public void FieldAssignedWithInitializer ()
+ {
+ AssertRuleSuccess ("AssignFieldWithInitializer");
+ }
}
}