Two gendarme rules about creating object without really using them

[GoogleCodeExporter]

This task is to create two (2) very similar rules for Gendarme [1] to check
for objects that are created but not used - and could be typos that affect
functionality.

1) Gendarme.Rules.BadPractice.CheckNewExceptionWithoutThrowingRule

Here the rule must check that an exception is being created. If so it must
check that:
* the exception is thrown in the method; or
* the exception is passed to a method as a parameter; or
* the exception is returned by the method (ref, out or return value)

2) Gendarme.Rules.BadPractice.CheckNewThreadWithoutStartRule

Here the rule must check if a Thread is being created. If so it must heck that:
* Start is being called on the thread;
* the thread object is passed to a method as a parameter; or
* the thread object is returned by the method (ref, out or return value)

Only two rules but don't let this fool you they are not simple! Consider
this task if you want a real challenge to finish GHOP and have some
experience with Cecil (or Gendarme).

To complete the task it must include:
- the rules source code;
- unit tests to ensure the rules works correctly;
- documentation for the web site (see [2] for an example)

This tasks should take between 4-5 days.

[1] http://www.mono-project.com/Gendarme
[2] http://groups.google.com/group/gendarme

Original issue reported on code.google.com by sebastie...@gmail.com on 21 Jan 2008 at 1:46

[GoogleCodeExporter]

I claim this task.

Original comment by andreas....@gmail.com on 21 Jan 2008 at 10:11

[GoogleCodeExporter]

Great :) The biggest challenge is to keep false-positive under control ;-)

Original comment by sebastie...@gmail.com on 21 Jan 2008 at 10:15

• Changed state: Claimed
• Added labels: ClaimedBy-andreas.noever, DueDate-2008-02-03

[GoogleCodeExporter]

Small status-update:

I have written a StackEntryAnalysis class that allows me to track usage of a
reference on the execution stack. With this class it's possible to find all
instructions that use the reference in some way (return it, use it as a 
parameter or
call a method on it). It supports branches, assignments (a = new object(); b = 
a;
return b; "return b" will be reported), try/catch and almost (small bug) 
try/finally
(see the test cases for some more examples :)).

The CheckNewExceptionWithoutThrowingRule uses this class and works really well 
with
it. Just one false positive in mscorlib (in the remoting-part, it reuses a 
parameter
as a variable, have to look into this tomorrow).

Note: This is still not ready. StackEntryAnalysis needs some cleanup and
restructuring. So no need to review it yet, it's meant as a preview ;)

Andreas Noever

Original comment by andreas....@gmail.com on 22 Jan 2008 at 10:36

Attachments:

• StackEntryAnalysis.cs
• StackEntryAnalysisTest.cs
• Rocks.cs
• CheckNewExceptionWithoutThrowingRule.cs
• CheckNewExceptionWithoutThrowingTest.cs

[GoogleCodeExporter]

Cool :)

> public static bool IsCall (this Instruction self)
Check FlowControl property, this should do the job (and we should be using it 
more
often ;-)... but I see you're already using the property somewhere else!

You may also want to look (or chat with jb) about Cecil.FlowAnalysis assembly 
but,
right now, Gendarme doesn't depend on it.

Original comment by sebastie...@gmail.com on 23 Jan 2008 at 1:53

[GoogleCodeExporter]

Hi,

StackEntryAnalysis should work correctly except for some special cases related 
to
out/ref parameters and the try/catch behavior is not 100% correct (it branches 
for
each exception handler even if no exception/the wrong exception is thrown). All 
test
cases pass and mono mscorlib.dll runs without errors.

Currently StackEntryAnalysis still outputs some debugging information to the 
console.

Andreas Noever

Original comment by andreas....@gmail.com on 23 Jan 2008 at 11:44

Attachments:

• CheckNewExceptionWithoutThrowingRule.cs
• CheckNewExceptionWithoutThrowingTest.cs
• Rocks.cs
• StackEntryAnalysis.cs
• StackEntryAnalysisTest.cs

[GoogleCodeExporter]

New version: With both rules, more documentation and more test cases.

StackEntryAnalysis supports:
* branches
* following assignments (local variables, arguments, fields and out/ref 
arguments)
For fields and out/ref arguments the results are not correct in all cases (some
additional usages might get reported). This is because the class does not know
exactly which value is on the stack before the stfld / stind instruction.
* try/catch/finally
All catch blocks are executed, regardless of which exception they are supposed 
to handle.

Andreas Noever

Original comment by andreas....@gmail.com on 24 Jan 2008 at 1:03

Attachments:

• 82.zip

[GoogleCodeExporter]

Review (part 1)
Overall: Very impressive (and that gets me thinking about other stuff it could 
do :)

CheckNewExceptionWithoutThrowingRule
* typo: assembly loader is missing -> assembly resolver, we can load them ;-)
* very nice to check "ins.Next.OpCode.Code == Code.Throw" before starting the 
analysis!

StackAnalysis
* remove (or define out) C.WL
* you shouldn't use Exception (maybe we should provide one, and yes we need a 
rule to
check people throwing Exception and others like that ;-) but, more important, 
add a
different description in each of them to ease tracking.
* code coverage by tests doesn't seem to cover everything (e.g. switch), it be 
nice
to have some test reaching this code :)
* typo: same loader/resolver in unit tests (Ignore)

Rocks.cs
This is very minor but I'm not very fond of the rocks.
* I "avoided" IsVoid before because it open the door to a gazillion other 
Is[Type]
which will hide the real "gems" in the rocks ;-) However maybe it makes sense 
that
some rules (or assembly) have their own LocalRocks...
* Instruction.IsCall is not correct, in fact we'll have to fix every (well 
most) rule
someday since calli (unverifiable and uncommon at least for compilers) will 
never
have a Method[Reference|Definition] as operand.
* AddIfNew is ok, not sure if we should have it in the framework or inside the 
rule
assembly (since it's used only by one class).

I'll continue reviewing tomorrow night (well this weekend) and try this on the 
class
libraries (been reading more than testing tonight ;-)

Original comment by sebastie...@gmail.com on 25 Jan 2008 at 2:42

[GoogleCodeExporter]

* changed loader -> resolver
* removed the C.WLs (the biggest part was figuring out was "C.WL" means :) )
* changed all Exceptions to NotImplementedExceptions; Added descriptions.
* Code coverage is at 95% now. Missing: Two safeguards for code that does not 
clean
the stack before a leave / throw instruction (c# always pops all elements of the
stack, but this is not required for correct IL code) + Exceptions + a few 
ldarg.2/3
cases...
* IsVoid has been removed (was only used in two places)
* IsCall is also gone. I'm using IMethodSignature where necessary or removed 
"case
Calli".

Original comment by andreas....@gmail.com on 25 Jan 2008 at 8:04

Attachments:

• 82_2.zip

[GoogleCodeExporter]

Impressive :) Performance is even better than I anticipated (or feared ;-). 
Even then
it might make sense (it's not in the task but you could add a comment in the 
source)
to cache the StackEntryAnalysis so multiple rules could reuse an existing one 
(at
least it's worth a try).

Only one thing should be fixed. Running Gendarme on your code turns out quite a 
few
(well 10) defects, including some by rules you wrote ;-) Yes, Gendarme is a very
humbling tool :) Results attached (forget about #1, it's unrelated to the task)

Once this is fixed (or reduced) only documentation is missing to close the task!
Great work!

Original comment by sebastie...@gmail.com on 29 Jan 2008 at 3:53

Attachments:

• bad.html

[GoogleCodeExporter]

#2 GetLoadSlot / GetStoreSlot contain duplicate code
Some lines are duplicates, but there is nothing that would make sense to 
extract into
another method.

#3 (GetStackEntryUsage) and #4 (FindLoad) are too long
GetStackEntryUsage has been split up. FindLoad is hard to split up because most 
parts
of the method need access to the "MethodState".

#5 GetNextInstruction can be made static.
Thats true. (I have already made it static for the #63 update
(GetLastErrorMustBeCalledRightAfterPInvokeRule))

#6 GetPushCount can be made static
For symmetry reasons I would like to keep it non static (GetPopCount needs the
returnvalue of the method).  But it could of course be made static.

#7 StoreType is not instantiated
This is a false positive. StoreType is an enum.

#8 StoreSlot contains public fields.
They are readonly now.

#9 OverrideEquals
I have now implemented Equals for StoreSlot. But it's a private nested class so 
it is
not really necessary.

#10 Provide an alternative method for !=
StoreSlot is a private struct. It will never be called from non C# code.

#11 IsNone() looks like a property
Fixed!

#12 CheckNewThreadWithoutStartRule::CheckMethod is too long
I extracted part of the method into CheckUsage(). This also improves 
readability of
the switch statement (no more threadUsed = true).

StackEntryAnalysis is very small (it only contains a MethodDefinition). So 
caching it
would probably be more expensive than instantiating it.

Andreas Noever

Original comment by andreas....@gmail.com on 29 Jan 2008 at 11:37

Attachments:

• CheckNewThreadWithoutStartRule.cs
• StackEntryAnalysis.cs

[GoogleCodeExporter]

Hey, I'll complete the review today (and I *must* since after this I need to 
move
back into my office before tomorrow ;-). 

Can you supply the rule documentation ? (since it's the last thing missing).
Thanks!

Original comment by sebastie...@gmail.com on 30 Jan 2008 at 2:17

[GoogleCodeExporter]

CheckNewExceptionWithoutThrowingRule

This rule checks for exception objects that are created but not thrown, 
returned or
passed to another method as an argument.

Bad Example:

void MissingThrow (object arg)
{
    if (arg == null)
        new ArgumentNullException ("arg");
    DoWork (arg);
}

Good Example

void Throw (object arg)
{
    if (arg == null)
        throw new ArgumentNullException ("arg");
    DoWork (arg);
}

Exception CreateException ()
{
    return new Exception ();
}

CheckNewThreadWithoutStartRule

This rule checks for threads that are created but not started, returned or 
passed to
another method as an argument.

Bad Example:

void UnusedThread ()
{
    Thread thread = new Thread (threadStart);
    thread.Name = "Thread 1";
}

Good Example:

void Start()
{
    Thread thread = new Thread (threadStart);
    thread.Name = "Thread 1";
    thread.Start ();
}

Thread InitializeThread ()
{
    Thread thread = new Thread (threadStart);
    thread.Name = "Thread 1";
    return thread;
}

Original comment by andreas....@gmail.com on 30 Jan 2008 at 3:29

[GoogleCodeExporter]

And so this last task gets to it's end. Very impressive work - much better than 
I
expected wrt to false-positives.

I moved StackEntryAnalysis and Rocks to Gendarme.Framework so it can be re-used 
for
other rules (like #63 which resides in interoperability). Doing a bit more 
testing
then I'll commit all this to SVN (probably tonight).

Thanks a lot!
Sebastien

p.s. I hope we will see you soon again on #gendarme and/or on our Google group 
:)

Original comment by sebastie...@gmail.com on 30 Jan 2008 at 4:46

• Changed state: Closed