Re: ConsiderReplacingPlatformSpecificPathsRule
Daniel Abramov
Here's slightly updated version. I renamed it to DoNotHardcodePathsRule.
Best,
Dan
Here's slightly updated version. I renamed it to DoNotHardcodePathsRule.
Best,
Dan
Sebastien Pouliot
A few minor things and it's ready to be committed after this :-)
Do you still have the resolving issue (parameters?) ? or is it fixed ?
Sebastien
On Thu, 2008-06-12 at 14:55 +0400, Daniel Abramov wrote:
> Occasionally I posted it to the wrong thread :P
>
> Here's slightly updated version. I renamed it to
> DoNotHardcodePathsRule.
>
> Best,
> Dan
>
...
> Index: rules/Test.Rules/Fixtures/RuleTestFixture.cs
> ===================================================================
> --- rules/Test.Rules/Fixtures/RuleTestFixture.cs (revision 105682)
> +++ rules/Test.Rules/Fixtures/RuleTestFixture.cs (working copy)
> @@ -39,7 +39,7 @@
> namespace Test.Rules.Fixtures {
Oops, I think we misunderstood each other for this change.
The point is to be able to check *any* defect individually, since they
can have different severity or confidence from each other.
Have you seen Cedric approach for the rule ? (patch on the group). I
think we could expose Defects the same way and use the "traditional"
NUnit Assert.* on them.
see example later
...
> Index: rules/Test.Rules/Fixtures/ChangeLog
> ===================================================================
> --- rules/Test.Rules/Fixtures/ChangeLog (revision 105682)
> +++ rules/Test.Rules/Fixtures/ChangeLog (working copy)
> @@ -1,3 +1,7 @@
> +2008-06-01 Daniel Abramov <e...@vingrad.ru>
> +
> + * RuleTestFixture.cs: Add AssertDefects ()
> +
> 2008-04-13 Sebastien Pouliot <seba...@ximian.com>
>
> * MethodRuleTestFixture.cs: Make sure all Assert* methods also
> Index: rules/Gendarme.Rules.Portability/Test/DoNotHardcodePathsTest.cs
> ===================================================================
> --- rules/Gendarme.Rules.Portability/Test/DoNotHardcodePathsTest.cs (revision 0)
> +++ rules/Gendarme.Rules.Portability/Test/DoNotHardcodePathsTest.cs (revision 0)
> @@ -0,0 +1,252 @@
> +//
> +// Unit tests for DoNotHardcodePathsRule
> +//
> +// Authors:
> +// Daniel Abramov <e...@vingrad.ru>
> +//
> +// Copyright (C) 2007 Daniel Abramov
> +//
> +// Permission is hereby granted, free of charge, to any person obtaining a copy
> +// of this software and associated documentation files (the "Software"), to deal
> +// in the Software without restriction, including without limitation the rights
> +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> +// copies of the Software, and to permit persons to whom the Software is
> +// furnished to do so, subject to the following conditions:
> +//
> +// The above copyright notice and this permission notice shall be included in
> +// all copies or substantial portions of the Software.
> +//
> +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> +// 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.Rules.Portability;
> +
> +using Test.Rules.Definitions;
> +using Test.Rules.Fixtures;
> +
> +using NUnit.Framework;
> +
> +namespace Test.Rules.Portability {
> +
> + [TestFixture]
> + public class DoNotHardcodePathsTest : MethodRuleTestFixture<DoNotHardcodePathsRule> {
> +
> + void TotalConfidence1 () // more than 13 points
> + {
> + // C:\directory\file.sys
> +
> + // drive letter 5
> + // no slashes 2
> + // 3-char extension 4
> + // var name contains 'file' 4
> + // TOTAL: 15 points
> + string pagefile = @"C:\directory\file.sys";
> + }
> +
> + void TotalConfidence2 () // more than 13 points
> + {
> + // /home/ex/.aMule/Incoming/Blues_Brothers.avi
> +
> + // starts with a slash 2
> + // starts with /home/ 4
> + // no backslashes 2
> + // 4 slashes 3
> + // 3-char extension 4
> + // param name contains 'file' 4
> + // TOTAL: 19 points
> + OpenFile (42, "/home/ex/.aMule/Incoming/Blues_Brothers.avi", "yarr!");
> + }
> +
> + void OpenFile (int something, string fileName, string somethingElse) { }
> +
> + void TotalConfidence3 () // more than 13 points
> + {
> + // /opt/mono/bin/mono
> +
> + // starts with a slash 2
> + // starts with /opt/ 4
> + // no backslashes 2
> + // 3 slashes 2
> + // 'file' in param 4
> + // TOTAL: 14 points
> + System.Diagnostics.Process.Start ("/opt/mono/bin/mono");
> + }
> +
> + void HighConfidence1 () // 10 to 13 points
> + {
> + // parser/data/English.nbin
> +
> + // no backslashes 2
> + // 4-char extension 3
> + // 2 slashes 2
> + // setter name contains 'path' 4
> + // TOTAL: 11 points
> + SomePath = "parser/data/English.nbin";
> + }
> +
> +
> + void HighConfidence2 () // 10 to 13 points
> + {
> + // \\SHARED\Music\Donovan\Mellow_Yellow.mp3
> +
> + // starts with \\ (UNC) 4
> + // no slashes 2
> + // 3 backslashes (except \\) 3
> + // 3-char extension 4
> + // TOTAL: 13 points
> + string music = @"\\SHARED\Music\Donovan\Mellow_Yellow.mp3";
> + }
> +
> + string SomePath {
> + set { }
> + }
> +
> + void NormalConfidence1 () // 8 to 9 points
> + {
> +
> + // bin\Debug\framework.dll
> +
> + // no slashes 2
> + // two backslashes 3
> + // 3-char extension 4
> + // TOTAL: 9 points
> +
> + string output = @"bin\Debug\framework.dll";
> + }
> +
> + void NormalConfidence2 () // 8 to 9 points
> + {
> +
> + // gendarme/bin/
> +
> + // no backslashes 2
> + // two slashes 2
> + // field name contains 'dir' 4
> + // TOTAL: 8 points
> +
> + gendarmeDirectory = @"gendarme/bin/";
> + }
> +
> + private string gendarmeDirectory;
> +
> +
> + void DontReportUris ()
> + {
> + string something = "http://somewhere.com/index.php";
> + }
> +
> + void DontReportSlashlessStrings ()
> + {
> + // we do not check strings that contain no (back)slashes
> + // since they can cause no portability problems
> + string file = "a elbereth gilthoniel.txt";
> + }
> +
> + void DontReportRegistryKeys ()
> + {
> + // they look like paths but they aren't
> + Microsoft.Win32.RegistryKey env = Microsoft.Win32.Registry.LocalMachine
> + .OpenSubKey (@"SYSTEM\CurrentControlSet\Control\Session Manager\Environment", true);
> + }
> +
> + void DontReportXML ()
> + {
> + string someXml = "<a><b /><c></c><b /></a>";
> + }
> +
> + void DontReportShortStrings ()
> + {
> + string someFile = "/";
> + }
> +
> + void DontReportStringsWithManyDots ()
> + {
> + string mimeType = "application/vnd.oasis-open.relax-ng.rnc";
> + }
> +
> + void DontReportXPath ()
> + {
> + System.Xml.XmlDocument doc = null;
> + System.Xml.XmlNode node = doc.SelectSingleNode ("/root/element/anotherelement");
> + }
> +
> + void DontReportRegexes ()
> + {
> + System.Text.RegularExpressions.Regex r = new System.Text.RegularExpressions.Regex (
> + @"^\s*"
> + + @"(((?<ORIGIN>(((\d+>)?[a-zA-Z]?:[^:]*)|([^:]*))):)"
> + + "|())"
> + + "(?<SUBCATEGORY>(()|([^:]*? )))"
> + + "(?<CATEGORY>(error|warning)) "
> + + "(?<CODE>[^:]*):"
> + + "(?<TEXT>.*)$",
> + System.Text.RegularExpressions.RegexOptions.IgnoreCase);
> + }
> +
> + [Test]
> + public void DoesNotApply ()
> + {
> + AssertRuleDoesNotApply (SimpleMethods.ExternalMethod);
> + }
> +
> + [Test]
> + public void EmptyOK ()
> + {
> + AssertRuleSuccess (SimpleMethods.EmptyMethod);
> + }
> +
> + [Test]
> + public void FailureTotalConfidence ()
> + {
> + AssertRuleFailure<DoNotHardcodePathsTest> ("TotalConfidence1");
keep total of defects like it was before (since it's the most useful for
most rules, i.e. with constant severity and confidence).
AssertRuleFailure<DoNotHardcodePathsTest> ("TotalConfidence1", 1);
> + AssertDefects (1, Confidence.Total);
a bit longer to type - but ultimate (i.e. future-proof) flexibility ;-)
Assert.AreEqual (Confidence.Total, Defects [0].Confidence, "Confidence[0]");
> + AssertRuleFailure<DoNotHardcodePathsTest> ("TotalConfidence2");
> + AssertDefects (1, Confidence.Total);
> + AssertRuleFailure<DoNotHardcodePathsTest> ("TotalConfidence3");
> + AssertDefects (1, Confidence.Total);
> + }
> +
> + [Test]
> + public void FailureHighConfidence ()
> + {
> + AssertRuleFailure<DoNotHardcodePathsTest> ("HighConfidence1");
> + AssertDefects (1, Confidence.High);
> + AssertRuleFailure<DoNotHardcodePathsTest> ("HighConfidence2");
> + AssertDefects (1, Confidence.High);
> + }
> +
> + [Test]
> + public void FailureNormalConfidence ()
> + {
> + AssertRuleFailure<DoNotHardcodePathsTest> ("NormalConfidence1");
> + AssertDefects (1, Confidence.Normal);
> + AssertRuleFailure<DoNotHardcodePathsTest> ("NormalConfidence2");
> + AssertDefects (1, Confidence.Normal);
> + }
> +
> + [Test]
> + public void IgnoreSomeCases ()
> + {
> + AssertRuleSuccess<DoNotHardcodePathsTest> ("DontReportUris");
> + AssertRuleSuccess<DoNotHardcodePathsTest> ("DontReportSlashlessStrings");
> + AssertRuleSuccess<DoNotHardcodePathsTest> ("DontReportRegistryKeys");
> + AssertRuleSuccess<DoNotHardcodePathsTest> ("DontReportXML");
> + AssertRuleSuccess<DoNotHardcodePathsTest> ("DontReportShortStrings");
> + AssertRuleSuccess<DoNotHardcodePathsTest> ("DontReportStringsWithManyDots");
> + AssertRuleSuccess<DoNotHardcodePathsTest> ("DontReportXPath");
> + AssertRuleSuccess<DoNotHardcodePathsTest> ("DontReportRegexes");
> + }
> + }
> +}
> Index: rules/Gendarme.Rules.Portability/Test/ChangeLog
> ===================================================================
> --- rules/Gendarme.Rules.Portability/Test/ChangeLog (revision 105682)
> +++ rules/Gendarme.Rules.Portability/Test/ChangeLog (working copy)
> @@ -1,3 +1,8 @@
> +2008-06-09 Daniel Abramov <e...@vingrad.ru>
> +
> + * DoNotHardcodePathsTest.cs: Unit tests for new rule.
> + * Test.Rules.Portability.mdp, Tests.Rules.Portability.csproj: Add new rule.
> +
> 2008-05-11 Andres G. Aragoneses <aarag...@novell.com>
>
> * Test.Rules.Portability.mdp: Fix references.
> Index: rules/Gendarme.Rules.Portability/Test/Test.Rules.Portability.mdp
> ===================================================================
> --- rules/Gendarme.Rules.Portability/Test/Test.Rules.Portability.mdp (revision 105682)
> +++ rules/Gendarme.Rules.Portability/Test/Test.Rules.Portability.mdp (working copy)
> @@ -19,6 +19,7 @@
> <File name="MonoCompatibilityReviewTest.cs" subtype="Code" buildaction="Compile" />
> <File name="ChangeLog" subtype="Code" buildaction="Nothing" />
> <File name="ExitCodeIsLimitedOnUnixTest.cs" subtype="Code" buildaction="Compile" />
> + <File name="DoNotHardcodePathsTest.cs" subtype="Code" buildaction="Compile" />
> </Contents>
> <References>
> <ProjectReference type="Project" localcopy="True" refto="Gendarme.Rules.Portability" />
> @@ -27,6 +28,7 @@
> <ProjectReference type="Gac" localcopy="True" refto="System, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089" />
> <ProjectReference type="Project" localcopy="True" refto="Mono.Cecil" />
> <ProjectReference type="Project" localcopy="True" refto="Test.Rules" />
> + <ProjectReference type="Gac" localcopy="True" refto="System.Xml, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089" />
> </References>
> <Deployment.LinuxDeployData generatePcFile="False" />
> <DeployTargets />
> Index: rules/Gendarme.Rules.Portability/Test/Tests.Rules.Portability.csproj
> ===================================================================
> --- rules/Gendarme.Rules.Portability/Test/Tests.Rules.Portability.csproj (revision 105682)
> +++ rules/Gendarme.Rules.Portability/Test/Tests.Rules.Portability.csproj (working copy)
> @@ -66,6 +66,7 @@
> </Compile>
> <Compile Include="MonoCompatibilityReviewTest.cs" />
> <Compile Include="NewLineLiteralTest.cs" />
> + <Compile Include="DoNotHardcodePathsTest.cs" />
> </ItemGroup>
> <ItemGroup>
> <ProjectReference Include="..\..\..\..\..\mcs\class\Mono.Cecil\Mono.Cecil.csproj">
> Index: rules/Gendarme.Rules.Portability/ChangeLog
> ===================================================================
> --- rules/Gendarme.Rules.Portability/ChangeLog (revision 105682)
> +++ rules/Gendarme.Rules.Portability/ChangeLog (working copy)
> @@ -1,3 +1,12 @@
> +2008-06-09 Daniel Abramov <e...@vingrad.ru>
> +
> + * DoNotHardcodePathsRule.cs: New rule that
> + looks for hardcoded path strings like 'C:\something.txt' or
> + '/usr/home' and warns the developer that they may break
> + application portability.
> + * Makefile.am, Gendarme.Rules.Portability.mdp,
> + * Gendarme.Rules.Portability.csproj: Add new rule.
> +
> 2008-05-27 Sebastien Pouliot <seba...@ximian.com>
>
> * MonoCompatibilityReviewRule.cs: Whine at the right stream
> Index: rules/Gendarme.Rules.Portability/Makefile.am
> ===================================================================
> --- rules/Gendarme.Rules.Portability/Makefile.am (revision 105682)
> +++ rules/Gendarme.Rules.Portability/Makefile.am (working copy)
> @@ -11,7 +11,8 @@
> portability_rules_sources_in = ../../AssemblyInfo.cs.in
> portability_rules_generated_sources = $(portability_rules_sources_in:.in=)
> portability_rules_sources = NewLineLiteralRule.cs FeatureRequiresRootPrivilegeOnUnixRule.cs \
> - MonoCompatibilityReviewRule.cs MoMAWebService.cs ExitCodeIsLimitedOnUnixRule.cs
> + MonoCompatibilityReviewRule.cs MoMAWebService.cs ExitCodeIsLimitedOnUnixRule.cs \
> + DoNotHardcodePathsRule.cs
>
> portability_rules_build_sources = $(addprefix $(srcdir)/, $(portability_rules_sources))
> portability_rules_build_sources += $(portability_rules_generated_sources)
> @@ -21,12 +22,13 @@
> -r:../../bin/Gendarme.Framework.dll -out:$@ $(portability_rules_build_sources)
>
> portability_test_sources = NewLineLiteralTest.cs FeatureRequiresRootPrivilegeOnUnixTest.cs \
> - MonoCompatibilityReviewTest.cs ExitCodeIsLimitedOnUnixTest.cs
> + MonoCompatibilityReviewTest.cs ExitCodeIsLimitedOnUnixTest.cs \
> + DoNotHardcodePathsTest.cs
> portability_test_build_sources = $(addprefix $(srcdir)/Test/, $(portability_test_sources))
>
> Test.Rules.Portability.dll: $(portability_test_build_sources) $(portability_rules_SCRIPTS)
> $(GMCS) -target:library -r:$(CECIL_ASM) -pkg:mono-nunit -r:../../bin/Gendarme.Framework.dll \
> - -r:$(portability_rules_SCRIPTS) -r:../Test.Rules/Test.Rules.dll -out:$@ $(portability_test_build_sources)
> + -r:$(portability_rules_SCRIPTS) -r:System.Xml.dll -r:../Test.Rules/Test.Rules.dll -out:$@ $(portability_test_build_sources)
>
> test: Test.Rules.Portability.dll
>
> Index: rules/Gendarme.Rules.Portability/DoNotHardcodePathsRule.cs
> ===================================================================
> --- rules/Gendarme.Rules.Portability/DoNotHardcodePathsRule.cs (revision 0)
> +++ rules/Gendarme.Rules.Portability/DoNotHardcodePathsRule.cs (revision 0)
> @@ -0,0 +1,472 @@
> +//
> +// Gendarme.Rules.Portability.DoNotHardcodePathsRule
> +//
> +// Authors:
> +// Daniel Abramov <e...@vingrad.ru>
> +//
> +// Copyright (C) 2008 Daniel Abramov
> +//
> +// Permission is hereby granted, free of charge, to any person obtaining a copy
> +// of this software and associated documentation files (the "Software"), to deal
> +// in the Software without restriction, including without limitation the rights
> +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> +// copies of the Software, and to permit persons to whom the Software is
> +// furnished to do so, subject to the following conditions:
> +//
> +// The above copyright notice and this permission notice shall be included in
> +// all copies or substantial portions of the Software.
> +//
> +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> +// OUT OF OR IN CONNECTION 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.Rocks;
> +
> +namespace Gendarme.Rules.Portability {
> +
> + [Problem ("This string looks like a path that may become invalid if the code is executed on a different operating system.")]
> + [Solution ("Use System.IO.Path and System.Environment types to generate paths instead of hardcoding them.")]
> + public class DoNotHardcodePathsRule : Rule, IMethodRule {
> +
> + // result cache
> + private static Dictionary<string, Confidence?> resultCache =
> + new Dictionary<string, Confidence?> ();
> +
> + // (back)slash counters
> + private int slashes;
> + private int backslashes;
> +
> + // method body being checked
> + private MethodBody method_body;
> +
> + // point counter
> + private int current_score;
> +
> + void AddPoints (int pts)
> + {
> + current_score += pts;
> + // Console.WriteLine ("// added {0} pts", pts);
> + }
> +
> + Confidence? CheckIfStringIsHardcodedPath (Instruction ldstr)
> + {
> + string str = (string) ldstr.Operand;
> +
> + // try to filter out false positives:
> +
> + // don't check too short strings
> + if (str.Length < 4)
> + return null;
> +
> + // count (back)slashes
> + slashes = CountOccurences (str, '/');
> + backslashes = CountOccurences (str, '\\');
> +
> + // if there's no (back)slashes, string's not going to cause
> + // any portability problems
> + if (slashes == 0 && backslashes == 0)
> + return null;
> +
> + // file paths don't contain //
> + if (str.Contains ("//"))
> + return null;
> +
> + // don't check XML strings
> + if (str.Contains ("</") || str.Contains ("/>"))
> + return null;
> +
> + // files paths don't usually have more than one dot (in extension)
> + if (CountOccurences (str, '.') > 2)
> + return null;
> +
> +
> + // handle different cases
> + if (CanBeWindowsAbsolutePath (str)) {
> + // whoooaaa! most probably we have a windows absolute path here
> + AddPoints (5); // add points (5 because '*:\*' is less common)
> + backslashes --; // we should't count a backslash in drive letter
> +
> + // process a windows path
> + ProcessWindowsPath (str);
> +
> + } else if (CanBeWindowsUNCPath (str)) {
> + AddPoints (4); // add points
> + backslashes -= 2;
> +
> + // go!
> + ProcessWindowsPath (str);
> +
> + } else if (CanBeUnixAbsolutePath (str)) {
> + // same for unix
> + AddPoints (2); // add points (2 because '/*' is more common)
> + slashes --; // we shouldn't count a slash in the beginning
> +
> + // process a unix path
> + ProcessUnixProbablyAbsolutePath (str);
> +
> + } else {
> + // since that's not an absolute path, we need to
> + // switch between unix/windows path handlers
> + // depending on what character is more common
> + // ('/' for unix, '\' for windows)
> +
> + if (backslashes > slashes)
> + ProcessWindowsPath (str); // like directory\something\..
> + else if (backslashes < slashes)
> + ProcessUnixPath (str); // like directory/something/..
> +
> + }
> +
> + // process the extension
> + ProcessExtension (System.IO.Path.GetExtension (str));
> +
> + // try to guess how the string is used
> + TryGuessUsage (ldstr);
> +
> + // Console.WriteLine ("// total score: {0}", current_score);
> +
> + if (current_score > 13)
> + return Confidence.Total;
> +
> + else if (current_score > 9)
> + return Confidence.High;
> +
> + else if (current_score > 7)
> + return Confidence.Normal;
> +
> + else return null;
> + }
> +
> + bool CanBeWindowsAbsolutePath (string s)
> + {
> + // true for strings like ?:\*
> + // e.g. 'C:\some\path' or 'D:\something.else"
> + return s [1] == ':' && s [2] == '\\';
> + }
> +
> + bool CanBeWindowsUNCPath (string s)
> + {
> + // true for Windows UNC paths
> + // e.g. \\Server\Directory\File
> + return s [0] == '\\' && s [1] == '\\';
> + }
> +
> + bool CanBeUnixAbsolutePath (string s)
> + {
> + // true for strings like /*
> + return s [0] == '/';
> + }
> +
> + void ProcessWindowsPath (string path)
> + {
> + // Console.WriteLine ("// process win path");
> +
> + // normally, windows paths don't contain slashes
> + if (slashes == 0 && backslashes > 1)
> + AddPoints (2);
> +
> + // the more backslashes there are
> + // the more we are convinced it's a path:
> +
> + if (backslashes > 3)
> + AddPoints (4);
> +
> + else if (backslashes > 1)
> + AddPoints (3);
> +
> + else if (backslashes == 1)
> + AddPoints (2);
> + }
> +
> + void ProcessUnixProbablyAbsolutePath (string path)
> + {
> + // check for common prefixes
> + if (path.StartsWith ("/bin/") ||
> + path.StartsWith ("/etc/") ||
> + path.StartsWith ("/sbin/") ||
> + path.StartsWith ("/dev/") ||
> + path.StartsWith ("/lib/") ||
> + path.StartsWith ("/usr/") ||
> + path.StartsWith ("/tmp/") ||
> + path.StartsWith ("/proc/") ||
> + path.StartsWith ("/sys/") ||
> + path.StartsWith ("/cdrom/") ||
> + path.StartsWith ("/home/") ||
> + path.StartsWith ("/media/") ||
> + path.StartsWith ("/mnt/") ||
> + path.StartsWith ("/opt/") ||
> + path.StartsWith ("/var/"))
> +
> + AddPoints (4);
> +
> + ProcessUnixPath (path);
> + }
> +
> + void ProcessUnixPath (string path)
> + {
> + // Console.WriteLine ("// process ux path");
> +
> + // normally, unix paths don't contain backslashes (unlike windows)
> + if (backslashes == 0)
> + AddPoints (2);
> +
> + // the more slashes there are
> + // the more we are convinced it's a path:
> +
> + if (slashes > 3)
> + AddPoints (3);
> +
> + else if (slashes > 1)
> + AddPoints (2);
> +
> + else if (slashes == 1)
> + AddPoints (1);
> + }
> +
> + void ProcessExtension (string ext)
> + {
> + // Console.WriteLine ("// process extension");
> +
> + int length = ext.Length;
> +
> + // now we look at the extension length
> + // NB: extension name also includes a dot (.)
> +
> + if (length < 2 || length > 6)
> + return;
> +
> + if (length == 4) // this is very common for extensions => really good sign :-)
> + AddPoints (4);
> +
> + else AddPoints (3); // less common but still good
> + }
> +
> + void TryGuessUsage (Instruction ldstr)
> + {
> + // Console.WriteLine ("// guess usage");
> +
> + // here we hope to get some additional points
> + // from further usage analysis
> +
> + // no further usage, good-bye!
> + if (ldstr.Next == null)
> + return;
> +
> + // we handle two cases:
> +
> + // #1: string can be stored into a local variable or field with a well-sounding name*
> + // * - well-sounding == name.Contains ("dir") || name.Contains ("file") || ...
> +
> + if (CheckIfStored (ldstr.Next))
> + return;
> +
> + // if we reach this point, it means #1 didn't catch anything
> +
> + // now, try option #2: navigate to the closest call(i|virt)? or newobj
> + // and see what we can learn
> +
> + Instruction current = ldstr.Next;
> + // this counter will be used to calculate parameter position
> + int paramOffset = 0;
> +
> + // take next instruction if any until it is call(i|virt)? or newobj
> + while (current != null && !CheckIfMethodOrCtorIsCalled (current, paramOffset)) {
> + current = current.Next;
> + paramOffset ++;
> + }
> + }
> +
> + // true == handled
> + // false == unhandled
> + bool CheckIfStored (Instruction afterLdstr)
> + {
> +
> + object operand = afterLdstr.Operand;
> + string name = null; // field or variable name (to be assigned later)
> +
> + switch (afterLdstr.OpCode.Code) {
> + case Code.Stfld: // store into field
> + name = ((FieldReference) operand).Name;
> + break;
you're not dealing with static fields (Stsfld)
> + case Code.Stloc: // store into local variable
> + case Code.Stloc_S:
> + if (operand is VariableDefinition)
> + name = ((VariableDefinition) operand).Name; // convert to var def
> + else // get var def by index
> + name = method_body.Variables [Convert.ToInt32 (operand)].Name;
> + break;
> + case Code.Stloc_0:
> + name = method_body.Variables [0].Name;
> + break;
> + case Code.Stloc_1:
> + name = method_body.Variables [1].Name;
> + break;
> + case Code.Stloc_2:
> + name = method_body.Variables [2].Name;
> + break;
> + case Code.Stloc_3:
> + name = method_body.Variables [3].Name;
> + break;
In general you can do stuff like
index = afterLdstr.OpCode.Code - Code.Stloc_0;
to simplify the code.
But now you can use the new Instruction rocks to simplify this
if (afterLdstr.IsStoreLoc ())
name = ins.GetVariable (method).Name;
> + }
> +
> + if (name == null)
> + return false; // unhandled
> +
> + CheckIdentifier (name);
> + return true; // handled
> + }
> +
> + // true == handled
> + // false == unhandled
> + bool CheckIfMethodOrCtorIsCalled (Instruction ins, int currentOffset)
> + {
> + switch (ins.OpCode.Code) {
> + case Code.Call:
> + case Code.Calli:
> + case Code.Callvirt:
> + // this is a call
> + MethodReference target = ins.Operand as MethodReference;
> +
> + // this happens sometimes so it's worth checking
> + if (target == null)
> + return true;
> +
> + // we can avoid some false positives by doing additional checks here
> +
> + string methodName = target.Name;
> + string typeName = target.DeclaringType.FullName;
> +
> + if (typeName.StartsWith ("Microsoft.Win32.Registry") // registry keys
> + || (typeName.StartsWith ("System.Xml") // xpath expressions
> + && methodName.StartsWith ("Select"))) {
> + AddPoints (-42);
> + return true; // handled
> + }
> +
> + // see what we can learn
> +
> + if (target.Name.StartsWith ("set_") &&
> + target.Parameters.Count == 1)
> + // to improve performance, don't Resolve ()
> + // this is a setter (in 99% cases)
> + CheckIdentifier (target.Name);
> + else
> + // we can also check parameter name
> + CheckMethodParameterName (target, currentOffset);
> +
> + break;
> +
> + case Code.Newobj:
> + // this is a constructor call
> + MethodReference ctor = (MethodReference) ins.Operand;
> + string createdTypeName = ctor.DeclaringType.FullName;
> +
> + // avoid catching regular expressions
> + if (createdTypeName == "System.Text.RegularExpressions.Regex")
> + AddPoints (-42);
> +
> + break;
> +
> + default:
> + return false;
> + }
> +
> + return true; // handled
> + }
> +
> + void CheckMethodParameterName (MethodReference methodReference, int parameterOffset)
> + {
> + MethodDefinition method = methodReference.Resolve ();
> + int parameterIndex = method.Parameters.Count - parameterOffset - 1;
> +
> + // to prevent some uncommon situations
> + if (parameterIndex < 0 || parameterIndex >= method.Parameters.Count)
> + return;
> +
> + // parameterOffset is distance in instructions between ldstr and call(i|virt)?
> + ParameterDefinition parameter = method.Parameters [method.Parameters.Count - parameterOffset - 1];
> +
> + // if its name is 'pathy', score some points!
> + CheckIdentifier (parameter.Name);
> + }
> +
> + void CheckIdentifier (string name)
> + {
> + if (IdentifierLooksLikePath (name))
> + AddPoints (4);
> + }
> +
> + bool IdentifierLooksLikePath (string name)
> + {
> + // Console.WriteLine ("// analyzing identifier '{0}'", name);
> +
> + name = name.ToLower ();
> + return name.Contains ("file") ||
> + name.Contains ("dir") ||
> + name.Contains ("path");
> + }
> +
> + int CountOccurences (string str, char c)
> + {
> + // helper method to tell how many times a certain character occurs in the string
> + int n = 0;
> + for (int i = 0; i < str.Length; i++)
> + if (str [i] == c)
> + n ++;
> + return n;
> + }
> +
> + public RuleResult CheckMethod (MethodDefinition method)
> + {
> + // if method has no IL, we don't check it
> + if (!method.HasBody)
> + return RuleResult.DoesNotApply;
> +
> + method_body = method.Body;
> +
> + // enumerate instructions to look for strings
> + foreach (Instruction ins in method.Body.Instructions) {
> + // Console.WriteLine ("{0} {1}", ins.OpCode, ins.Operand);
> +
> + if (ins.OpCode != OpCodes.Ldstr)
> + continue;
> +
> + slashes = backslashes = current_score = 0;
> +
> + // check if loaded string is a hardcoded path
> + Confidence? conf = GetConfidence (ins);
> +
> + // if sure enough, report the problem
> + if (conf != null)
> + Runner.Report (method, ins, Severity.High, conf.Value);
uho, you forgot my previous comment! Here the user *wants* to know which
offending string we're talking about. It's even more important for false
positives (no need to look at the code itself) or if debugging
information is not available (scanning all the code).
> + }
> +
> + return Runner.CurrentRuleResult;
> + }
> +
> + Confidence? GetConfidence (Instruction ldstr)
> + {
> + string str = (string) ldstr.Operand;
> +
> + // check if string is already in cache
> + if (resultCache.ContainsKey (str))
> + return resultCache [str];
> +
> + // get cached result
> + Confidence? result = CheckIfStringIsHardcodedPath (ldstr);
> + resultCache.Add (str, result);
> + return result;
> + }
> + }
> +}
> \ No newline at end of file
> Index: rules/Gendarme.Rules.Portability/Gendarme.Rules.Portability.csproj
> ===================================================================
> --- rules/Gendarme.Rules.Portability/Gendarme.Rules.Portability.csproj (revision 105682)
> +++ rules/Gendarme.Rules.Portability/Gendarme.Rules.Portability.csproj (working copy)
> @@ -61,6 +61,7 @@
> </ItemGroup>
> <ItemGroup>
> <Compile Include="ExitCodeIsLimitedOnUnixRule.cs" />
> + <Compile Include="DoNotHardcodePathsRule.cs" />
> <Compile Include="FeatureRequiresRootPrivilegeOnUnixRule.cs" />
> <Compile Include="MoMAWebService.cs" />
> <Compile Include="MonoCompatibilityReviewRule.cs" />
> Index: rules/Gendarme.Rules.Portability/Gendarme.Rules.Portability.mdp
> ===================================================================
> --- rules/Gendarme.Rules.Portability/Gendarme.Rules.Portability.mdp (revision 105682)
> +++ rules/Gendarme.Rules.Portability/Gendarme.Rules.Portability.mdp (working copy)
> @@ -19,6 +19,7 @@
> <File name="MoMAWebService.cs" subtype="Code" buildaction="Compile" />
> <File name="MonoCompatibilityReviewRule.cs" subtype="Code" buildaction="Compile" />
> <File name="ExitCodeIsLimitedOnUnixRule.cs" subtype="Code" buildaction="Compile" />
> + <File name="DoNotHardcodePathsRule.cs" subtype="Code" buildaction="Compile" />
> <File name="ChangeLog" subtype="Code" buildaction="Nothing" />
> </Contents>
> <References>
>
Exception
On 17 июн, 05:07, Sebastien Pouliot <sebastien.poul...@gmail.com>
wrote:
>
> A few minor things and it's ready to be committed after this :-)
>
> Do you still have the resolving issue (parameters?) ? or is it fixed ?
>
> Sebastien
>
>
>
>
>
> On Thu, 2008-06-12 at 14:55 +0400, Daniel Abramov wrote:
> > Occasionally I posted it to the wrong thread :P
>
> > Here's slightly updated version. I renamed it to
> > DoNotHardcodePathsRule.
>
> > Best,
> > Dan
>
> ...
> > Index: rules/Test.Rules/Fixtures/RuleTestFixture.cs
> > ===================================================================
> > --- rules/Test.Rules/Fixtures/RuleTestFixture.cs (revision 105682)
> > +++ rules/Test.Rules/Fixtures/RuleTestFixture.cs (working copy)
> > @@ -39,7 +39,7 @@
> > namespace Test.Rules.Fixtures {
>
> Oops, I think we misunderstood each other for this change.
>
> The point is to be able to check *any* defect individually, since they
> can have different severity or confidence from each other.
>
> Have you seen Cedric approach for the rule ? (patch on the group). I
> think we could expose Defects the same way and use the "traditional"
> NUnit Assert.* on them.
much stuff :D
>
> see example later
OK.
>
> ...
>
> > Index: rules/Test.Rules/Fixtures/ChangeLog
> > ===================================================================
> > --- rules/Test.Rules/Fixtures/ChangeLog (revision 105682)
> > +++ rules/Test.Rules/Fixtures/ChangeLog (working copy)
> > @@ -1,3 +1,7 @@
> > +2008-06-01 Daniel Abramov <e...@vingrad.ru>
> > +
> > + * RuleTestFixture.cs: Add AssertDefects ()
> > +
>
> продолжение >>
Sebastien Pouliot
On Tue, 2008-06-17 at 06:48 -0700, Exception wrote:
...
> > Oops, I think we misunderstood each other for this change.
> >
> > The point is to be able to check *any* defect individually, since
> they
> > can have different severity or confidence from each other.
> >
> > Have you seen Cedric approach for the rule ? (patch on the group). I
> > think we could expose Defects the same way and use the "traditional"
> > NUnit Assert.* on them.
>
> Nope, I haven't. Which one to look at? I'm afraid Cedric posted quite
> much stuff :D
the shortest one ;-) "do not count external types in inheritance depth"
Sebastien