[Mono-devel-list] Compiler warnings: what to report (Re: System.XML warning)

[Atsushi Eno]

Hello,


Marek Safar wrote:
> Hello,
> 
> Comments in lined.
> 
>>>
>>> System.Xml/XmlReader.cs(1051) warning CS0169: The private method 
>>> 'System.Xml.XmlReader.CheckSupport()' is never used
>>
>> This is used in NET_2_0 part of code.
>> Should we enclose ALL of such members in _nasty_ preprocessor
>> directives? Does it ease developers read it?
> 
> Yes, it is easier to read and easier to maintain. If you don't enclose 
> your members in #if NET_2_0 directive how can I know
> that a field is not valid for NET_1_x ?. I have to search in your code 
> and try to find out whether the field is valid or not.

IMO having ifdefs are much more nasty than just having unused fields.
If there are ifdefs in effect, in every block (not only field 
declarations) you must grep up to find whether it is valid block
or not.

If those warning output REALLY annoys developers, I'd just add
/nowarn:169 as well as 162, 612 and 618.

>>> System.Xml/XmlTextReader.cs(1672) warning CS0168: The variable 
>>> 'dummyValue' is declared but never used
>>
>>
>>
>> It is _required_ even though it is actually not used (otherwise mcs/csc
>> will reject this code). CSC never reports it as warning and I think
>> that regarding it as a warning is bad design.
>>
> Is it some managed <-> unmanaged work ?

If you tried to remove that assignment statement for the dummy value,
csc will complain that:

     System.Xml\XmlTextReader.cs(1674,23): error CS0201: Only
     assignment, call, increment, decrement, and new object
     expressions can be used as a statement

and mcs saying that:

     syntax error, got token `SEMICOLON'
     System.Xml\XmlTextReader.cs(1674) error CS1002: Expecting `;'

Well, aside this code, there is possibility that those extra fields
might be used from other assemblies. For example, I used to have
XQueryCommandImpl type and field in System.Xml.dll and referenced it
from System.Data.SqlXml.dll via reflection.

>>> System.Xml.Xsl/XslTransform.cs(604) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xsltCleanupGlobals()' is 
>>> never used
>>> System.Xml.Xsl/XslTransform.cs(611) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlNewDoc( string)' is 
>>> never used
>>> System.Xml.Xsl/XslTransform.cs(614) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlSaveFile( string,  
>>> System.IntPtr)' is never used
>>> System.Xml.Xsl/XslTransform.cs(626) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlCleanupParser()' is 
>>> never used
>>> System.Xml.Xsl/XslTransform.cs(629) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlDocDumpMemory( 
>>> System.IntPtr, ref System.IntPtr&, ref int&)' is never used
>>> System.Xml.Xsl/XslTransform.cs(632) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlFree( System.IntPtr)' 
>>> is never used
>>> System.Xml.Xsl/XslTransform.cs(649) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.valuePop( 
>>> System.IntPtr)' is never used
>>> System.Xml.Xsl/XslTransform.cs(652) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.valuePush( 
>>> System.IntPtr,  System.Xml.Xsl.UnmanagedXslTransform.xpathobject*)' 
>>> is never used
>>> System.Xml.Xsl/XslTransform.cs(655) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathFreeObject( 
>>> System.Xml.Xsl.UnmanagedXslTransform.xpathobject*)' is never used
>>> System.Xml.Xsl/XslTransform.cs(658) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathNewCString( 
>>> string)' is never used
>>> System.Xml.Xsl/XslTransform.cs(661) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathNewFloat( 
>>> double)' is never used
>>> System.Xml.Xsl/XslTransform.cs(664) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathNewBoolean( 
>>> int)' is never used
>>> System.Xml.Xsl/XslTransform.cs(667) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathNewNodeSet( 
>>> System.IntPtr)' is never used
>>> System.Xml.Xsl/XslTransform.cs(670) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathCastToBoolean( 
>>> System.Xml.Xsl.UnmanagedXslTransform.xpathobject*)' is never used
>>> System.Xml.Xsl/XslTransform.cs(673) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathCastToNumber( 
>>> System.Xml.Xsl.UnmanagedXslTransform.xpathobject*)' is never used
>>> System.Xml.Xsl/XslTransform.cs(676) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathCastToString( 
>>> System.Xml.Xsl.UnmanagedXslTransform.xpathobject*)' is never used
>>> System.Xml.Xsl/XslTransform.cs(679) warning CS0169: The private 
>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathStringFunction( 
>>> System.IntPtr,  int)' is never used
>>
>> Why are they reported as never used? They are actually in use.
> 
> Are you sure that they are used ?
> 
> I tried to remove 
> 'System.Xml.Xsl.UnmanagedXslTransform.xsltCleanupGlobals' and no 
> compilation error.

Mmm, sorry then I should have checked more in details. At least

 >>> System.Xml.Xsl/XslTransform.cs(649) warning CS0169: The private
 >>> method 'System.Xml.Xsl.UnmanagedXslTransform.valuePop(
 >>> System.IntPtr)' is never used

this is really in use.

I'll check for each unmanaged methods and comment out if they are
unused.

>>> System.Xml.Schema/XmlSchemaSet.cs(134) warning CS3019: CLS compliance 
>>> checking will not be performed on 
>>> 'System.Xml.Schema.XmlSchemaSet.XmlResolver' because it is private or 
>>> internal
>>
>> This is reported just because this type is public only under NET_2_0.
>> When there are such types, should we put NET_2_0 on every such
>> attributes?
> 
> You can keep it. In this case Microsoft made custom property modifiers 
> CLS-Compliant (similar case as generics). I am going to remove this 
> warning soon.

Ok, cool.

>> BTW how can we verify that those sources became clean after
>> manual warning elimination?
> 
> I will send you mcs patch.

Please don't send me but post it to the mail list so that everyone
can have a chance to try.

>> Also, I think that some part of those warning report feature are
>> problematic. For example,
>>
>>     - I think there is no (or little) need to report not-in-use
>>       private methods. When we found such methods that we don't
>>       know what it is, we usually grep (usually) single file
>>       and notice that it is not in use. I believe that why MS
>>       csc has such reporting feature for unused fields is that
>>       they might be confused with local variables or parameters.
>>       I don't think mcs should not let developers to be puritan
>>       that believes ALL unused members MUST be eliminated.
> 
>  From my point of view, there are several reasons why report "unused" code.
> - I like every feature which tells me that something is "wrong" with my 
> code.
> - Your code can be easily faster/smaller. When you remove class member 
> you allocate less.
> When you remove method sometimes it leads to next methods/fields clean up.
> 
> Maybe I am wrong but why should I keep unused code ?

 From my point of view, there are many reasons not to report "unused" code.
- I like every feature which tells me that something is "wrong" with my
   code, only when it is REALLY wrong.
- It is too trivial contribution to performance to remove unused code,
   unless there are heavy reflection work.
- Those puritan warnings really discourages us at every point of
   incomplete development where we wonder if our refactory is fine.
   For example, suppose we were developing UnmanagedXslTransform and
   going to support all exposed functionality in unmanaged library,
   Checking every import is just a pain.

>>
>>     - As I noted above, there are unexpected unused field check
>>       that at least csc does not regard as should-be-warned.
>>
> If you know about any wrong warnings please let me know.

As for that unused field, I think I explaind understandably, though
I don't know that is the merginal line between those code that is
worthy of being warned and those code that is not.

> But csc is really not good in this area. They made some improvement for 
> 2.0. But even
> simple cases are still not detected.

Yes, csc 2.0 seems added some additional code analysis. But I doubt
if the remaining check is "better". It sounds like design principle
difference.

Atsushi Eno