2008-02-15 Sebastien Pouliot <sebastien@ximian.com>

	* GetLastErrorMustBeCalledRightAfterPInvokeRule.cs
	* MarshalStringsInPInvokeDeclarationsRule.cs
	* PInvokeShouldNotBeVisibleRule.cs
	* UseManagedAlternativesToPInvokeRule.cs:
		Update rules wrt framework changes.


svn path=/trunk/mono-tools/; revision=95794

5 files changed, 89 insertions, 91 deletions

diff --git a/gendarme/rules/Gendarme.Rules.Interoperability/ChangeLog b/gendarme/rules/Gendarme.Rules.Interoperability/ChangeLog
index 7a2c5fa2..ad256e41 100644
--- a/gendarme/rules/Gendarme.Rules.Interoperability/ChangeLog
+++ b/gendarme/rules/Gendarme.Rules.Interoperability/ChangeLog
@@ -1,3 +1,11 @@
+2008-02-15  Sebastien Pouliot  <sebastien@ximian.com>
+
+	* GetLastErrorMustBeCalledRightAfterPInvokeRule.cs
+	* MarshalStringsInPInvokeDeclarationsRule.cs
+	* PInvokeShouldNotBeVisibleRule.cs
+	* UseManagedAlternativesToPInvokeRule.cs:
+		Update rules wrt framework changes.
+
 2008-01-30  Sebastien Pouliot  <sebastien@ximian.com>
 
 	* GetLastErrorMustBeCalledRightAfterPInvokeRule.cs: Changed to
diff --git a/gendarme/rules/Gendarme.Rules.Interoperability/GetLastErrorMustBeCalledRightAfterPInvokeRule.cs b/gendarme/rules/Gendarme.Rules.Interoperability/GetLastErrorMustBeCalledRightAfterPInvokeRule.cs
index e37dba0e..3e303ef8 100644
--- a/gendarme/rules/Gendarme.Rules.Interoperability/GetLastErrorMustBeCalledRightAfterPInvokeRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Interoperability/GetLastErrorMustBeCalledRightAfterPInvokeRule.cs
@@ -26,17 +26,21 @@
 // WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
 //
 
+using System;
 using System.Collections.Generic;
 
 using Mono.Cecil;
 using Mono.Cecil.Cil;
 
 using Gendarme.Framework;
+using Gendarme.Framework.Helpers;
 using Gendarme.Framework.Rocks;
 
 namespace Gendarme.Rules.Interoperability {
 
-	public class GetLastErrorMustBeCalledRightAfterPInvokeRule : IMethodRule {
+	[Problem ("GetLastError() should be called immediately after this the P/Invoke call.")]
+	[Solution ("Move the call to GetLastError just after the P/Invoke call.")]
+	public class GetLastErrorMustBeCalledRightAfterPInvokeRule : Rule, IMethodRule {
 
 		struct Branch {
 			public readonly Instruction Instruction;
@@ -49,6 +53,8 @@ namespace Gendarme.Rules.Interoperability {
 			}
 		}
 
+		private const string Message = "GetLastError() should be called immediately after this the PInvoke call.";
+
 		private const string GetLastError = "System.Int32 System.Runtime.InteropServices.Marshal::GetLastWin32Error()";
 		private List<string> AllowedCalls;
 
@@ -62,7 +68,7 @@ namespace Gendarme.Rules.Interoperability {
 
 		List<Branch> branches = new List<Branch> ();
 
-		private bool CheckPInvoke (MethodDefinition pinvoke, Instruction startInstruction)
+		private bool CheckPInvoke (Instruction startInstruction)
 		{
 			branches.Clear ();
 			branches.Add (new Branch (startInstruction.Next, false));
@@ -102,8 +108,9 @@ namespace Gendarme.Rules.Interoperability {
 						break;
 
 					if (alternatives != null) {
-						if (alternatives is Instruction) {
-							branches.AddIfNew (new Branch ((Instruction) alternatives, dirty));
+						Instruction alt_ins = (alternatives as Instruction);
+						if (alt_ins != null) {
+							branches.AddIfNew (new Branch (alt_ins, dirty));
 						} else {
 							Instruction [] alts = (Instruction []) alternatives;
 							foreach (Instruction altIns in alts)
@@ -124,12 +131,11 @@ namespace Gendarme.Rules.Interoperability {
 			return true;
 		}
 
-		public MessageCollection CheckMethod (MethodDefinition method, Runner runner)
+		public RuleResult CheckMethod (MethodDefinition method)
 		{
+			// rule does not apply if the method has no IL
 			if (!method.HasBody)
-				return runner.RuleSuccess;
-
-			MessageCollection results = null;
+				return RuleResult.DoesNotApply;
 
 			foreach (Instruction ins in method.Body.Instructions) {
 				switch (ins.OpCode.Code) {
@@ -150,21 +156,18 @@ namespace Gendarme.Rules.Interoperability {
 						break;
 
 					// check if GetLastError is called near enough this pinvoke call
-					if (CheckPInvoke (pinvoke, ins))
+					if (CheckPInvoke (ins))
 						break;
 
-					if (results == null)
-						results = new MessageCollection ();
-					Location loc = new Location (method, ins.Offset);
-					Message msg = new Message ("GetLastError() should be called immediately after this the PInvoke call.", loc, MessageType.Error);
-					results.Add (msg);
+					// code might not work if an error occurs
+					Runner.Report (method, ins, Severity.High, Confidence.High, String.Empty);
 					break;
 				default:
 					break;
 				}
 			}
 
-			return results;
+			return Runner.CurrentRuleResult;
 		}
 	}
 }
diff --git a/gendarme/rules/Gendarme.Rules.Interoperability/MarshalStringsInPInvokeDeclarationsRule.cs b/gendarme/rules/Gendarme.Rules.Interoperability/MarshalStringsInPInvokeDeclarationsRule.cs
index ab5744eb..643a3397 100644
--- a/gendarme/rules/Gendarme.Rules.Interoperability/MarshalStringsInPInvokeDeclarationsRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Interoperability/MarshalStringsInPInvokeDeclarationsRule.cs
@@ -33,38 +33,39 @@ using Gendarme.Framework;
 
 namespace Gendarme.Rules.Interoperability {
 
-	public class MarshalStringsInPInvokeDeclarationsRule : IMethodRule {
+	[Problem ("Marshaling information for string types is incomplete and what is required may be different from what you expected the default to be.")]
+	[Solution ("Add [DllImport CharSet=] on the method or [MarshalAs] on the parameter(s)")]
+	public class MarshalStringsInPInvokeDeclarationsRule : Rule, IMethodRule {
 
 		private static bool IsStringOrSBuilder (TypeReference reference)
 		{
-			TypeReference original = reference.GetOriginalType ();
-			return original.FullName == "System.String" || original.FullName == "System.Text.StringBuilder";
+			switch (reference.GetOriginalType ().FullName) {
+			case "System.String":
+			case "System.Text.StringBuilder":
+				return true;
+			default:
+				return false;
+			}
 		}
 
-		private static void AddBadParameterMessage (ref MessageCollection messages, ParameterDefinition param)
+		public RuleResult CheckMethod (MethodDefinition method)
 		{
-			if (messages == null)
-				messages = new MessageCollection ();
+			// rule does not apply to non-pinvoke methods
+			if (!method.IsPInvokeImpl)
+				return RuleResult.DoesNotApply;
 
-			Location loc = new Location (param.Method as MethodDefinition);
-			string text = string.Format ("Parameter '{0}', of type '{1}', does not have [MarshalAs] attribute, yet no [DllImport CharSet=] is set for the method '{2}'.", 
-				param.Name, param.ParameterType.Name, param.Method.Name);
-			Message msg = new Message (text, loc, MessageType.Error);
-			messages.Add (msg);
-		}
+			if (!method.PInvokeInfo.IsCharSetNotSpec)
+				return RuleResult.Success;
 
-		public MessageCollection CheckMethod (MethodDefinition method, Runner runner)
-		{
-			if (!method.IsPInvokeImpl || !method.PInvokeInfo.IsCharSetNotSpec)
-				return runner.RuleSuccess;
-
-			MessageCollection messages = runner.RuleSuccess;
 			foreach (ParameterDefinition parameter in method.Parameters) {
 				if (IsStringOrSBuilder (parameter.ParameterType) && (parameter.MarshalSpec == null)) {
-					AddBadParameterMessage (ref messages, parameter);
+					string text = string.Format ("Parameter '{0}', of type '{1}', does not have [MarshalAs] attribute, yet no [DllImport CharSet=] is set for the method '{2}'.",
+						parameter.Name, parameter.ParameterType.Name, parameter.Method.Name);
+					Runner.Report (parameter, Severity.High, Confidence.Total, text);
 				}
 			}
-			return messages;
+
+			return Runner.CurrentRuleResult;
 		}
 	}
 }
diff --git a/gendarme/rules/Gendarme.Rules.Interoperability/PInvokeShouldNotBeVisibleRule.cs b/gendarme/rules/Gendarme.Rules.Interoperability/PInvokeShouldNotBeVisibleRule.cs
index 9079dec8..f5768898 100644
--- a/gendarme/rules/Gendarme.Rules.Interoperability/PInvokeShouldNotBeVisibleRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Interoperability/PInvokeShouldNotBeVisibleRule.cs
@@ -31,32 +31,30 @@ using System;
 using Mono.Cecil;
 
 using Gendarme.Framework;
+using Gendarme.Framework.Rocks;
 
 namespace Gendarme.Rules.Interoperability {
 
-	public class PInvokeShouldNotBeVisibleRule : IMethodRule {
+	[Problem ("P/Invoke declarations should not be visible outside of the assembly.")]
+	[Solution ("Wrap the p/invoke call into a managed class/method and include parameters, and result, validation(s).")]
+	public class PInvokeShouldNotBeVisibleRule : Rule, IMethodRule {
 
-		public MessageCollection CheckMethod (MethodDefinition method, Runner runner)
+		public RuleResult CheckMethod (MethodDefinition method)
 		{
+			// rule does not apply to non-p/invoke
 			if (!method.IsPInvokeImpl)
-				return runner.RuleSuccess;
-
-			if (!method.IsPublic)
-				return runner.RuleSuccess;
-
-			TypeDefinition type = (TypeDefinition) method.DeclaringType;
-
-			while (type.IsNested) {
-				if (!type.IsNestedPublic)
-					return runner.RuleSuccess;
-				type = (TypeDefinition) type.DeclaringType;
-			}
-			if (!type.IsPublic)
-				return runner.RuleSuccess;
-
-			Location loc = new Location (method);
-			Message msg = new Message ("P/Invoke declarations should not be visible outside of the assembly.", loc, MessageType.Warning);
-			return new MessageCollection (msg);
+				return RuleResult.DoesNotApply;
+			
+			// rule applies
+			
+			// ok if method is not visible (this include it's declaring type too)
+			if (!method.IsVisible ())
+				return RuleResult.Success;
+
+			// code will work (low) but it's bad design (non-fx-like validations) and makes
+			// it easier to expose security vulnerabilities
+			Runner.Report (method, Severity.Low, Confidence.Total, String.Empty);
+			return RuleResult.Failure;
 		}
 	}
 }
diff --git a/gendarme/rules/Gendarme.Rules.Interoperability/UseManagedAlternativesToPInvokeRule.cs b/gendarme/rules/Gendarme.Rules.Interoperability/UseManagedAlternativesToPInvokeRule.cs
index c158f3f0..bbe235c9 100644
--- a/gendarme/rules/Gendarme.Rules.Interoperability/UseManagedAlternativesToPInvokeRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Interoperability/UseManagedAlternativesToPInvokeRule.cs
@@ -37,7 +37,9 @@ using Mono.Cecil;
 
 namespace Gendarme.Rules.Interoperability {
 
-	public class UseManagedAlternativesToPInvokeRule : IMethodRule {
+	[Problem ("There is a potential managed API for the p/invoke declaration.")]
+	[Solution ("Use the suggested managed alternative to your p/invoke call and remove it's declaration.")]
+	public class UseManagedAlternativesToPInvokeRule : Rule, IMethodRule {
 
 		private struct PInvokeCall {
 
@@ -89,7 +91,7 @@ namespace Gendarme.Rules.Interoperability {
 			}
 		}
 
-		private class ManagedAlternatives {
+		private sealed class ManagedAlternatives {
 
 			private List<string> alternatives = new List<string> ();
 			private TargetRuntime runtime;
@@ -122,28 +124,17 @@ namespace Gendarme.Rules.Interoperability {
 			}
 		}
 
-		private static Dictionary<PInvokeCall, ManagedAlternatives> managedAlternatives;
-
-		static UseManagedAlternativesToPInvokeRule ()
-		{
-			managedAlternatives = new Dictionary<PInvokeCall, ManagedAlternatives> ();
-			managedAlternatives.Add (new PInvokeCall ("kernel32.dll", "Sleep"),
-						 new ManagedAlternatives ("System.Threading.Thread::Sleep ()"));
-			managedAlternatives.Add (new PInvokeCall ("kernel32.dll", "FindFirstFile"),
-						 new ManagedAlternatives ("System.IO.Directory::GetDirectories ()", "System.IO.Directory::GetFiles ()", "System.IO.Directory::GetFileSystemEntries ()"));
-			managedAlternatives.Add (new PInvokeCall ("kernel32.dll", "ReadFile"),
-						 new ManagedAlternatives ("System.IO.FileStream"));
-			managedAlternatives.Add (new PInvokeCall ("kernel32.dll", "WaitForMultipleObjects"),
-						 new ManagedAlternatives ("System.Threading.WaitHandle::WaitAny ()", "System.Threading.WaitHandle::WaitAll ()"));
-			managedAlternatives.Add (new PInvokeCall ("kernel32.dll", "GetLastError"),
-						 new ManagedAlternatives ("System.Runtime.InteropServices.Marshal::GetLastWin32Error ()"));
-			managedAlternatives.Add (new PInvokeCall ("user32.dll", "MessageBox"),
-						 new ManagedAlternatives ("System.Windows.Forms.MessageBox::Show ()"));
-			managedAlternatives.Add (new PInvokeCall ("kernel32.dll", "Beep"),
-						 new ManagedAlternatives (TargetRuntime.NET_2_0, "System.Console::Beep ()"));
-			managedAlternatives.Add (new PInvokeCall ("winmm.dll", "PlaySound"),
-						 new ManagedAlternatives (TargetRuntime.NET_2_0, "System.Media.SoundPlayer"));
-		}
+		private static Dictionary<PInvokeCall, ManagedAlternatives> managedAlternatives =
+			new Dictionary<PInvokeCall, ManagedAlternatives> () {
+				{ new PInvokeCall ("kernel32.dll", "Sleep"), new ManagedAlternatives ("System.Threading.Thread::Sleep ()") },
+				{ new PInvokeCall ("kernel32.dll", "FindFirstFile"), new ManagedAlternatives ("System.IO.Directory::GetDirectories ()", "System.IO.Directory::GetFiles ()", "System.IO.Directory::GetFileSystemEntries ()") },
+				{ new PInvokeCall ("kernel32.dll", "ReadFile"), new ManagedAlternatives ("System.IO.FileStream") },
+				{ new PInvokeCall ("kernel32.dll", "WaitForMultipleObjects"), new ManagedAlternatives ("System.Threading.WaitHandle::WaitAny ()", "System.Threading.WaitHandle::WaitAll ()") },
+				{ new PInvokeCall ("kernel32.dll", "GetLastError"), new ManagedAlternatives ("System.Runtime.InteropServices.Marshal::GetLastWin32Error ()") },
+				{ new PInvokeCall ("user32.dll", "MessageBox"), new ManagedAlternatives ("System.Windows.Forms.MessageBox::Show ()") },
+				{ new PInvokeCall ("kernel32.dll", "Beep"), new ManagedAlternatives (TargetRuntime.NET_2_0, "System.Console::Beep ()") },
+				{ new PInvokeCall ("winmm.dll", "PlaySound"), new ManagedAlternatives (TargetRuntime.NET_2_0, "System.Media.SoundPlayer") }
+			};
 
 		private static ManagedAlternatives GetManagedAlternatives (MethodDefinition method, TargetRuntime runtime)
 		{
@@ -151,30 +142,27 @@ namespace Gendarme.Rules.Interoperability {
 			PInvokeCall callInfo = new PInvokeCall (moduleName, method.Name);
 			if (managedAlternatives.ContainsKey (callInfo)) {
 				ManagedAlternatives alts = managedAlternatives [callInfo];
-				if (IsRuntimeVersionGreaterOrEqual (runtime, alts.Runtime))
+				if (runtime >= alts.Runtime)
 					return alts;
 			}
 			return null;
 		}
 
-		private static bool IsRuntimeVersionGreaterOrEqual (TargetRuntime a, TargetRuntime b)
-		{
-			return (int) a >= (int) b; // as of now it works (and should work further)
-		}
-
-		public MessageCollection CheckMethod (MethodDefinition method, Runner runner)
+		public RuleResult CheckMethod (MethodDefinition method)
 		{
+			// rule does not apply to non-pinvoke methods
 			if (!method.IsPInvokeImpl)
-				return runner.RuleSuccess;
+				return RuleResult.DoesNotApply;
+
+			// rule apply, looks for alternatives
 
 			ManagedAlternatives alternatives = GetManagedAlternatives (method, method.DeclaringType.Module.Assembly.Runtime);
 			if (alternatives == null)
-				return runner.RuleSuccess;
+				return RuleResult.Success;
 
-			Location loc = new Location (method);
 			string message = string.Format ("Do not perform platform-dependent call ({0}) if it can be avoided. Use (one of) the following alternative(s) provided by .NET Framework: {1}.", method.Name, alternatives);
-			Message msg = new Message (message, loc, MessageType.Warning);
-			return new MessageCollection (msg);
+			Runner.Report (method, Severity.Low, Confidence.High, message);
+			return RuleResult.Failure;
 		}
 	}
 }