Closed GoogleCodeExporter closed 9 years ago
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
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
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:
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
> 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
Shit, that was my mom's account o_O
Original comment by dan.abra...@gmail.com
on 27 Dec 2007 at 11:47
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
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
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:
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
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
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
Ok, first part in SVN r91994/5.
Thanks :)
Original comment by sebastie...@gmail.com
on 28 Dec 2007 at 5:47
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
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:
> 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
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:
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
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
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
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
Update.
Original comment by dan.abra...@gmail.com
on 29 Dec 2007 at 4:09
Attachments:
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
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
Original issue reported on code.google.com by
sebastie...@gmail.com
on 26 Dec 2007 at 5:54