[PATCH] new AvoidUnnecessarySpecializationRule
Cedric Vivier
This new method rule in the Maintainability category suggests using a
less specialized method's parameter type when the specialization is
actually not necessary.
(i.e the method does not use any specific of the type).
Since an example is worth a thousand words :
public bool HasNumber(System.Collections.Generic.List<int> numbers, int number)
{
foreach (int i in numbers)
if (i == number) return true;
return false;
}
Will emit a failure with :
* Parameter `numbers` could be of type
`System.Collections.Generic.IEnumerable`1`.
Cheers,
ps: the patch also contains two small fixes for TypeRocks and StackEntryAnalysis
Cedric Vivier
in the specialization calculus.
+ use StackEntryAnalysis.UsageResult's new name as of rev. 105267
Attached both full patch against HEAD and diff patch from previous patch.
Cedric Vivier
parameter specialization. As in :
public void GenericMethod<T> (T x) where T : System.ArgumentException
{
Console.WriteLine (x.Message);
}
==> Parameter 'x' could be constrained to type System.Exception
Attached both full patch and diff from v2.
Exception
On 9 ะธัะฝ, 03:41, "Cedric Vivier" <cedr...@neonux.com> wrote:
> Third version with support for suggesting about constrained generic
> parameter specialization. As in :
>
> public void GenericMethod<T> (T x) where T : System.ArgumentException
> {
> ย ย Console.WriteLine (x.Message);
>
> }
>
> ==> Parameter 'x' could be constrained to type System.Exception
>
> Attached both full patch and diff from v2.
>
> > Improved version of the rule that does not ignore static field stores
> > in the specialization calculus.
> > + use StackEntryAnalysis.UsageResult's new name as of rev. 105267
>
> > Attached both full patch against HEAD and diff patch from previous patch.
>
> >> Hi everyone!
>
> >> This new method rule in the Maintainability category suggests using a
> >> less specialized method's parameter type when the specialization is
> >> actually not necessary.
> >> (i.e the method does not use any specific of the type).
>
> >> Since an example is worth a thousand words :
>
> >> public bool HasNumber(System.Collections.Generic.List<int> numbers, int number)
> >> {
> >> ย ย foreach (int i in numbers)
> >> ย ย ย ย if (i == number) return true;
> >> ย ย return false;
> >> }
>
> >> Will emit a failure with :
> >> * Parameter `numbers` could be of type
> >> `System.Collections.Generic.IEnumerable`1`.
>
> >> Cheers,
>
> >> ps: the patch also contains two small fixes for TypeRocks and StackEntryAnalysis
>
>
>
> 30Kะะฐะณััะทะธัั
>
> ย AvoidUnnecessarySpecializationRule.v3_diff.patch
> 8Kะะฐะณััะทะธัั
Sebastien Pouliot
It looks great - but you need to start using "make self-test" ;-)
A few methods are not well covered by unit tests (e.g.
IsSystemObjectMethod always return false).
Sebastien
p.s. non-existing rocks are (always) suggestions - you can commit
without them (i.e. we can fix this later).
On Mon, 2008-06-09 at 01:41 +0200, Cedric Vivier wrote:
>
>
>
>
>
> text/x-diff attachment (AvoidUnnecessarySpecializationRule.v3.patch)
>
> diff --git a/gendarme/framework/Gendarme.Framework.Helpers/StackEntryAnalysis.cs b/gendarme/framework/Gendarme.Framework.Helpers/StackEntryAnalysis.cs
> index 5e18624..df94fcf 100644
> --- a/gendarme/framework/Gendarme.Framework.Helpers/StackEntryAnalysis.cs
> +++ b/gendarme/framework/Gendarme.Framework.Helpers/StackEntryAnalysis.cs
> @@ -416,7 +416,8 @@ namespace Gendarme.Framework.Helpers {
>
> }
>
> - if (ins.OpCode.Code == Code.Endfinally) { //pop the last leave instruction and branch to it
> + //pop the last leave instruction and branch to it
> + if (ins.OpCode.Code == Code.Endfinally && null != insWithLeave.LeaveStack) {
> LoadAlternatives.AddIfNew (insWithLeave.Pop ());
> break;
> }
> diff --git a/gendarme/framework/Gendarme.Framework.Rocks/TypeRocks.cs b/gendarme/framework/Gendarme.Framework.Rocks/TypeRocks.cs
> index c0bebb4..4f81654 100644
> --- a/gendarme/framework/Gendarme.Framework.Rocks/TypeRocks.cs
> +++ b/gendarme/framework/Gendarme.Framework.Rocks/TypeRocks.cs
> @@ -344,8 +344,8 @@ namespace Gendarme.Framework.Rocks {
> public static bool IsDelegate (this TypeReference self)
> {
> TypeDefinition type = self.Resolve ();
> - // e.g. this occurs for <Module>
> - if (type.BaseType == null)
> + // e.g. this occurs for <Module> or GenericParameter
> + if (null == type || type.BaseType == null)
> return false;
>
> switch (type.BaseType.FullName) {
> diff --git a/gendarme/rules/Gendarme.Rules.Maintainability/AvoidUnnecessarySpecializationRule.cs b/gendarme/rules/Gendarme.Rules.Maintainability/AvoidUnnecessarySpecializationRule.cs
> new file mode 100644
> index 0000000..30a53c0
> --- /dev/null
> +++ b/gendarme/rules/Gendarme.Rules.Maintainability/AvoidUnnecessarySpecializationRule.cs
> @@ -0,0 +1,341 @@
> +//
> +// Gendarme.Rules.Maintainability.AvoidUnnecessarySpecializationRule class
> +//
> +// Authors:
> +// Cedric Vivier <ced...@neonux.com>
> +//
> +// Copyright (C) 2008 Cedric Vivier
> +//
> +// 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 Mono.Cecil;
> +using Mono.Cecil.Cil;
> +
> +using Gendarme.Framework;
> +using Gendarme.Framework.Rocks;
> +using Gendarme.Framework.Helpers;
> +
> +namespace Gendarme.Rules.Maintainability {
> +
> + [Problem ("This method has a parameter whose type is more specialized than necessary. It can be harder to reuse and/or extend the method in derived types.")]
> + [Solution ("Replace parameter type with the least specialized type necessary, or make use of the specifics of the actual parameter type.")]
> + public class AvoidUnnecessarySpecializationRule : Rule, IMethodRule {
> +
> + private StackEntryAnalysis sea;
> + private TypeReference[] leastTypes;
> + private int[] leastDepths;
having similar fields prefix triggers AvoidLargeClassRule
> +
> + private static ParameterDefinition GetParameter (MethodDefinition method, Instruction ins)
> + {
> + switch (ins.OpCode.Code) {
> + case Code.Ldarg_0:
> + case Code.Ldarg_1:
> + case Code.Ldarg_2:
> + case Code.Ldarg_3:
> + int index = ins.OpCode.Code - Code.Ldarg_0;
> + if (!method.IsStatic) {
> + index--;
> + if (index < 0) return null;
> + }
> + return method.Parameters [index];
> + case Code.Ldarg_S :
> + case Code.Ldarga_S :
spaces versus tabs
> + case Code.Ldarg:
> + case Code.Ldarga:
> + return (ins.Operand as ParameterDefinition);
> + }
> +
> + throw new ArgumentException("not a ldarg instruction", "ins");
> + }
Make a rock for the above code - but make is return null for default.
> + private static TypeReference GetActualType (TypeReference type)
> + {
> + if (type is GenericParameter && 1 == ((GenericParameter) type).Constraints.Count)
> + return ((GenericParameter) type).Constraints [0];
> + return type;
> + }
That looks like something we should be using elsewhere - but I never
seen/used code like this.
> + private static int GetActualTypeDepth (TypeReference type)
> + {
> + return GetTypeDepth (GetActualType (type));
> + }
> +
> + private static int GetTypeDepth (TypeReference type)
> + {
> + if (type is GenericParameter && 1 == ((GenericParameter) type).Constraints.Count)
> + type = ((GenericParameter) type).Constraints [0];
> + return GetTypeDepth (type.Resolve ());
> + }
The difference with the above code is only the Resolve, right ?
i.e. this triggers the AvoidCodeDuplicatedInSameClassRule
> +
> + private static int GetTypeDepth (TypeDefinition type)
> + {
> + int depth = 0;
> + while (null != type && type.BaseType != null) {
> + depth++;
> + type = type.BaseType.Resolve ();
> + }
> + return depth;
> + }
> +
> + private static TypeDefinition GetBaseImplementor (TypeReference type, MethodSignature signature)
> + {
> + return GetBaseImplementor (type.Resolve(), signature);
> + }
> +
> + private static TypeDefinition GetBaseImplementor (TypeDefinition type, MethodSignature signature)
> + {
> + TypeDefinition implementor = type;
> +
> + while (null != type && type.IsVisible ()) {
> + //search for matching interface
> + TypeDefinition ifaceDef = GetInterfaceImplementor (type, signature);
> + if (null != ifaceDef)
> + implementor = ifaceDef;
> + else if (null != type.GetMethod (signature))
> + implementor = type;
> + type = type.BaseType.Resolve ();
> + }
> +
> + return implementor;
> + }
> +
> + private static TypeDefinition GetInterfaceImplementor (TypeDefinition type, MethodSignature signature)
> + {
> + TypeDefinition ifaceDef = null;
> +
> + foreach (TypeReference iface in type.Interfaces) {
> + TypeDefinition candidate = iface.Resolve ();
> +
> + if (!candidate.IsVisible () || candidate.Name.StartsWith("_"))
> + continue; //ignore non-cls-compliant interfaces
> + if (null == candidate.GetMethod (signature))
> + continue; //does not implement
> +
> + if (null == ifaceDef) {
> + ifaceDef = candidate;
> + } else if (ifaceDef.GenericParameters.Count < candidate.GenericParameters.Count) {
> + //prefer the most generic interface
> + ifaceDef = candidate;
> + } else if (ifaceDef.Interfaces.Count < candidate.Interfaces.Count) {
> + //prefer the most specific interface
> + ifaceDef = candidate;
> + } else if (ifaceDef.Interfaces.Count >= candidate.Interfaces.Count
> + || ifaceDef.GenericParameters.Count >= candidate.GenericParameters.Count) {
> + continue; //we already have a better match
> + } else {
> + //ambiguous/explicit, ignore
> + ifaceDef = null;
> + break;
> + }
> + }
> +
> + return ifaceDef;
> + }
> +
> + private static MethodSignature GetSignature (MethodReference method)
> + {
> + string[] parameters = new string [method.Parameters.Count];
> + for (int i = 0; i < method.Parameters.Count; ++i) {
> + TypeReference pType = method.Parameters [i].ParameterType;
> + if (pType is GenericParameter)
> + parameters [i] = null; //TODO: constructed mapping?
> + else
> + parameters [i] = pType.FullName;
> + }
> + return new MethodSignature (method.Name, GetReturnTypeSignature (method), parameters);
> + }
> +
> + private static string GetReturnTypeSignature (MethodReference method)
> + {
> + if (method.Name == "GetEnumerator" && 0 == method.Parameters.Count) //special exception
> + return null;
> + return method.ReturnType.ReturnType.FullName;
> + }
This rule triggers itself on the above method ;-) and propose
IMemberReference
> + private static bool IsSystemObjectMethod (MethodReference method)
> + {
> + switch (method.Name) {
> + case "Finalize" :
> + case "GetHashCode" :
> + case "GetType" :
> + case "MemberwiseClone" :
> + case "ToString" :
> + return method.Parameters.Count == 0;
> + case "Equals" :
> + return method.Parameters.Count == 1 || method.Parameters.Count == 2;
> + case "ReferenceEquals" :
> + return method.Parameters.Count == 2;
> +
> + //HACK: BOO:
> + case "EqualityOperator" :
> + return method.Parameters.Count == 2 && method.DeclaringType.FullName == "Boo.Lang.Runtime.RuntimeServices";
> + }
> + return false;
> + }
This rule triggers itself on the above method ;-) and propose
IMemberReference
> +
> + private void UpdateParameterLeastType (ParameterDefinition parameter, StackEntryUsageResult[] usageResults)
> + {
> + int pIndex = parameter.Sequence - 1;
> + if (pIndex < 0) throw new InvalidOperationException("parameter.Sequence < 1");
split lines between if and the conditional code
> + int parameterDepth = GetActualTypeDepth (parameter.ParameterType);
> +
> + TypeReference currentLeastType = null;
> + int currentLeastDepth = 0;
> +
> + foreach (var usage in usageResults) {
> + bool needUpdate = false;
> +
> + switch (usage.Instruction.OpCode.Code) {
> + case Code.Newobj :
> + case Code.Call :
> + case Code.Callvirt :
> + MethodReference method = (MethodReference) usage.Instruction.Operand;
> +
> + //potential generalization to object does not really make sense
> + //from a readability/maintainability point of view
> + if (IsSystemObjectMethod (method)) continue;
> +
> + MethodSignature signature = GetSignature (method);
> +
> + if (usage.StackOffset == method.Parameters.Count) {
> + //argument is used as `this` in the call
> + currentLeastType = GetBaseImplementor (GetActualType (method.DeclaringType), signature);
> + } else {
> + //argument is also used as an argument in the call
> + currentLeastType = method.Parameters [method.Parameters.Count - usage.StackOffset - 1].ParameterType;
> + }
> +
> + //if the best we could find is object, ignore this round
> + if (currentLeastType.FullName == "System.Object") continue;
split lines between if and the conditional code
> + needUpdate = true;
> + break;
> +
> + case Code.Stfld :
> + case Code.Stsfld :
> + FieldReference field = (FieldReference) usage.Instruction.Operand;
> + currentLeastType = field.FieldType;
> +
> + needUpdate = true;
> + break;
> + }
> +
> + if (needUpdate) {
> + currentLeastDepth = GetActualTypeDepth (currentLeastType);
> + if (null == leastTypes [pIndex] || currentLeastDepth > leastDepths [pIndex]) {
> + leastTypes [pIndex] = currentLeastType;
> + leastDepths [pIndex] = currentLeastDepth;
> + }
> + if (currentLeastDepth == parameterDepth) //no need to check further
> + return;
> + }
> + }
> + }
This rule triggers itself on the above method ;-) and propose
ParameterReference
> + private void CheckParameter (MethodDefinition method, Instruction ins)
> + {
> + ParameterDefinition parameter = GetParameter (method, ins);
> + if (null == parameter) //this is `this`, we do not care
> + return;
> + if (parameter.IsOut || parameter.IsOptional || parameter.ParameterType.IsValueType)
> + return;
> + if (parameter.ParameterType.IsArray () || parameter.ParameterType.IsDelegate ())
> + return; //TODO: these are more complex to handle, not supported for now
> +
> + if (null == sea || sea.Method != method)
> + sea = new StackEntryAnalysis (method);
> +
> + StackEntryUsageResult [] usage = sea.GetStackEntryUsage (ins);
> +
> + //FIXME: below opt. would require a non-repeating SEA with multiple ldarg
> + //if (null != leastTypes [parameter.Sequence-1]) //already analyzed, next!
> + // return;
> +
> + UpdateParameterLeastType (parameter, usage);
> + }
> +
> + public RuleResult CheckMethod (MethodDefinition method)
> + {
> + if (!method.HasBody || method.IsGeneratedCode () || method.IsCompilerControlled)
> + return RuleResult.DoesNotApply;
> + if (method.Parameters.Count == 0 || method.IsProperty ())
> + return RuleResult.DoesNotApply;
> +
> + leastTypes = new TypeReference [method.Parameters.Count];
> + leastDepths = new int [leastTypes.Length];
> +
> + //look at each argument usage
> + foreach (Instruction ins in method.Body.Instructions) {
> + switch (ins.OpCode.Code) {
> + case Code.Ldarg_0 :
> + case Code.Ldarg_1 :
> + case Code.Ldarg_2 :
> + case Code.Ldarg_3 :
> + case Code.Ldarg_S :
> + case Code.Ldarga_S :
> + case Code.Ldarg :
> + case Code.Ldarga :
> + CheckParameter (method, ins);
A IsParamater rocks would be useful.
> + break;
> + default:
> + continue;
> + }
> + }
> +
> + CheckParametersSpecializationDelta (method);
> +
> + return Runner.CurrentRuleResult;
> + }
> +
> + private void CheckParametersSpecializationDelta (MethodDefinition method)
> + {
> + for (int i = 0; i < method.Parameters.Count; ++i) {
> + if (null == leastTypes [i]) continue; //argument is not used
split lines between if and the conditional code
> + ParameterDefinition parameter = method.Parameters [i];
> + int delta = GetActualTypeDepth (parameter.ParameterType) - leastDepths [i];
> +
> + if (delta > 0) {
> + string message = GetSuggestionMessage (parameter);
> + Severity sev = (delta < 3) ? Severity.Medium : Severity.High;
> + Runner.Report (method, sev, Confidence.High, message);
> + }
> + }
> + }
This rule triggers itself on the above method ;-) and propose
IMethodSignature
> + private string GetSuggestionMessage (ParameterDefinition parameter)
> + {
> + System.Text.StringBuilder sb = new System.Text.StringBuilder ();
> + sb.Append ("Parameter '");
> + sb.Append (parameter.Name);
> + if (parameter.ParameterType is GenericParameter)
> + sb.Append ("' could be constrained to type '");
> + else
> + sb.Append ("' could be of type '");
> + sb.Append (leastTypes [parameter.Sequence - 1].FullName);
> + sb.Append ("'.");
> + return sb.ToString ();
> + }
This rule triggers itself on the above method ;-) and propose
ParameterReference
> + }
> +
> +}
> +
> diff --git a/gendarme/rules/Gendarme.Rules.Maintainability/Makefile.am b/gendarme/rules/Gendarme.Rules.Maintainability/Makefile.am
> index 26d7524..5fb5ff8 100644
> --- a/gendarme/rules/Gendarme.Rules.Maintainability/Makefile.am
> +++ b/gendarme/rules/Gendarme.Rules.Maintainability/Makefile.am
> @@ -14,6 +14,7 @@ maintainability_rules_sources = \
> AvoidComplexMethodsRule.cs \
> AvoidDeepInheritanceTreeRule.cs \
> AvoidLackOfCohesionOfMethodsRule.cs \
> + AvoidUnnecessarySpecializationRule.cs \
> PreferStringIsNullOrEmptyRule.cs
>
> maintainability_rules_build_sources = $(addprefix $(srcdir)/, $(maintainability_rules_sources))
> @@ -27,6 +28,7 @@ maintainability_test_sources = \
> AvoidComplexMethodsTest.cs \
> AvoidDeepInheritanceTreeTest.cs \
> AvoidLackOfCohesionOfMethodsTest.cs \
> + AvoidUnnecessarySpecializationTest.cs \
> PreferStringIsNullOrEmptyTest.cs
>
> maintainability_test_build_sources = $(addprefix $(srcdir)/Test/, $(maintainability_test_sources))
> diff --git a/gendarme/rules/Gendarme.Rules.Maintainability/Test/AvoidUnnecessarySpecializationTest.cs b/gendarme/rules/Gendarme.Rules.Maintainability/Test/AvoidUnnecessarySpecializationTest.cs
> new file mode 100644
> index 0000000..b964e7e
> --- /dev/null
> +++ b/gendarme/rules/Gendarme.Rules.Maintainability/Test/AvoidUnnecessarySpecializationTest.cs
> @@ -0,0 +1,357 @@
> +//
> +// Unit tests for AvoidUnnecessarySpecializationRule
> +//
> +// Authors:
> +// Cedric Vivier <ced...@neonux.com>
> +//
> +// Copyright (C) 2008 Cedric Vivier
> +//
> +// 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.Reflection;
> +using System.Collections;
> +using System.Collections.Generic;
> +
> +using Mono.Cecil;
> +
> +using Gendarme.Framework;
> +using Gendarme.Framework.Rocks;
> +using Gendarme.Rules.Maintainability;
> +
> +using NUnit.Framework;
> +using Test.Rules.Fixtures;
> +
> +namespace Test.Rules.Maintainability {
> +
> + public class Base {
> + public virtual void Foo()
> + {
> + }
> + }
> +
> + public class Derived : Base {
> + public override void Foo()
> + {
> + }
> +
> + public void Bar(int x, string y)
> + {
> + }
> + }
> +
> + public class DerivedDerived : Derived {
> + public DerivedDerived (Base parent)
> + {
> + }
> +
> + public override void Foo()
> + {
> + }
> +
> + public new void Bar(int x, string y)
> + {
> + }
> +
> + public void DerivedDerivedSpecific()
> + {
> + }
> + }
> +
> +
> + public class GeneralizedClass {
> +
> + public void FooCouldBeBase (Base foo)
> + {
> + foo.Foo ();
> + }
> +
> + public void DerivedDerivedSpecificCouldNotBeGeneralized (DerivedDerived specific)
> + {
> + specific.Bar (999, "bar");
> + specific.DerivedDerivedSpecific ();
> + specific.Foo ();
> + }
> +
> + public void DerivedBarCouldNotBeGeneralized (Derived bar)
> + {
> + bar.Bar (0, string.Empty);
> + }
> +
> + public int BarAndFooCouldNotBeGeneralized (string foo, Derived bar)
> + {
> + bar.Bar (foo.Length, foo);
> + return 0;
> + }
> +
> + public void BarAsGeneralizedArgument (string hash, Derived dbar)
> + {
> + dbar.Foo (); //if bar wasn't used as a Derived argument below it would be eligible
> + DerivedBarCouldNotBeGeneralized(dbar);
> + }
> +
> + public bool Interface (IEnumerable list, int number)
> + {
> + foreach (int i in list)
> + if (number == i)
> + return true;
> + return false;
> + }
> +
> + public bool GenericInterface (IEnumerable<int> list, int number)
> + {
> + foreach (int i in list)
> + if (number == i)
> + return true;
> + return false;
> + }
> +
> + public int Property {
> + get { return prop; }
> + set { prop = value; }
> + }
> + int prop;
> +
> + public void ParameterLessMethod ()
> + {
> + }
> +
> + public void GenericAddBar (Base bar)
> + {
> + List<Base> l = new List<Base>();
> + l.Add(bar);
> + }
> +
> + public DerivedDerived Constructor (Base bar)
> + {
> + return new DerivedDerived (bar);
> + }
> +
> + public Derived derived;
> + public void Stfld (Derived bar)
> + {
> + derived = bar;
> + }
> + public void Stfld2 (DerivedDerived bar)
> + {
> + derived = bar;//no warn since we use specific below
> + bar.DerivedDerivedSpecific ();
> + }
> +
> + public static Derived sderived;
> + public void Stsfld (Derived bar)
> + {
> + sderived = bar;
> + }
> + public void Stsfld2 (DerivedDerived bar)
> + {
> + sderived = bar;//no warn since we use specific below
> + bar.DerivedDerivedSpecific ();
> + }
> +
> + public void GenericMethod<T> (T x) where T : System.Exception
> + {
> + Console.WriteLine (x.Message);
> + }
> + }
> +
> + public class SpecializedClass {
> +
> + public void FooCouldBeBase (Derived foo)
> + {
> + foo.Foo ();
> + }
> +
> + public void DerivedDerivedFooCouldBeBase (int dummy, DerivedDerived foo)
> + {
> + foo.Foo ();
> + }
> +
> + public void DerivedDerivedBarCouldBeDerived (DerivedDerived bar)
> + {
> + bar.Bar (0, null);
> + }
> +
> + public void BarAndFooCouldBeGeneralized (Derived foo, DerivedDerived bar)
> + {
> + bar.Bar (42, "hash");
> + foo.Foo ();
> + }
> +
> + public int BarCouldBeGeneralizedButNotFoo (string foo, DerivedDerived bar)
> + {
> + bar.Bar (42, "hash");
> + return foo.Length;
> + }
> +
> + public int BarAsSpecializedArgument (string hash, DerivedDerived bar)
> + {
> + FooCouldBeBase(bar);
> + return 0;
> + }
> +
> + //`list` could be a IEnumerable
> + public bool Interface (ArrayList list, int number)
> + {
> + foreach (int i in list)
> + if (number == i) return true;
> + return false;
> + }
> +
> + //`list` could be a IEnumerable<int> (or int* ;)
> + public bool GenericInterface (List<int> list, int number)
> + {
> + foreach (int i in list)
> + if (number == i) return true;
> + return false;
> + }
> +
> + //`bar` could be Base
> + public void GenericAddBar (Derived bar)
> + {
> + List<Base> l = new List<Base>();
> + l.Add(bar);
> + }
> +
> + //`bar` could be Base
> + public DerivedDerived Constructor (Derived bar)
> + {
> + return new DerivedDerived (bar);
> + }
> +
> + public Derived derived;
> + public void Stfld (DerivedDerived bar)
> + {
> + derived = bar;
> + }
> +
> + public static Derived sderived;
> + public void Stsfld (DerivedDerived bar)
> + {
> + sderived = bar;
> + }
> +
> + public void GenericMethod<T> (T x) where T : System.ArgumentException
> + {
> + Console.WriteLine (x.Message);
> + }
> + }
> +
> + [TestFixture]
> + public class AvoidUnnecessarySpecializationTest : MethodRuleTestFixture<AvoidUnnecessarySpecializationRule> {
> +
> + [Test]
> + public void FooCouldBeBase ()
> + {
> + AssertRuleFailure<SpecializedClass> ("FooCouldBeBase");
> + AssertRuleSuccess<GeneralizedClass> ("FooCouldBeBase");
> + }
> +
> + [Test]
> + public void DerivedDerivedFooCouldBeBase ()
> + {
> + AssertRuleFailure<SpecializedClass> ("DerivedDerivedFooCouldBeBase");
> + AssertRuleSuccess<GeneralizedClass> ("DerivedDerivedSpecificCouldNotBeGeneralized");
> + }
> +
> + [Test]
> + public void DerivedDerivedBarCouldBeDerived ()
> + {
> + AssertRuleFailure<SpecializedClass> ("DerivedDerivedBarCouldBeDerived");
> + AssertRuleSuccess<GeneralizedClass> ("DerivedBarCouldNotBeGeneralized");
> + }
> +
> + [Test]
> + public void BarAndFooCouldBeGeneralized ()
> + {
> + AssertRuleFailure<SpecializedClass> ("BarAndFooCouldBeGeneralized", 2);
> + AssertRuleFailure<SpecializedClass> ("BarCouldBeGeneralizedButNotFoo", 1);
> + AssertRuleSuccess<GeneralizedClass> ("BarAndFooCouldNotBeGeneralized");
> + }
> +
> + [Test]
> + public void BarUsedAsArgument ()
> + {
> + AssertRuleFailure<SpecializedClass> ("BarAsSpecializedArgument");
> + AssertRuleSuccess<GeneralizedClass> ("BarAsGeneralizedArgument");
> + }
> +
> + [Test]
> + public void Interface ()
> + {
> + AssertRuleFailure<SpecializedClass> ("Interface");
> + AssertRuleSuccess<GeneralizedClass> ("Interface");
> + }
> +
> + [Test]
> + public void GenericInterface ()
> + {
> + AssertRuleFailure<SpecializedClass> ("GenericInterface");
> + AssertRuleSuccess<GeneralizedClass> ("GenericInterface");
> + }
> +
> + [Test]
> + public void GenericAddBar ()
> + {
> + AssertRuleFailure<SpecializedClass> ("GenericAddBar");
> + AssertRuleSuccess<GeneralizedClass> ("GenericAddBar");
> + }
> +
> + [Test]
> + public void Constructor ()
> + {
> + AssertRuleFailure<SpecializedClass> ("Constructor");
> + AssertRuleSuccess<GeneralizedClass> ("Constructor");
> + }
> +
> + [Test]
> + public void Field ()
> + {
> + AssertRuleFailure<SpecializedClass> ("Stfld");
> + AssertRuleSuccess<GeneralizedClass> ("Stfld");
> + AssertRuleSuccess<GeneralizedClass> ("Stfld2");
> + }
> +
> + [Test]
> + public void StaticField ()
> + {
> + AssertRuleFailure<SpecializedClass> ("Stsfld");
> + AssertRuleSuccess<GeneralizedClass> ("Stsfld");
> + AssertRuleSuccess<GeneralizedClass> ("Stsfld2");
> + }
> +
> + [Test]
> + public void GenericMethod ()
> + {
> + AssertRuleSuccess<GeneralizedClass> ("GenericMethod");
> + AssertRuleFailure<SpecializedClass> ("GenericMethod");
> + }
> +
> + [Test]
> + public void DoesNotApply ()
> + {
> + AssertRuleDoesNotApply<GeneralizedClass> ("ParameterLessMethod");
> + AssertRuleDoesNotApply<GeneralizedClass> ("get_Property");
> + AssertRuleDoesNotApply<GeneralizedClass> ("set_Property");
> + }
> +
> + }
> +
> +}
> +
Sebastien Pouliot
To avoid missing the branch deadline I committed the rule, as is, into
SVN.
Most of the rocks we talked about are now available so the update
should be easy.
Sebastien
On Jun 15, 7:54 pm, Sebastien Pouliot <sebastien.poul...@gmail.com>
wrote:
>
> read more ยป