diff options
author | Sebastien Pouliot <sebastien@ximian.com> | 2008-02-15 22:56:22 +0300 |
---|---|---|
committer | Sebastien Pouliot <sebastien@ximian.com> | 2008-02-15 22:56:22 +0300 |
commit | 62e2e332d1e50a146372237d304cff7fee415db0 (patch) | |
tree | cb6b788a7f81bec0c0c839e9fe81447ec4473cec /gendarme/rules | |
parent | a22ed0764a8dc295e02e9820ee339d6d8ef2178e (diff) |
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
Diffstat (limited to 'gendarme/rules')
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; } } } |