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

	* ExitCodeIsLimitedOnUnixRule.cs
	* FeatureRequiresRootPrivilegeOnUnixRule.cs
	* MonoCompatibilityReviewRule.cs
	* NewLineLiteralRule.cs:
		Updated rules wrt framework changes.


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

5 files changed, 169 insertions, 162 deletions

diff --git a/gendarme/rules/Gendarme.Rules.Portability/ChangeLog b/gendarme/rules/Gendarme.Rules.Portability/ChangeLog
index 21cb3ce4..1474facf 100644
--- a/gendarme/rules/Gendarme.Rules.Portability/ChangeLog
+++ b/gendarme/rules/Gendarme.Rules.Portability/ChangeLog
@@ -1,3 +1,11 @@
+2008-02-15  Sebastien Pouliot  <sebastien@ximian.com> 
+
+	* ExitCodeIsLimitedOnUnixRule.cs
+	* FeatureRequiresRootPrivilegeOnUnixRule.cs
+	* MonoCompatibilityReviewRule.cs
+	* NewLineLiteralRule.cs:
+		Updated rules wrt framework changes.
+
 2008-02-12  Sebastien Pouliot  <sebastien@ximian.com>
 
 	* Gendarme.Rules.Portability.xml.in: Fix definition for
diff --git a/gendarme/rules/Gendarme.Rules.Portability/ExitCodeIsLimitedOnUnixRule.cs b/gendarme/rules/Gendarme.Rules.Portability/ExitCodeIsLimitedOnUnixRule.cs
index f292839f..651aa067 100644
--- a/gendarme/rules/Gendarme.Rules.Portability/ExitCodeIsLimitedOnUnixRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Portability/ExitCodeIsLimitedOnUnixRule.cs
@@ -30,10 +30,15 @@ using Mono.Cecil;
 using Mono.Cecil.Cil;
 
 using Gendarme.Framework;
+using Gendarme.Framework.Rocks;
 
 namespace Gendarme.Rules.Portability {
 
-	public class ExitCodeIsLimitedOnUnixRule : IAssemblyRule, IMethodRule {
+	[Problem ("The rule detected a value outside the 0-255 range or couldn't be sure of the returned value.")]
+	[Solution ("Review that your return values are all between 0 and 255, this will ensure them to works under both Unix and Windows OS.")]
+	public class ExitCodeIsLimitedOnUnixRule : Rule, IAssemblyRule, IMethodRule {
+
+		private const string Message = "In Unix, unlike in Windows, Main () method must return values between 0 and 255 inclusively. Change the exit code or change method return type from 'int' to 'void'.";
 
 		private enum InspectionResult {
 			Good,
@@ -41,52 +46,78 @@ namespace Gendarme.Rules.Portability {
 			Unsure
 		}
 
-		public MessageCollection CheckAssembly (AssemblyDefinition assemblyDefinition, Runner runner)
+		public override void Initialize (IRunner runner)
 		{
-			MethodDefinition entryPoint = assemblyDefinition.EntryPoint;
-			// if no entry point, good bye
-			if (entryPoint == null)
-				return runner.RuleSuccess;
-
-			// if it returns void, we don't introspect it
-			// FIXME: entryPoint.ReturnType.ReturnType should not be null with void Main ()
-			// either bad unit tests or bug in cecil
-			if (entryPoint.ReturnType.ReturnType == null || entryPoint.ReturnType.ReturnType.FullName != "System.Int32")
-				return runner.RuleSuccess;
-
-			return CheckIntMainBody (entryPoint.Body, runner);
+			base.Initialize (runner);
+
+			// we always want to call CheckAssembly (single call on each assembly)
+			Runner.AnalyzeAssembly += delegate (object o, RunnerEventArgs e) {
+				Active = true;
+			};
+
+			// but we want to avoid checking all methods if the Environment type
+			// isn't referenced in a module (big performance difference)
+			Runner.AnalyzeModule += delegate (object o, RunnerEventArgs e) {
+				Active = false;
+				foreach (TypeReference type in e.CurrentAssembly.MainModule.TypeReferences) {
+					if (type.FullName == "System.Environment") {
+						Active = true;
+						break;
+					}
+				}
+			};
 		}
 
+		private void Report (MethodDefinition method, Instruction ins, InspectionResult result)
+		{
+			switch (result) {
+			case InspectionResult.Good:
+				// should never occur
+				break;
+			case InspectionResult.Bad:
+				Runner.Report (method, ins, Severity.Medium, Confidence.High, 
+					"Return value is outside the range of valid values (0-255).");
+				break;
+			case InspectionResult.Unsure:
+				Runner.Report (method, ins, Severity.Medium, Confidence.Low, 
+					"Make sure not to return values that are out of range (0-255).");
+				break;
+			}
+		}
 
-		private static MessageCollection CheckIntMainBody (MethodBody body, Runner runner)
+		public RuleResult CheckAssembly (AssemblyDefinition assembly)
 		{
-			MessageCollection messages = runner.RuleSuccess;
+			MethodDefinition entry_point = assembly.EntryPoint;
+
+			// the rule does not apply if the assembly has no entry point
+			// or if it's entry point has no IL
+			if ((entry_point == null) || !entry_point.HasBody)
+				return RuleResult.DoesNotApply;
+
+			// the rule does not apply of the entry point returns void
+			// FIXME: entryPoint.ReturnType.ReturnType should not be null with void Main ()
+			// either bad unit tests or bug in cecil
+			if (entry_point.ReturnType.ReturnType == null || entry_point.ReturnType.ReturnType.FullName != "System.Int32")
+				return RuleResult.DoesNotApply;
+
 			Instruction previous = null;
-			foreach (Instruction current in body.Instructions) {
+			foreach (Instruction current in entry_point.Body.Instructions) {
 				switch (current.OpCode.Code) {
 				case Code.Nop:
 					break;
 				case Code.Ret:
-					InspectionResult results = CheckInstruction (previous);
-					if (results == InspectionResult.Good)
+					InspectionResult result = CheckInstruction (previous);
+					if (result == InspectionResult.Good)
 						break;
 
-					Location loc = new Location (body.Method, current.Offset);
-					Message message = null;
-					if (results == InspectionResult.Bad)
-						message = new Message ("In Unix, unlike in Windows, Main () method must return values between 0 and 255 inclusively. Change the exit code or change method return type from 'int' to 'void'.", loc, MessageType.Error);
-					else if (results == InspectionResult.Unsure)
-						message = new Message ("In Unix, unlike in Windows, Main () method must return values between 0 and 255 inclusively. Be sure not to return values that are out of range.", loc, MessageType.Warning);
-					if (messages == null)
-						messages = new MessageCollection ();
-					messages.Add (message);
+					Report (entry_point, current, result);
 					break;
 				default:
 					previous = current;
 					break;
 				}
 			}
-			return messages;
+			return Runner.CurrentRuleResult;
 		}
 
 		private static InspectionResult CheckInstruction (Instruction instruction)
@@ -114,29 +145,17 @@ namespace Gendarme.Rules.Portability {
 			default:
 				return InspectionResult.Unsure;
 			}
-
 		}
 
-		public MessageCollection CheckMethod (MethodDefinition method, Runner runner)
+		public RuleResult CheckMethod (MethodDefinition method)
 		{
-			// here we check for usage of Environment.ExitCode property
-			MethodDefinition entryPoint = method.DeclaringType.Module.Assembly.EntryPoint;
-
-			// assembly must have an entry point
-			if (entryPoint == null)
-				return runner.RuleSuccess;
-
-			// rule applies only to void Main-ish assemblies
-			// int Main () anyways returns something so we shouldn't care about ExitCode usage
-			if (entryPoint.ReturnType.ReturnType != null && entryPoint.ReturnType.ReturnType.FullName == "System.Int32")
-				return runner.RuleSuccess;
+			// rule does not apply if method has no IL
+			if (!method.HasBody)
+				return RuleResult.DoesNotApply;
 
 			// go!
-			MessageCollection messages = runner.RuleSuccess;
 			Instruction previous = null;
 			foreach (Instruction current in method.Body.Instructions) {
-				// rather useful for debugging
-				// Console.WriteLine ("{0} {1}", current.OpCode, current.Operand);
 				switch (current.OpCode.Code) {
 				case Code.Nop:
 					break;
@@ -149,25 +168,16 @@ namespace Gendarme.Rules.Portability {
 					if (calledMethod.DeclaringType.FullName != "System.Environment")
 						break;
 
-					InspectionResult results = CheckInstruction (previous);
-					if (results == InspectionResult.Good)
+					InspectionResult result = CheckInstruction (previous);
+					if (result == InspectionResult.Good)
 						break;
 
-					if (messages == runner.RuleSuccess)
-						messages = new MessageCollection ();
-
-					Message message = null;
-					Location loc = new Location (method, current.Offset);
-					if (results == InspectionResult.Unsure)
-						message = new Message ("In Unix, unlike in Windows, process exit code can be a value between 0 and 255 inclusively. Be sure not to set it to values that are out of range.", loc, MessageType.Warning);
-					else // bad
-						message = new Message ("In Unix, unlike in Windows, process exit code can be a value between 0 and 255 inclusively. Do not set it to values that are out of range.", loc, MessageType.Error);
-					messages.Add (message);
+					Report (method, current, result);
 					break;
 				}
 				previous = current;
 			}
-			return messages;
+			return Runner.CurrentRuleResult;
 		}
 	}
 }
diff --git a/gendarme/rules/Gendarme.Rules.Portability/FeatureRequiresRootPrivilegeOnUnixRule.cs b/gendarme/rules/Gendarme.Rules.Portability/FeatureRequiresRootPrivilegeOnUnixRule.cs
index 632ca8b0..5fbd53b4 100644
--- a/gendarme/rules/Gendarme.Rules.Portability/FeatureRequiresRootPrivilegeOnUnixRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Portability/FeatureRequiresRootPrivilegeOnUnixRule.cs
@@ -38,8 +38,15 @@ using Gendarme.Framework;
 
 namespace Gendarme.Rules.Portability {
 
-	public class FeatureRequiresRootPrivilegeOnUnixRule : IMethodRule {
+	[Problem ("The method use some features that requires 'root' priviledge under Unix.")]
+	[Solution ("Make sure your code can work without requiring users to have 'root' priviledge.")]
+	public class FeatureRequiresRootPrivilegeOnUnixRule : Rule, IMethodRule {
 
+		// localizable
+		private const string ProcessMessage = "Setting Process.PriorityClass to something else than ProcessPriorityClass.Normal requires root privileges.";
+		private const string PingMessage = "Usage of System.Net.NetworkInformation.Ping requires root privileges.";
+
+		// non-localizable
 		private const string Ping = "System.Net.NetworkInformation.Ping";
 
 		//Check for usage of System.Diagnostics.Process.set_PriorityClass
@@ -86,47 +93,36 @@ namespace Gendarme.Rules.Portability {
 			case Code.Calli:
 			case Code.Callvirt:
 				MethodReference method = (ins.Operand as MethodReference);
-				return (method.DeclaringType.FullName == "System.Net.NetworkInformation.Ping");
+				return (method.DeclaringType.FullName == Ping);
 			}
 			return false;
 		}
 
-		public MessageCollection CheckMethod (MethodDefinition method, Runner runner)
+		public RuleResult CheckMethod (MethodDefinition method)
 		{
+			// rule does not apply to methods without IL
 			if (!method.HasBody)
-				return runner.RuleSuccess;
-
-			// do not apply the rule to the Ping class itself (i.e. when running Gendarme on System.dll 2.0+)
-			bool is_ping = (method.DeclaringType.FullName == Ping ||
-				(method.DeclaringType.IsNested && method.DeclaringType.DeclaringType.FullName == Ping));
+				return RuleResult.DoesNotApply;
 
-			MessageCollection results = null;
+			// Ping only exists in fx 2.0 and later
+			bool fx20 = (method.DeclaringType.Module.Assembly.Runtime >= TargetRuntime.NET_2_0);
 
 			foreach (Instruction ins in method.Body.Instructions) {
 
 				// Check for usage of System.Diagnostics.Process.set_PriorityClass
 				if (CheckProcessSetPriorityClass (ins)) {
-					if (results == null)
-						results = new MessageCollection ();
-
-					Location loc = new Location (method, ins.Offset);
-					Message msg = new Message ("Setting Process.PriorityClass to something else than ProcessPriorityClass.Normal requires root privileges.", loc, MessageType.Warning);
-					results.Add (msg);
+					// code won't work with default (non-root) users == High
+					Runner.Report (method, ins, Severity.High, Confidence.High, ProcessMessage);
 				}
 
 				// short-circuit
-				if (!is_ping && CheckPing (ins)) {
-					if (results == null)
-						results = new MessageCollection ();
-
-					Location loc = new Location (method, ins.Offset);
-					string s = String.Format ("Usage of {0} requires root privileges.", Ping);
-					Message msg = new Message (s, loc, MessageType.Warning);
-					results.Add (msg);
+				if (fx20 && CheckPing (ins)) {
+					// code won't work with default (non-root) users == High
+					Runner.Report (method, ins, Severity.High, Confidence.High, PingMessage);
 				}
 			}
 
-			return results;
+			return Runner.CurrentRuleResult;
 		}
 	}
 }
diff --git a/gendarme/rules/Gendarme.Rules.Portability/MonoCompatibilityReviewRule.cs b/gendarme/rules/Gendarme.Rules.Portability/MonoCompatibilityReviewRule.cs
index 4b61af37..333c0361 100644
--- a/gendarme/rules/Gendarme.Rules.Portability/MonoCompatibilityReviewRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Portability/MonoCompatibilityReviewRule.cs
@@ -27,51 +27,48 @@
 //
 
 using System;
-using System.Text;
 using System.Collections.Generic;
-using System.Diagnostics;
 using System.IO;
+using System.Net;
 
 using Mono.Cecil;
 using Mono.Cecil.Cil;
 using Gendarme.Framework;
 
-using ICSharpCode.SharpZipLib;
 using ICSharpCode.SharpZipLib.Zip;
 
 using MoMA.Analyzer.MoMAWebService;
 
 namespace Gendarme.Rules.Portability {
 
-	public class MonoCompatibilityReviewRule : IMethodRule {
+	[Problem ("The method has some known limitations when used with the Mono:: runtime.")]
+	[Solution ("Check if this code is critical to your application. Also make sure your definition files are up to date.")]
+	public class MonoCompatibilityReviewRule : Rule, IMethodRule {
+
+		private const string NotImplementedMessage = "{0} is not implemented.{1}";
+		private const string MissingMessage = "{0} is missing from Mono.{1}";
+		private const string TodoMessage = "{0} is marked with a [MonoTODO] attribute: {1}.";
 
-		private System.Net.WebException DownloadExceptionInternal;
 		private Dictionary<string, string> NotImplementedInternal; //value is unused
 		private Dictionary<string, string> MissingInternal; //value is unused
 		private Dictionary<string, string> TodoInternal; //value = TODO Description
 
-		public System.Net.WebException DownloadException {
-			get { return DownloadExceptionInternal; }
-			set { DownloadExceptionInternal = value; }
-		}
-
 		public Dictionary<string, string> NotImplemented {
 			get { return NotImplementedInternal; }
-			set { NotImplementedInternal = value; }
 		}
 
 		public Dictionary<string, string> Missing {
 			get { return MissingInternal; }
-			set { MissingInternal = value; }
 		}
 
-		public Dictionary<string, string> Todo {
+		public Dictionary<string, string> ToDo {
 			get { return TodoInternal; }
-			set { TodoInternal = value; }
 		}
 		
-		public MonoCompatibilityReviewRule ()
+		public override void Initialize (IRunner runner)
 		{
+			base.Initialize (runner);
+
 			string localAppDataFolder = Environment.GetFolderPath (Environment.SpecialFolder.LocalApplicationData);
 			string definitionsFolder = Path.Combine (localAppDataFolder, "Gendarme");
 			string definitionsFile = Path.Combine (definitionsFolder, "definitions.zip");
@@ -80,17 +77,8 @@ namespace Gendarme.Rules.Portability {
 				if (!Directory.Exists (definitionsFolder))
 					Directory.CreateDirectory (definitionsFolder);
 
-				try {
-					//try to download files from the net
-					MoMASubmit ws = new MoMASubmit ();
-					string definitionsUri = ws.GetLatestDefinitionsVersion ().Split ('|') [2];
-					ws.Dispose ();
-
-					System.Net.WebClient wc = new System.Net.WebClient ();
-					wc.DownloadFile (new Uri (definitionsUri), definitionsFile);
-				}
-				catch (System.Net.WebException e) {
-					DownloadException = e;
+				if (!Download (definitionsFile)) {
+					Active = false;
 					return;
 				}
 			}
@@ -100,18 +88,41 @@ namespace Gendarme.Rules.Portability {
 			while ((ze = zs.GetNextEntry ()) != null) {
 				switch (ze.Name) {
 				case "exception.txt":
-					NotImplemented = Read (new StreamReader (zs), false);
+					NotImplementedInternal = Read (new StreamReader (zs), false);
 					break;
 				case "missing.txt":
-					Missing = Read (new StreamReader (zs), false);
+					MissingInternal = Read (new StreamReader (zs), false);
 					break;
 				case "monotodo.txt":
-					Todo = Read (new StreamReader (zs), true);
+					TodoInternal = Read (new StreamReader (zs), true);
 					break;
 				default:
 					break;
 				}
 			}
+
+			// rule is active only if we have, at least one of, the MoMA files
+			Active = ((NotImplemented != null) || (Missing != null) || (ToDo != null));
+		}
+
+		private bool Download (string definitionsFile)
+		{
+			// try to download files from the net
+			try {
+				string definitionsUri;
+				using (MoMASubmit ws = new MoMASubmit ()) {
+					definitionsUri = ws.GetLatestDefinitionsVersion ().Split ('|') [2];
+				}
+
+				WebClient wc = new WebClient ();
+				wc.DownloadFile (new Uri (definitionsUri), definitionsFile);
+			}
+			catch (WebException e) {
+				if (Runner.VerbosityLevel > 0)
+					Console.WriteLine (e);
+				return false;
+			}
+			return true;
 		}
 
 		private static Dictionary<string, string> Read (StreamReader reader, bool split)
@@ -129,43 +140,26 @@ namespace Gendarme.Rules.Portability {
 			return dict;
 		}
 
-		private static void Check (Dictionary<string, string> dict, string calledMethod,
-				   MethodDefinition method, Instruction ins,
-				   ref MessageCollection results, string error, MessageType type)
+		private void Check (Dictionary<string, string> dict, MethodDefinition method, Instruction ins, string error, Severity severity)
 		{
-			if (dict == null)
-				return;
-			if (!dict.ContainsKey (calledMethod))
+			string callee = ins.Operand.ToString ();
+			if (!dict.ContainsKey (callee))
 				return;
 
-			if (results == null)
-				results = new MessageCollection ();
-
-			error = string.Format (error, calledMethod, dict [calledMethod]);
-			Location loc = new Location (method, ins.Offset);
-
-			Message msg = new Message (error, loc, type);
-			results.Add (msg);
+			string message = String.Format (error, callee, dict [callee]);
+			// confidence is Normal since we can't be sure if MoMA data is up to date
+			Runner.Report (method, ins, severity, Confidence.Normal, message);
 		}
 		
-		public MessageCollection CheckMethod (MethodDefinition method, Runner runner)
+		public RuleResult CheckMethod (MethodDefinition method)
 		{
+			// rule doesn't apply if method has no IL
 			if (!method.HasBody)
-				return runner.RuleSuccess;
-
-			MessageCollection results = null;
+				return RuleResult.DoesNotApply;
 
-			if (DownloadException != null) {
-				results = new MessageCollection ();
-				results.Add (new Message (string.Format ("Unable to read or download the definitions file: {0}.", DownloadException.Message), null, MessageType.Warning));
-				DownloadException = null;
-			}
-
-			if (NotImplemented == null && Missing == null && Todo == null)
-				return results;
+			// rule applies
 
 			foreach (Instruction ins in method.Body.Instructions) {
-
 				switch (ins.OpCode.Code) {
 				case Code.Call:
 				case Code.Calli:
@@ -174,23 +168,27 @@ namespace Gendarme.Rules.Portability {
 				case Code.Initobj:
 				case Code.Ldftn:
 				case Code.Ldvirtftn:
-
-					string calledMethodString = ins.Operand.ToString ();
-
-					Check (NotImplemented, calledMethodString, method, ins,
-					       ref results, "{0} is not implemented.", MessageType.Warning);
-					Check (Missing, calledMethodString, method, ins,
-					       ref results, "{0} is missing.", MessageType.Error);
-					Check (Todo, calledMethodString, method, ins,
-					       ref results, "{0} is marked with the MonoTODOAttribute ({1}).", MessageType.Warning);
-
+					// calling not implemented method is very likely not to work == High
+					if (NotImplemented != null) {
+						Check (NotImplemented, method, ins, NotImplementedMessage, Severity.High);
+					}
+
+					// calling missing methods can't work == Critical
+					if (Missing != null) {
+						Check (Missing, method, ins, MissingMessage, Severity.Critical);
+					}
+
+					// calling todo methods migh work with some limitations == Medium
+					if (ToDo != null) {
+						Check (ToDo, method, ins, TodoMessage, Severity.Medium);
+					}
 					break;
 				default:
 					break;
 				}
 			}
 
-			return results;
+			return Runner.CurrentRuleResult;
 		}
 	}
 }
diff --git a/gendarme/rules/Gendarme.Rules.Portability/NewLineLiteralRule.cs b/gendarme/rules/Gendarme.Rules.Portability/NewLineLiteralRule.cs
index 8ea63fa5..7413611d 100644
--- a/gendarme/rules/Gendarme.Rules.Portability/NewLineLiteralRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Portability/NewLineLiteralRule.cs
@@ -35,19 +35,20 @@ using Gendarme.Framework;
 
 namespace Gendarme.Rules.Portability {
 
-	public class NewLineLiteralRule: IMethodRule {
+	[Problem ("The method use some literal values for new lines (e.g. \\r\\n) which aren't portable across operating systems.")]
+	[Solution ("Replace literals with Environment.NewLine.")]
+	public class NewLineLiteralRule : Rule, IMethodRule {
 
 		private static char[] InvalidChar = { '\r', '\n' };
 
-		public MessageCollection CheckMethod (MethodDefinition method, Runner runner)
+		public RuleResult CheckMethod (MethodDefinition method)
 		{
 			// methods can be empty (e.g. p/invoke declarations)
 			if (!method.HasBody)
-				return runner.RuleSuccess;
+				return RuleResult.DoesNotApply;
 
 			// rule applies
 
-			MessageCollection results = null;
 			foreach (Instruction ins in method.Body.Instructions) {
 				switch (ins.OpCode.Code) {
 				case Code.Ldstr:
@@ -57,17 +58,11 @@ namespace Gendarme.Rules.Portability {
 						continue;
 
 					if (s.IndexOfAny (InvalidChar) >= 0) {
-						Location loc = new Location (method, ins.Offset);
 						// make the invalid char visible on output
 						s = s.Replace ("\n", "\\n");
 						s = s.Replace ("\r", "\\r");
-						Message msg = new Message (String.Format ("Found string: \"{0}\"", s),
-							loc, MessageType.Warning);
-
-						if (results == null)
-							results = new MessageCollection (msg);
-						else
-							results.Add (msg);
+						s = String.Format ("Found string: \"{0}\"", s);
+						Runner.Report (method, ins, Severity.Low, Confidence.High, s);
 					}
 					break;
 				default:
@@ -75,7 +70,7 @@ namespace Gendarme.Rules.Portability {
 				}
 			}
 
-			return results;
+			return Runner.CurrentRuleResult;
 		}
 	}
 }