AvoidUnusedPrivateFieldsRule - 2 bugs

[Alan]

The 'problem' text reads:
This method calls several times into some properties. This is
expensive for virtual properties or when the property cannot be
inlined.

That doesn't appear to relate to the rule name at all.

Also, I'm getting a warning that I have an unused private field
(FastPeersFlag) even though i'm using it in two separate methods. I
get a similar warning for the other flags.

http://anonsvn.mono-project.com/source/trunk/bitsharp/src/MonoTorrent/MonoTorrent.Client/Messages/StandardMessages/HandshakeMessage.cs

[Sebastien Pouliot]

On Sun, 2008-07-20 at 08:39 -0700, Alan wrote:
> The 'problem' text reads:
> This method calls several times into some properties. This is
> expensive for virtual properties or when the property cannot be
> inlined.
>
> That doesn't appear to relate to the rule name at all.

Good catch, it's a copy-paste error from another rule (still in
development). I'll fix this asap.

> Also, I'm getting a warning that I have an unused private field
> (FastPeersFlag) even though i'm using it in two separate methods. I
> get a similar warning for the other flags.
>
> http://anonsvn.mono-project.com/source/trunk/bitsharp/src/MonoTorrent/MonoTorrent.Client/Messages/StandardMessages/HandshakeMessage.cs

ah it looks like I missed some cases...

what happens here is that the compilers turns *constant* fields into
values inside IL (let me know if the occurs to non-constant fields!)

E.g. you don't see a "LDFLD FastPeersFlag" but a "LDC_I4_4 +
CONV.I1" (or something like that, I did not compile it ;-)

Anyway this means Gendarme must look at all constants inside IL to see
if one of them match the constant fields. This is not a 100% safe
process (it could be a real 4 value not related to the, possible unused,
field) but it's better than the (current) alternative.

Thanks for the report!
Sebastien

[Sebastien Pouliot]

Both issues are fixed.

Literals (like const) are now ignored[1] since it looking for their
value turned out too many false positives too.

Thanks again
Sebastien

[1] it turns out that fxcop does the same (i.e. ignoring them)

[Calvin Rien]

I'm getting the AvoidUnusedPrivateFieldsRule warning in a coroutine that is definitely using the variable.

Here's the chunk of code causing the warning.

https://gist.github.com/748d0593277be1ca82de

[steves]

It's because AvoidUnusedPrivateFieldsRule does not check accesses to
private field in nested types, which actually have access to private
fields of their parent type. Compiler generates a nested class that
implements the coroutine and all the accesses to the private fields in
your example are moved into the new generated class, which is then not
taken into consideration by AvoidUnusedPrivateFieldsRule.

Fix seems to be easy: just check nested types for private fields
access, i did it here: https://github.com/steve-s/mono-tools/commit/8e81fd26112b4fe792822b42da8a3243360ba4d9
and will send a pull request...

However, if we don't take into account compiler generated nested
types, it could be a sign of a bad design if a private field is used
only in nested type(s), so maybe new rule?

Steves

[David Schmitt]

On 17.03.2012 15:48, steves wrote:
> It's because AvoidUnusedPrivateFieldsRule does not check accesses to
> private field in nested types, which actually have access to private
> fields of their parent type. Compiler generates a nested class that
> implements the coroutine and all the accesses to the private fields in
> your example are moved into the new generated class, which is then not
> taken into consideration by AvoidUnusedPrivateFieldsRule.
>
> Fix seems to be easy: just check nested types for private fields
> access, i did it here: https://github.com/steve-s/mono-tools/commit/8e81fd26112b4fe792822b42da8a3243360ba4d9
> and will send a pull request...
>
> However, if we don't take into account compiler generated nested
> types, it could be a sign of a bad design if a private field is used
> only in nested type(s), so maybe new rule?

That would depend on the nested class, wouldn't it? Enumerators and the
whole async state machine stuff come to my mind, but I'm not firm enough
with the details here :-/

Best Regards, David

[Leszek Ciesielski]

Steves,

could you send the actual pull request? :-) I've just hit the same
false positive - private field used in lambda, so compiler-generated
closure was the only thing referencing it. I've already started
writing a patch for this, but then I found your fix, which looks good.
And I'm willing to bet that compiler generated classes are much more
likely to trigger this false positive than some wonky bad code.

Regards,

Leszek 'skolima' Ciesielski

[steves]

Hi there,

On Apr 29, 9:29 am, Leszek Ciesielski <skol...@gmail.com> wrote:
> could you send the actual pull request? :-)

it's here: https://github.com/mono/mono-tools/pull/26