Wikimedia Bugzilla is closed!

Created attachment 987 [details]
List of unused variables, unused globals, etc. in MediaWiki 1.5.0

How are you judging these as unused? Globals are used as they're needed, for
configuration settings and so forth.

Globals are imported into a function's local namespace with the 'global' statement. 
Sometimes code using a global is removed, but the 'global' import gets left in. It 
doesn't harm anything, but removing the unused ones helps make the code more readable 
and maintainable, since you can better see what's used where.

Nick, is this list produced from an automated tool? If so, is that tool available? It 
could be handy to have around.

> How are you judging these as unused? Globals are used as they're needed, for
> configuration settings and so forth.

They're unused in the sense that there is no reason for them to be there.

For example "global $thing" is unused in the following code:
----------------------------------
function x() {
  global $thing;
  print "hello world!\n";
}
----------------------------------

With the "global $thing" there it is still valid PHP code. It would however be
improved by removing that line, without any loss of functionality.

However, before you come to the conclusion this list is totally cosmetic, let me
assure you that it is not. For example, two of the errors it found in
includes/Title.php were this:
----------------------------------
Variable $wgUser appears only once (line 1662)
The value of variable $wgUser was never used (line 1662)
----------------------------------

And the line of code was this:
----------------------------------
wfRunHooks( 'TitleMoveComplete', array( &$this, &$nt, &$wgUser, $pageid,
$redirid ) );
----------------------------------

Because $wgUser has not been defined locally, and the $wgUser global is not in
scope in the above code, this results in a reference to NULL being passed to
wfRunHooks, rather than the $wgUser global. There are two problems with this:
1) It is part of the reason that page moves no longer work in PHP 4.1.2, because
earlier versions of PHP don't handle NULL as well as later versions in some
situations.
2) It's not actually doing what you want it to do, irrespective of which version
of PHP you are running. It is a bug, and it is a bug that is extremely hard to
spot with the human eye, because it is somewhere in the middle of a large file,
and because it looks just like every other call to wfRunHooks - and yet it is wrong.

The solution is simple, and is to change the code to this:
----------------------------------
global $wgUser;
wfRunHooks( 'TitleMoveComplete', array( &$this, &$nt, &$wgUser, $pageid,
$redirid ) );
----------------------------------

So, in other words, most of the things are unneeded code, but some of the things
it finds are bugs. Therefore it is a very good idea to fix all of the things on
this list, unless you have good reason to believe something is a false positive
(which can be the sometimes be the case with some of the reference-related
errors, but they are generally all worth checking).

Note that the fix for the wfRunHooks problem listed above (plus other cleanups)
has already been included as an attachment to bug 3667.

> Nick, is this list produced from an automated tool? If so, is that tool 
> available?

Yes to the automated tool, but unfortunately it is commercial / non-free.
Specifically it was Zend Studio 5.0 Beta 2 (which can be downloaded here:
http://www.zend.com/store/products/zend-studio/beta.php ). Note though that the
beta 2 executable will expire on 21-Nov-2005.

It contains a thing called the "Code Analyzer" which is form a 'static code
analysis' tool. (Ref: http://en.wikipedia.org/wiki/Static_code_analysis )

Other examples of static code analysis are what the Eclipse IDE automatically
does for Java code - finds unneeded imports, dead/unreachable code, unused
variables, etc.

In the past I've looked extensively for free / GPL static code analysis tools
that work with PHP, but I have not found any yet. There is research in this area
(e.g. http://www.openwaves.net/webssari.htm for static code analysis of PHP for
security holes, but last I checked they hadn't released their code). If you do
ever come across anything free-as-in-speech that does static code analysis for
PHP, then please email me the link, I'll be very grateful.

> It could be handy to have around.

Very handy. Some of the things it spots in seconds can lead to subtly wrong
behaviour that I'm sure could take hours and hours to find by hand. I highly
recommend static code analysis as a way of eliminating bugs and making code cleaner.

I cleaned up most occurences of unused globals in the code.

My original question was more a case of, "how are you detecting them as unused?"
- I know what an unused global *is*. Nice work. :)

Created attachment 1321 [details]
List of unused variables, unused globals, etc. in MediaWiki 1.5.6

Refresh for MediaWiki 1.5.6.

FYI, the following warnings / errors stand out (to me, at least) as being
notably different :

Mediawiki-1.5.6\maintenance\splitLanguageFiles.inc
		Parsing Error: parse error, unexpected ';', expecting ')' (line
1174) [array declaration does not end, so this is not a valid PHP file]

Mediawiki-1.5.6\maintenance\storage\resolveStubs.php
		Wrong break depth (line 87) - [Use of 'continue' is
meaningless, as not in a loop]

H:\MediaWiki\mediawiki-1.5.6\mediawiki-1.5.6\includes\ParserXML.php
		Use of deprecated call-time pass-by-reference (line 325) [The
'&' prefix in the function call is not required, as it is specified in the
function declaration]

==========================================================

My notes to myself on how to recreate this file and remove most of the dross:

* Run code analyzer, copy and paste to ultraedit.
* Ctrl-A, indent all with two tabs.
* Search replace: "^t^tH:\MediaWiki\mediawiki-1.5.6\m" with "M" (no regex).
* Search replace: "\t\tGlobal variable \$(.*)  was used before it was defined
\(line (\d*)\)\r\n" with "" (regex on)
* Search replace: "\t\tAssignment in condition \(line (\d*)\)\r\n" with ""
(regex on)
* Search replace: "\t\t\$(.*) is passed by reference without being modified
\(line (\d*)\)\r\n" with "" (regex on)
* Search replace: "\t\tUnsafe use of variable in call include\(\)/require\(\)
\(line (\d*)\)\r\n" with "" (regex on)
* Search replace: "\t\tif-if-else without curly braces \(line (\d*)\)\r\n" with
"" (regex on)
* Search replace: "\t\tVariable \$matches was used before it was defined \(line
(\d*)\)\r\n" with "" (regex on)
* Search replace: "\t\tVariable \$m2 was used before it was defined \(line
(\d*)\)\r\n" with "" (regex on)
* Search replace: "\t\tVariable \$m was used before it was defined \(line
(\d*)\)\r\n" with "" (regex on)
* Search replace: "\t\tVariable \$mat was used before it was defined \(line
(\d*)\)\r\n" with "" (regex on)
* Search replace: "\t\tVariable \$n was used before it was defined \(line
(\d*)\)\r\n" with "" (regex on)
* Search replace: "Mediawiki-1.5.6\\([^\n]*)\r\nMediawiki-1.5.6" with
"Mediawiki-1.5.6" (regex on)

Created attachment 1438 [details]
unused globals script

Here's a perl script designed to report and eliminate unused global variables.
It detects quite a lot of them in current CVS.

Use at own risk :-)

Created attachment 1510 [details]
List of unused variables, unused globals, etc. in MediaWiki 1.6.3

Attach update for MediaWiki 1.6.3.

A few errors stand out, as being more than just unused globals:

Problem: Variable $wgHideInterlangageLinks appears only once (line 337 of
Mediawiki-1.6.3\maintenance\dumpHTML.inc)
Explanation: Typo. You want $wgHideInterlanguageLinks (note the extra 'U').

Problem: Variable $wgMathPath appears only once (line 340 of
Mediawiki-1.6.3\maintenance\dumpHTML.inc)
Explanation: You want to add $wgMathPath to the list of globals, because it's
not there at the moment, and so this is only changing $wgMathPath in the local
scope (and then not using it), whereas you want the global scope.

Problem: Variable $wgPersistentObjects appears only once (line 47 of
Mediawiki-1.6.3\includes\PersistentObject.php). 
Explanation: You probably want a "global $wgPersistentObjects" in that
function, or at least "$this->wgPersistentObjects". At the moment though
$wgPersistentObjects falls out of scope every time the function ends, so there
won't be much persistence going on.

Problem: Variable $wgContLanguageCode appears only once (line 1376 of
Mediawiki-1.6.3\config\index.php).
Explanation: You probably want a "global $wgContLanguageCode;" at the start of
that if statement.

Problem: Use of deprecated call-time pass-by-reference (line 661 of
Mediawiki-1.6.3\includes\DatabaseOracle.php)
Explanation: Arg 3 is already a ref (see function declaration at
http://php.net/manual/en/function.oci-bind-by-name.php ), so:
- oci_bind_by_name($stmt, ":bobj", &$blob, -1, OCI_B_BLOB);
+ oci_bind_by_name($stmt, ":bobj", $blob, -1, OCI_B_BLOB);

Problem: Use of deprecated call-time pass-by-reference (line 325 of
Mediawiki-1.6.3\includes\ParserXML.php)
Explanation: Arg 3 is already a ref (see function declaration on line 190 of
same file), so:
- $ret .= $this->getTemplateXHTML($title, $parts, & $parser);
+ $ret .= $this->getTemplateXHTML($title, $parts, $parser);

can you run your tool trunk instead of REL1_6 ? thanks :)

Created attachment 1634 [details]
List of unused variables, unused globals, etc on MediaWiki SVN checkout

Did "svn co http://svn.wikimedia.org/svnroot/mediawiki/trunk/phase3", then ran
tool.

Mediawiki\SVN checkout\phase3\includes\Article.php
        Both empty return and return with value used in function
getPreloadedText() (line 231)

This message can be helpful, but most of the time it get confused
when we want to return empty string ( '' ).


The value of variable $foobar was never used

That one get confused with extract() of course, but also when we
do stuff like:

$foobar = object->method();
function( &$foobar);

The same happen when we use a variable and return it immediatly:

function foo() {
 $ret = 'some value';
 return $ret;
}

$key is shown as unused in following foreach, but we HAVE to put $key :p

foreach( $array as $key => $value ) {
  print $value;
}


I am not sure the tool is able to find variables in string concatenation.
$curIdEq in EnhancedChangesList::recentChangesLine() is a good example.


Variable $array was used before it was defined

This one get confused when using:
preg_match_all( 'regex', $string, $array );

Not sure if we should $array = array(); before calling it.


Cleaned some of them.

> That one get confused with extract()
> ...
> The same happen when we use a variable and return it immediately:

The tool does have bugs, but it is nevertheless useful. A more recent version
may have fewer bugs, but I only have an older version.

Brion had a small number of licenses for the current version (
http://mail.wikipedia.org/pipermail/wikitech-l/2006-April/034888.html ), so it
may be worth asking him, but bear in mind that they may have already been taken.

> but most of the time it get confused
> when we want to return empty string ( '' )

Mostly I saw this with this type of thing:
if ($x == 1) return "blah";
else return;

I'd personally favour this:
else return '';

(which seems more explicit to me).

> $key is shown as unused in following foreach, but we HAVE to put $key :p
> 
> foreach( $array as $key => $value ) {
>    print $value;
> }

What I do in my stuff to avoid this message is if you only want the value do this:
foreach( $array as $value )
and if you only want the key do this:
foreach( array_keys($array) as $key )
Thus avoiding the unused $key or $value variable.

> Variable $array was used before it was defined
> This one get confused when using:
> preg_match_all( 'regex', $string, $array );
> Not sure if we should $array = array(); before calling it.

That's what I personally do. It's not required of course, but it seems
marginally more explicit to me, and it certainly doesn't hurt.

1.5 is now obsolete; marking WONTFIX.

I meant it more for the current latest and greatest version of MediaWiki (I
should not have used 1.5 in the description, my bad).
Not that it makes much difference, but reopening and will then shortly mark this
as "resolved (fixed)".

Mostly done in r17880, r19145, r17991, and r17952.
Related smaller fixes also done in r19146, r19147, r19148, r19149, r19150,
r19152, r19153, r17992, r17993, r17994, r17995, r17996, r17882, r17881, r19154,
r19159, and some prior to that  by Ashar.
Marking as Resolved / Fixed.