Novell Hack Week and replacing MessageType
Sebastien Pouliot
This week is Novell's second Hack Week. Unless some big issue comes up
I'll be able to work on Gendarme all week long :-)
There's a lot of stuff that we've been talking over the last year (and
some older than a year). I'll try to fix as many as possible this
week.
Now knowing that something is bad or suboptimal is a lot easier than
knowing how to fix it (once and for all). So I'll post some of the
things I'm doing here (since many brains are better than one). Feel
free to comment on them, either by replying to the group or on IRC.
That being said, I never been fond of MessageType which allows
Gendarme to say if a defect is a Warning or an Error. While making
distinctions is good, I think we're mixing two concepts into one. So I
plan (actually I'm testing ;-) to replace it with:
* Severity: How severe is a defect found by the rule (e.g.
UseStringEmptyRule is less severe than BadRecursiveInvocationRule).
Some rules may vary their severity if they check for more than one
thing (we already have rules returning Warning or Error).
and
* Confidence: The level of confidence about the rule results. The more
confidence we have also means it's less likely for the rule will
return false positives.
In both case enums will be used (currently 4 values in each). This
change should also help runners (e.g. gui) to produce better reports
(e.g. sorted or filtered).
Comments welcome!
Sebastien
Néstor Salceda
On lun, 2008-02-11 at 11:55 -0800, Sebastien Pouliot wrote:
> Hello,
>
> This week is Novell's second Hack Week. Unless some big issue comes up
> I'll be able to work on Gendarme all week long :-)
>
> There's a lot of stuff that we've been talking over the last year (and
> some older than a year). I'll try to fix as many as possible this
> week.
Cool, I also have some time this week and I can help you with those
fixings :) I will be all day on IRC tomorrow.
> Now knowing that something is bad or suboptimal is a lot easier than
> knowing how to fix it (once and for all). So I'll post some of the
> things I'm doing here (since many brains are better than one). Feel
> free to comment on them, either by replying to the group or on IRC.
>
> That being said, I never been fond of MessageType which allows
> Gendarme to say if a defect is a Warning or an Error. While making
> distinctions is good, I think we're mixing two concepts into one. So I
> plan (actually I'm testing ;-) to replace it with:
Yes, I believe I have never used those values.
> * Severity: How severe is a defect found by the rule (e.g.
> UseStringEmptyRule is less severe than BadRecursiveInvocationRule).
> Some rules may vary their severity if they check for more than one
> thing (we already have rules returning Warning or Error).
It's a good idea. We could categorize the rules for severity, and if
other people uses Gendarme as code analyzer, they could spend more
effort fixing the more several issues in their code.
> and
>
> * Confidence: The level of confidence about the rule results. The more
> confidence we have also means it's less likely for the rule will
> return false positives.
It's also a nice idea. Measuring the rule's reliability may help us to
identify the rules with more false positives, and we can fix it first.
> In both case enums will be used (currently 4 values in each). This
> change should also help runners (e.g. gui) to produce better reports
> (e.g. sorted or filtered).
Yes, is the simplest solution and will work fine.
> Comments welcome!
Good work Sebastien :)
Néstor.
Sebastien Pouliot
On Feb 11, 6:48 pm, Néstor Salceda <nestor.salc...@gmail.com> wrote:
> Cool, I also have some time this week and I can help you with those
> fixings :) I will be all day on IRC tomorrow.
> > Now knowing that something is bad or suboptimal is a lot easier than
> > knowing how to fix it (once and for all). So I'll post some of the
> > things I'm doing here (since many brains are better than one). Feel
> > free to comment on them, either by replying to the group or on IRC.
>
> > That being said, I never been fond of MessageType which allows
> > Gendarme to say if a defect is a Warning or an Error. While making
> > distinctions is good, I think we're mixing two concepts into one. So I
> > plan (actually I'm testing ;-) to replace it with:
>
> Yes, I believe I have never used those values.
> > * Severity: How severe is a defect found by the rule (e.g.
> > UseStringEmptyRule is less severe than BadRecursiveInvocationRule).
> > Some rules may vary their severity if they check for more than one
> > thing (we already have rules returning Warning or Error).
>
> It's a good idea. We could categorize the rules for severity, and if
> other people uses Gendarme as code analyzer, they could spend more
> effort fixing the more several issues in their code.
already have rules that uses both Warning and Errors in different
cases. So far this seems to me a valid use case, so it would be better
not to fix the values and let the rule report a severity (and
confidence) for each defects it finds.
However the runner can still report the defects by severity or by
sorted by any other fields (I'd like to see LINQ at work for that ;-).
Thanks for the feedback!
Sebastien
Exception
On 11 фев, 22:55, Sebastien Pouliot <sebastien.poul...@gmail.com>
wrote:
> confidence we have also means it's less likely for the rule will
> return false positives.
we already have unit-tests to be sure there can be no false positives?
If any, they should be *fixed*, not just making the rule's Confidence
less. Right?
Happy hacking,
Dan.
Sebastien Pouliot
On Feb 12, 12:00 am, Exception <dan.abra...@gmail.com> wrote:
> Hello!
>
> On 11 фев, 22:55, Sebastien Pouliot <sebastien.poul...@gmail.com>
> wrote:
>
> > * Confidence: The level of confidence about the rule results. The more
> > confidence we have also means it's less likely for the rule will
> > return false positives.
>
> I wonder how do we choose the Confidence for a particular rule. Don't
> we already have unit-tests to be sure there can be no false positives?
very good results. Running gendarme on itself (and then on the class
libs) are far more likely to find false positives. Of course they
should be turned into new (second batch) unit tests.
> If any, they should be *fixed*, not just making the rule's Confidence
> less. Right?
The short one is that "false positives" are bugs from a user point of
view. Some of them are fixable, some are not, but still the rule is
still useful with them.
Now the long answer:
(a) For many rules[1] we can have a "Total" confidence level. Naming
and Design rules are good candidate for this.
(b) For some rules we tolerate[2] less-than perfect results. Mostly
because some cases are a lot harder to get right, yet will not
generate a lot of false positives.
A good example is the
Gendarme.Rules.Portability.ExitCodeIsLimitedOnUnixRule that you wrote.
Your analysis (internally) returns InspectionResult.[Good|Bad|Unsure].
You can get a false positive by running the rule on gendarme.exe
because the try/catch in Main will "confuse" the rule logic. Right now
your rule knowns it's unsure about this results and report this using
a different text (string) message. Using an enum for this makes it
easier for a runner to sort (or exclude) this results. So this kind is
fixable but, for the moment, the time required to do so may be better
invested creating new rules.
(c) Finally in some cases we can't, and won't ever, know for sure. A
rule about exposing immutable objects [3] in static/const fields would
be next to impossible to write without false positives. Yet warning
about them, as potential problems, is worth the false positives.
The conclusion is that, fixable or not, Gendarme should hint the
runner/user about what confidence it has about each of it's result.
[1] generally the simpler rules not involving IL
[2] I said "tolerate" because it's (very possibly) fixable (but often
with a lot more code)
[3] yet to be written
Sebastien