Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two Gendarme rules about Main #61

Closed
GoogleCodeExporter opened this issue Jun 3, 2015 · 24 comments
Closed

Two Gendarme rules about Main #61

GoogleCodeExporter opened this issue Jun 3, 2015 · 24 comments

Comments

@GoogleCodeExporter
Copy link

This task is to create 2 specific rules for Gendarme [1] about the usage of
the most common method, Main ;-)

1. Gendarme.Rules.Design.MainShouldNotBePublicRule

An easy design rule where you ensure that the real (EntryPoint) Main is not
public. Here testing will need to be a bit more creative than usual since
we cannot define several entry points in our unit test assemblies.

2. Gendarme.Rules.Portability.MainReturnValueIsLimitedUnderUnixRule

This rule should warn if the Main entry point returns a int (to ensure
developers are aware of the difference between Windows (32bits) and Unix
(8bits)). However the rule should also scan the IL of the Main method to
see if it returns a value bigger than 255. In that case it should report
error(s) (instead of a warning).


To complete the task both rules must include:
- the 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 2-5 days (depending if you already know
Gendarme/Cecil or not).

[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 26 Dec 2007 at 5:54

@GoogleCodeExporter
Copy link
Author

I claim this task.
Can you please tell more about difference between Windows and Unix way, or 
provide
some linkies to docs?
Thanks, Dan.

Original comment by dan.abra...@gmail.com on 26 Dec 2007 at 9:42

@GoogleCodeExporter
Copy link
Author

Look at this discussion 
http://www.opensubscriber.com/message/mono-list@lists.ximian.com/8207136.html

This rule could be extended (and probably renamed) to deal with other cases
* Environment.ExitCode property
* Environment.Exit method
* Process.ExitCode property
which would result in the same potential problem.

Original comment by sebastie...@gmail.com on 26 Dec 2007 at 10:12

  • Changed state: Claimed
  • Added labels: ClaimedBy-dan.abramov

@GoogleCodeExporter
Copy link
Author

First one completed :-) .

Docs:

MainShouldNotBePublicRule 

This rule is used to warn developer if assembly's entry point (so-called Main 
method)
is exposed to other assemblies as a public method and can be accessed by them. 
It is
recommended to make it private to make sure that it can be called only by CLR 
itself
while executing the code.

Bad example:  

public class MainClass {

    public void Main ()
    {
    }
}

Good example:

internal class MainClass {

    public void Main ()
    {
    }
}

Another good example:

public class MainClass {

    internal void Main ()
    {
    }
}

Original comment by nastya.a...@gmail.com on 27 Dec 2007 at 8:05

Attachments:

@GoogleCodeExporter
Copy link
Author

Using Cecil to generate the assemblies is a good idea (never used Cecil for such
myself ;-). However it doesn't seems to work for me. All three tests throws a
NullReferenceException on this line (MethodRock.cs) since the second ReturnType 
is null.

    switch (self.ReturnType.ReturnType.Name) {

Don't you get this error ?  I tested the code on both Linux and Windows (same 
Cecil
from SVN was used). BTW it's nice to see you use the new extension methods 
(rocks :)

Other (minor) comments:

* try to write rules in two stages (this will be required in the next version of
gendarme to gather rule statistics). First stage is to determine if the rule 
applies
(or not, in that case return runner.RuleSuccess) then execute rule logic. E.g. 
in
this case the rule applies if there is an EntryPoint defined on the Assembly.

* add a test case for an assembly without an entry point (you can use the unit 
test
assembly for this). Again the *win* of this test case will be more apparent in 
the
future ;-)

* it seems that VB.NET cannot declare Main as private (I need to confirm this 
as I've
never used VB.NET myself). In this case we might need an extra check to see if 
the
assembly reference a VB.NET assembly (I'll ask someone about this).

Thanks!

p.s. it's a good idea to split the review for each rule!

Original comment by sebastie...@gmail.com on 27 Dec 2007 at 9:06

@GoogleCodeExporter
Copy link
Author

> Using Cecil to generate the assemblies is a good idea (never used Cecil for 
such
myself ;-)
Neither did I :-) . It just seemed simplier than having to mantain additional 
files etc.

> All three tests throws a
NullReferenceException on this line (MethodRock.cs) since the second ReturnType 
is null.
That's strange. Try removing .IsMain () call then. I haven't yet tested the 
code with
this call since Rocks does not want to compile on my machine (perhaps old Mono
version?), and I thought it would just work this way. Without .IsMain () 
everything
was OK.

> BTW it's nice to see you use the new extension methods
They really rock (I just didn't know Mono already can do that)! However, as you 
see,
as of now I can't take full advantage of using them. If you're wondering how 
does my
code compile at all, since Rocks is disabled - now I am smarter :) now I 
compile just
what I need (e.g. one rule and test case for it), not all files.

> try to write rules in two stages
Yes, I have noticed that pattern in other rules. Will follow.

> add a test case for an assembly without an entry point
OK.

> it seems that VB.NET cannot declare Main as private 
Just tested it myself, you're right. Checking if an assembly references
Microsoft.VisualBasic.dll is a good point (it can't be turned off in VB .NET 
project
settings, dunno about compiler options though) but what if a C# developer 
feeling
nostalgic about good VB times adds a ref to that assembly? Of course, this is 
mostly
not likely to happen, we should be more confident here, shouldn't we? I wonder 
if
there's any other ways to "catch" a vbc-compiled assembly.

p.s. Yep, I knew you'd like it ;-)

Original comment by nastya.a...@gmail.com on 27 Dec 2007 at 11:43

@GoogleCodeExporter
Copy link
Author

Shit, that was my mom's account o_O

Original comment by dan.abra...@gmail.com on 27 Dec 2007 at 11:47

@GoogleCodeExporter
Copy link
Author

Ok, IsMain isn't really required since would likely be a compiler doing "bad
stuff"(tm) - and we can consider this to be outside Gendarme's goal (it aims to 
catch
developers, not compilers, doing bad stuff ;-)

As for VB, I'm waiting for an answer, but I guess looking for an referenced 
assembly
(and documenting that the rule targets C#) would be enough.

Original comment by sebastie...@gmail.com on 28 Dec 2007 at 12:24

@GoogleCodeExporter
Copy link
Author

For VB assemblies...

<jpobst> most VB programs will link to Microsoft.VisualBasic.dll
<jpobst> they don't have to, but most VB programmers won't know how to *NOT* 
link to it
<jpobst> since VS does it behind your back

and for the second rule...

maybe it would be better to rename it ExitCodeValueIsLimitedUnderUnixRule since 
it's
covers a bit outside Main.

Original comment by sebastie...@gmail.com on 28 Dec 2007 at 12:34

@GoogleCodeExporter
Copy link
Author

After a bit of investigating, I found out that by default Main () in VB is 
Public.
However (!) class that contains it is _not_ public by default. That means that 
we
_still_ must warn user if he wants to make it public (though we should better 
alarm
not at the Main () method itself but at so-called vb-ish Module, i.e. static 
class).
I restructured the code a little bit and added test case for non-executable 
assemblies.

I am currently unable to test this, but it must work as intended with VB 
assemblies too.

Original comment by dan.abra...@gmail.com on 28 Dec 2007 at 1:35

Attachments:

@GoogleCodeExporter
Copy link
Author

Placing an arbitrary due date for house keeping purposes, the task owner and 
claimee
can negotiate and change it if needed.

Original comment by jpo...@gmail.com on 28 Dec 2007 at 2:37

  • Added labels: DueDate-2008-01-04

@GoogleCodeExporter
Copy link
Author

Ok, I tested that

Public Module Module1
    Sub Main()
    End Sub
End Module

is reported and that

Module Module1
    Sub Main()
    End Sub
End Module

isn't. So the first rule is ok :) and I'll commit it soon to SVN (*)

Please update your rule description (comment #3) to include a note about vb 
case.

(*) I'll move up your "rule applies" comment. The rule actually applies to every
assembly with an EntryPoint, all other stuff is checking the rule itself.

Original comment by sebastie...@gmail.com on 28 Dec 2007 at 4:38

@GoogleCodeExporter
Copy link
Author

Docs rev. 2:

MainShouldNotBePublicRule 

This rule is used to warn developer if assembly's entry point (so-called Main 
method)
is exposed to other assemblies as a public method and can be accessed by them. 
It is
recommended to make it private to make sure that it can be called only by CLR 
itself
while executing the code. Since in VB .NET Main () method cannot be made other 
than
public, for assemblies generated by VB compiler it is being checked whether the
module or class that contains entry point is public, and thus accessible to 
other
assemblies. 

Bad example:  

public class MainClass {

    public void Main ()
    {
    }
}

Public Module MainModule
    Public Sub Main ()
    End Sub
End Module

Good example:

internal class MainClass {

    public void Main ()
    {
    }
}

Module MainModule
    Public Sub Main ()
    End Main
End Module

Another good example:

public class MainClass {

    internal void Main ()
    {
    }
}

Original comment by dan.abra...@gmail.com on 28 Dec 2007 at 5:15

@GoogleCodeExporter
Copy link
Author

Ok, first part in SVN r91994/5.
Thanks :)

Original comment by sebastie...@gmail.com on 28 Dec 2007 at 5:47

@GoogleCodeExporter
Copy link
Author

By the way, I faced the same issue
(NullReferenceException on this line (MethodRock.cs) since the second 
ReturnType is null.
switch (self.ReturnType.ReturnType.Name))
while implementing some other stuff, and from now I seem to understand what 
happens.
If method return type is void, it's not method.ReturnType.ReturnType.Name is
"System.Void", it's just method.ReturnType.ReturnType is null so there's no 
wonder it
fails with void Main ().

Original comment by dan.abra...@gmail.com on 28 Dec 2007 at 7:06

@GoogleCodeExporter
Copy link
Author

Implemented first revision of MainReturnValueIsLimitedUnderUnixRule.
It handles cases when return value is specified right in Main () code - like 
this:

public static int Main () 
{
    if (some-condition) return 10;
    else if (another-condition) return -1; // forbidden
    // ...
    return 5;
}

Still it does not handle things like method calls (how should this be 
implemented?).

P.S. Hope you'll like the way I load/generate assemblies for testing - I like 
it a lot =)
P.P.S. ModuleDefinition.Inject () is awesome.

Original comment by dan.abra...@gmail.com on 28 Dec 2007 at 9:19

Attachments:

@GoogleCodeExporter
Copy link
Author

> Still it does not handle things like method calls (how should this be 
implemented?).

Just check what's the return type of the method call. If it's different than 
byte
(int8 in IL) then you can't be sure (warn) otherwise it may be ok (and you 
cannot
easily know if the return value is bigger than 255).

What about my suggestion to rename this rule to 
ExitCodeValueIsLimitedUnderUnixRule
and add checks for 
* Environment.ExitCode property
* Environment.Exit method
* Process.ExitCode property
(see comment #2)

Looking at the code now... ;-)

Original comment by sebastie...@gmail.com on 28 Dec 2007 at 9:30

@GoogleCodeExporter
Copy link
Author

Done. Much code (particularly test cases) but I tried reducing it as much as 
possible.
Hope to hear from you soon!

Original comment by dan.abra...@gmail.com on 29 Dec 2007 at 1:28

Attachments:

@GoogleCodeExporter
Copy link
Author

Docs rev. 3:

ExitCodeIsLimitedOnUnixRule

This rule applies to all executable assemblies. A thing that many Windows 
developers
might not be aware of is that in Unix systems, process exit code must be 
between zero
and 255, unlike in Windows where it can be any valid integer value. This rule 
warns
user if he tries returning something that might be out of range either by 
setting
Main () method return value or by setting Environment.ExitCode property or 
calling
Environment.Exit (exitCode) method. Error is reported in case number which is
definitely out of range is being returned as an exit code.

Bad example:  

class MainClass {

    private static int Main ()
    {
        return -1;
        -or-
        return 256; 
        -or
        Environment.ExitCode = 1000;
        -or-
        Environment.Exit (512);
    }
}

Good example:

class MainClass {

    private static int Main ()
    {
        return 1;
        -or-
        return 255; 
        -or
        Environment.ExitCode = 42;
        -or-
        Environment.Exit (100);        
    }
}

Another good example (do not return anything):

class MainClass {

    private static void Main ()
    {
    }
}



P.S. In docs to first rule, replace "void Main"s with "static void Main"s! I 
forgot
about that.

Original comment by dan.abra...@gmail.com on 29 Dec 2007 at 1:45

@GoogleCodeExporter
Copy link
Author

here's some quick notes (I'll do more testing tomorrow)

(1)
    case Code.Call:

this isn't enough, since it won't catch virtual calls (Callvirt).
Also a Calli (uncommon since unverifiable) should be interpreted as "unsure".

(2) I'll add the Location (MethodDefinition method, int offset) constructor :)

Original comment by sebastie...@gmail.com on 29 Dec 2007 at 3:07

@GoogleCodeExporter
Copy link
Author

I wonder how can I _virtually_ call Enviroment method (). All unit test go 
without a
problem. However you may change it if you're unsure.

Original comment by dan.abra...@gmail.com on 29 Dec 2007 at 10:24

@GoogleCodeExporter
Copy link
Author

Major:
* CheckIntMainBody only track the first problem, so it can report a warning 
while
there's clearly an error in there. MessageCollection exists so it's possible to
report many errors (and exact locations) from a rule (just like CheckMethod 
does ;-)

Minor:
* compilers may use callvirt for non-virtual calls (the other way isn't valid);
* there's a Console.WriteLine in your rule, it should be removed;

Information:
Some test fails when compiled with CSC in debug mode. That's because the IL it
generates is pretty messed up. It's not a new problem for Gendarme and it 
doesn't
affect you completing the task (i.e. FYI ;-).

Original comment by sebastie...@gmail.com on 29 Dec 2007 at 3:33

@GoogleCodeExporter
Copy link
Author

Update.

Original comment by dan.abra...@gmail.com on 29 Dec 2007 at 4:09

Attachments:

@GoogleCodeExporter
Copy link
Author

Looks good! I'll integrate this into Gendarme build later (and ping you if 
there are
problems, but I don't expect any). You can go ahead and claim another task in 
the
meantime (I'll close this once the code is committed).

Thanks!

Original comment by sebastie...@gmail.com on 29 Dec 2007 at 4:14

@GoogleCodeExporter
Copy link
Author

Second rule tested and now in SVN r92027/8. Closing task!

note: I have added the new Location constructor (r92026) and modified the rule 
for it.

Thanks!
Sebastien

Original comment by sebastie...@gmail.com on 30 Dec 2007 at 3:42

  • Changed state: Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant