r89343 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r89342‎ \| r89343 \| r89344 >
Date:	15:28, 2 June 2011
Author:	platonides
Status:	ok (Comments)
Tags:
Comment:	Added -Wname and -Wno-name parameters to configure the desired warnings Disable by default evil-@ and missed-docblock
Modified paths:	/trunk/tools/code-utils/check-vars.php (modified) (history)

Diff [purge]

Index: trunk/tools/code-utils/check-vars.php
—	—	@@ -103,6 +103,36 @@
104	104	'wfVarDump' => 'Debugging function.', //var_export() wrapper
105	105	);
106	106
	107	+ static $enabledWarnings = array(
	108	+ 'utf8-bom' => true,
	109	+ 'php-trail' => true,
	110	+ 'double-php-open' => true,
	111	+ 'double-;' => true,
	112	+ 'this-in-static' => true,
	113	+ 'missed-docblock' => false,
	114	+ 'profileout' => true,
	115	+ 'evil-@' => false,
	116	+ 'global-in-switch' => true,
	117	+ 'global-as-local' => true,
	118	+ 'global-names' => true,
	119	+ 'double-globals' => true,
	120	+ 'unused-global' => true,
	121	+ 'undefined-global' => true,
	122	+ 'function-function' => true,
	123	+ 'missing-function' => true,
	124	+ 'missing-class' => true,
	125	+ 'orphan-parent' => true,
	126	+ '$self' => true,
	127	+ 'function-throw' => true,
	128	+ 'undefined-constant' => true,
	129	+ 'missing-requires' => true,
	130	+ 'deprecated-calls' => true,
	131	+ 'deprecated-might' => true,
	132	+ 'poisoned-function' => true,
	133	+ 'error' => true
	134	+ );
	135	+
	136	+
107	137	protected $generateDeprecatedList = false;
108	138	protected $generateParentList = false;
109	139
—	—	@@ -258,11 +288,11 @@
259	289
260	290	$source = file_get_contents( $file );
261	291	if ( substr( $source, 0, 3 ) == "\xEF\xBB\xBF" ) {
262		~~- $this->warning( "$file has an UTF-8 BOM" );~~
	292	+ $this->warning( 'utf8-bom', "$file has an UTF-8 BOM" );
263	293	}
264	294	$source = rtrim( $source );
265	295	if ( substr( $source, -2 ) == '?>' ) {
266		~~- $this->warning( "?> at end of file is deprecated in MediaWiki code" );~~
	296	+ $this->warning( 'php-trail', "?> at end of file is deprecated in MediaWiki code" );
267	297	}
268	298	if ( $shortcircuit && !preg_match( "/^[^'\"#]function [^\"']*\$/m", $source ) ) {
269	299	$this->mTokens = array();
—	—	@@ -284,12 +314,12 @@
285	315
286	316	if ( $lastMeaningfulToken[0] == T_OPEN_TAG && $token[0] == T_OPEN_TAG ) {
287	317	# See r69767
288		~~- $this->warning( "{$token[1]} in line {$token[2]} after {$lastMeaningfulToken[1]} in line {$lastMeaningfulToken[2]}" );~~
	318	+ $this->warning( 'double-php-open', "{$token[1]} in line {$token[2]} after {$lastMeaningfulToken[1]} in line {$lastMeaningfulToken[2]}" );
289	319	}
290	320	if ( $token == ';' ) {
291	321	if ( $lastMeaningfulToken == ';' ) {
292	322	# See r72751, warn on ;;
293		~~- $this->warning( "Empty statement" );~~
	323	+ $this->warning( 'double-;', "Empty statement" );
294	324	} elseif ( $lastMeaningfulToken[0] == T_FOR ) {
295	325	# But not on infinte for loops: for ( ; ; )
296	326	$currentToken = array(';', ';', $lastMeaningfulToken[2] );
—	—	@@ -325,7 +355,7 @@
326	356	if ( $token[0] == T_COMMENT ) {
327	357	if ( substr( $token[1], 0, 2 ) == '/' && substr( $token[1], 0, 3 ) != '/*'
328	358	&& preg_match( '/^\s+\*(?!\/)/m', $token[1] ) && strpos( $token[1], "\$separatorTransformTable = array( ',' => '' )" ) === false ) {
329		~~- $this->warning( "Multiline comment with /* in line $token[2]" );~~
	359	+ $this->warning( 'missed-docblock', "Multiline comment with /* in line $token[2]" );
330	360	}
331	361	}
332	362
—	—	@@ -421,7 +451,7 @@
422	452	$this->purgeGlobals();
423	453	if ( ! $this->mBraces ) {
424	454	if ( $this->mInProfilingFunction && $this->mAfterProfileOut & 1 ) {
425		~~- $this->warning( "Reached end of $this->mClass::$this->mFunction with last statement not being wfProfileOut" );~~
	455	+ $this->warning( 'profileout', "Reached end of $this->mClass::$this->mFunction with last statement not being wfProfileOut" );
426	456	}
427	457
428	458	$this->mStatus = self::WAITING_FUNCTION;
—	—	@@ -437,12 +467,12 @@
438	468	$this->mAfterProfileOut = 3;
439	469	}
440	470	} elseif ( $token == '@' ) {
441		~~- $this->warning( "Use of @ operator in function {$this->mFunction}" );~~
	471	+ $this->warning( 'evil-@', "Use of @ operator in function {$this->mFunction}" );
442	472	} elseif ( is_array ( $token ) ) {
443	473	if ( $token[0] == T_GLOBAL ) {
444	474	$this->mStatus = self::IN_GLOBAL;
445	475	if ( $this->mInSwitch ) {
446		~~- $this->warning( "Defining global variables inside a switch in line $token[2], function {$this->mFunction}" );~~
	476	+ $this->warning( 'global-in-switch', "Defining global variables inside a switch in line $token[2], function {$this->mFunction}" );
447	477	}
448	478	} elseif ( ( $token[0] == T_CURLY_OPEN ) \|\| ( $token[0] == T_DOLLAR_OPEN_CURLY_BRACES ) ) {
449	479	// {$ and ${ and All these three end in }, so we need to open an extra brace to balance
—	—	@@ -457,7 +487,7 @@
458	488	# $this->debug( "Found variable $token[1]" );
459	489
460	490	if ( ( $token[1] == '$this' ) && in_array( T_STATIC, $this->mFunctionQualifiers ) ) {
461		~~- $this->warning( "Use of \$this in static method function {$this->mFunction} in line $token[2]" );~~
	491	+ $this->warning( 'this-in-static', "Use of \$this in static method function {$this->mFunction} in line $token[2]" );
462	492	}
463	493
464	494	if ( $lastMeaningfulToken[0] == T_PAAMAYIM_NEKUDOTAYIM ) {
—	—	@@ -466,17 +496,17 @@
467	497	if ( isset( $this->mFunctionGlobals[ $token[1] ] ) ) {
468	498	$this->mFunctionGlobals[ $token[1] ][0] ++;
469	499	} elseif ( $this->shouldBeGlobal( $token[1] ) ) {
470		~~- $this->warning( "{$token[1]} is used as local variable in line $token[2], function {$this->mFunction}" );~~
	500	+ $this->warning( 'global-as-local', "{$token[1]} is used as local variable in line $token[2], function {$this->mFunction}" );
471	501	}
472	502	}
473	503	} elseif ( $token[0] == T_RETURN && $this->mInProfilingFunction ) {
474	504	if ( $this->mAfterProfileOut == 2 ) {
475	505	$this->mAfterProfileOut = 0;
476	506	} else {
477		~~- $this->warning( "$token[1] in line $token[2] is not preceded by wfProfileOut" );~~
	507	+ $this->warning( 'profileout', "$token[1] in line $token[2] is not preceded by wfProfileOut" );
478	508	}
479	509	} elseif ( $token[0] == T_FUNCTION ) {
480		~~- $this->warning( "Uh? Function inside function? A lamda function?" );~~
	510	+ $this->warning( 'function-function', "Uh? Function inside function? A lamda function?" );
481	511	$this->error( $token );
482	512	} elseif ( $token[0] == T_SWITCH ) {
483	513	if ( !$this->mInSwitch )
—	—	@@ -484,7 +514,7 @@
485	515	} elseif ( ( $token[0] == T_PAAMAYIM_NEKUDOTAYIM ) && is_array( $lastMeaningfulToken ) && ( $lastMeaningfulToken[0] == T_VARIABLE ) ) {
486	516	if ( ( $lastMeaningfulToken[1] == '$self' ) \|\| ( $lastMeaningfulToken[1] == '$parent' ) ) {
487	517	# Bug of r69904
488		~~- $this->warning( "$lastMeaningfulToken[1]:: used in line $lastMeaningfulToken[2] It probably should have been " . substr( $lastMeaningfulToken[1], 1 ) . "::" );~~
	518	+ $this->warning( '$self', "$lastMeaningfulToken[1]:: used in line $lastMeaningfulToken[2] It probably should have been " . substr( $lastMeaningfulToken[1], 1 ) . "::" );
489	519	}
490	520	} elseif ( ( $token[0] == T_STRING ) && ( is_array( $lastMeaningfulToken )
491	521	&& in_array( $lastMeaningfulToken[0], array( T_OBJECT_OPERATOR, T_PAAMAYIM_NEKUDOTAYIM ) ) ) ) {
—	—	@@ -514,7 +544,7 @@
515	545	// throw Exception("Foo"); -> Exception() is a function
516	546	// throw new Exception("Foo"); -> Exception is a class.
517	547
518		~~- $this->warning( "Not using new when throwing token {$token[1]} in line $token[2], function {$this->mFunction}" );~~
	548	+ $this->warning( 'function-throw', "Not using new when throwing token {$token[1]} in line $token[2], function {$this->mFunction}" );
519	549	}
520	550	}
521	551
—	—	@@ -560,7 +590,7 @@
561	591	} else {
562	592
563	593	if ( !defined( $lastMeaningfulToken[1] ) && !in_array( $lastMeaningfulToken[1], $this->mConstants ) && !self::inIgnoreList( $lastMeaningfulToken[1], self::$constantIgnorePrefixes ) ) {
564		~~- $this->warning( "Use of undefined constant $lastMeaningfulToken[1] in line $lastMeaningfulToken[2]" );~~
	594	+ $this->warning( 'undefined-constant', "Use of undefined constant $lastMeaningfulToken[1] in line $lastMeaningfulToken[2]" );
565	595	}
566	596	}
567	597	}
—	—	@@ -579,11 +609,11 @@
580	610	if ( is_array( $token ) ) {
581	611	if ( $token[0] == T_VARIABLE ) {
582	612	if ( !$this->shouldBeGlobal( $token[1] ) && !$this->canBeGlobal( $token[1] ) ) {
583		~~- $this->warning( "Global variable {$token[1]} in line {$token[2]}, function {$this->mFunction} does not follow coding conventions" );~~
	613	+ $this->warning( 'global-names', "Global variable {$token[1]} in line {$token[2]}, function {$this->mFunction} does not follow coding conventions" );
584	614	}
585	615	if ( isset( $this->mFunctionGlobals[ $token[1] ] ) ) {
586	616	if ( !$this->mInSwitch ) {
587		~~- $this->warning( $token[1] . " marked as global again in line {$token[2]}, function {$this->mFunction}" );~~
	617	+ $this->warning( 'double-globals', $token[1] . " marked as global again in line {$token[2]}, function {$this->mFunction}" );
588	618	}
589	619	} else {
590	620	$this->checkGlobalName( $token[1] );
—	—	@@ -648,7 +678,7 @@
649	679
650	680	if ( !file_exists( $requirePath ) ) {
651	681	if ( strpos( $requirePath, '$' ) === false ) {
652		~~- $this->warning( "Did not found the expected require of $requirePath" );~~
	682	+ $this->warning( 'missing-requires', "Did not found the expected require of $requirePath" );
653	683	}
654	684	} else {
655	685	$requirePath = realpath( $requirePath );
—	—	@@ -684,7 +714,7 @@
685	715	if ( isset( $this->mFunctionGlobals[ $token[1] ] ) ) {
686	716	$this->mFunctionGlobals[ $token[1] ][0] ++;
687	717	} elseif ( $this->shouldBeGlobal( $token[1] ) ) {
688		~~- $this->warning( "{$token[1]} is used as local variable in line $token[2], function {$this->mFunction}" );~~
	718	+ $this->warning( 'global-as-local', "{$token[1]} is used as local variable in line $token[2], function {$this->mFunction}" );
689	719	}
690	720	}
691	721	if ( $token == '.' ) {
—	—	@@ -746,7 +776,7 @@
747	777	$class = $token['class'];
748	778	do {
749	779	if ( in_array( $class, $mwDeprecatedFunctions[ $token[1] ] ) ) {
750		~~- $this->warning( "Non deprecated function $this->mFunction calls deprecated function {$token['class']}::{$token[1]} in line {$token[2]}" );~~
	780	+ $this->warning( 'deprecated-calls', "Non deprecated function $this->mFunction calls deprecated function {$token['class']}::{$token[1]} in line {$token[2]}" );
751	781	return;
752	782	}
753	783	if ( !isset( $mwParentClasses[ $class ] ) ) {
—	—	@@ -755,7 +785,7 @@
756	786	$class = $mwParentClasses[ $class ];
757	787	} while( true );
758	788	} else if ( isset( $token['base'] ) ) { # Avoid false positives for local functions, see maintenance/rebuildInterwiki.inc
759		~~- $this->warning( "Non deprecated function $this->mFunction may be calling deprecated function " .~~
	789	+ $this->warning( 'deprecated-might', "Non deprecated function $this->mFunction may be calling deprecated function " .
760	790	implode( '/', $mwDeprecatedFunctions[ $token[1] ] ) . "::" . $token[1] . " in line {$token[2]}" );
761	791	}
762	792	}
—	—	@@ -779,7 +809,7 @@
780	810	// Allow var_dump if the function purpose is really to dump contents
781	811	return;
782	812	}
783		~~- $this->warning( "Poisoned function {$token[1]} called from {$this->mFunction} in line {$token[2]}: " . self::$poisonedFunctions[strtolower($token[1])] );~~
	813	+ $this->warning( 'poisoned-function', "Poisoned function {$token[1]} called from {$this->mFunction} in line {$token[2]}: " . self::$poisonedFunctions[strtolower($token[1])] );
784	814	return;
785	815	}
786	816
—	—	@@ -795,7 +825,7 @@
796	826	}
797	827
798	828	if ( $warn == 'now' ) {
799		~~- $this->warning( "Unavailable function {$token[1]} in line {$token[2]}" );~~
	829	+ $this->warning( 'missing-function', "Unavailable function {$token[1]} in line {$token[2]}" );
800	830	} else if ( $warn == 'defer' ) {
801	831	// Defer to the end of the file
802	832	$this->mUnknownFunctions[] = $token;
—	—	@@ -862,11 +892,14 @@
863	893	$msg .= " in line $token[2]";
864	894	}
865	895	$msg .= "\n";
866		~~- $this->warning( $msg );~~
	896	+ $this->warning( 'error', $msg );
867	897	die( 1 );
868	898	}
869	899
870		~~- function warning( $msg ) {~~
	900	+ function warning( $name, $msg ) {
	901	+ if ( !self::$enabledWarnings[$name] ) {
	902	+ return;
	903	+ }
871	904	if ( !$this->mProblemCount ) {
872	905	echo "Problems in {$this->mFilename}:\n";
873	906	}
—	—	@@ -917,7 +950,7 @@
918	951	# the if block (see r69883).
919	952
920	953	if ( $globalData[0] == 0 ) {
921		~~- $this->warning( "Unused global $globalName in function {$this->mFunction} line $globalData[2]" );~~
	954	+ $this->warning( 'unused-global', "Unused global $globalName in function {$this->mFunction} line $globalData[2]" );
922	955	}
923	956	unset( $this->mFunctionGlobals[$globalName] );
924	957	}
—	—	@@ -928,7 +961,7 @@
929	962	if ( substr( $name, 0, 3 ) == '$wg' ) {
930	963	if ( ( self::$mDefaultSettingsGlobals != null ) && !in_array( $name, self::$mDefaultSettingsGlobals ) ) {
931	964	if ( !isset( self::$mGlobalsPerFile[$name] ) \|\| !in_array( basename( $this->mFilename ) , self::$mGlobalsPerFile[$name] ) ) {
932		~~- $this->warning( "Global variable $name is not present in DefaultSettings" );~~
	965	+ $this->warning( 'undefined-global', "Global variable $name is not present in DefaultSettings" );
933	966	}
934	967	}
935	968	}
—	—	@@ -971,7 +1004,7 @@
972	1005
973	1006	if ( !isset( $wgAutoloadLocalClasses[$token[1]] ) && !in_array( $token[1], $this->mKnownFileClasses ) ) {
974	1007	if ( $warn == 'now' ) {
975		~~- $this->warning( "Use of unknown class $token[1] in line $token[2]" );~~
	1008	+ $this->warning( 'missing-class', "Use of unknown class $token[1] in line $token[2]" );
976	1009	} else if ( $warn == 'defer' ) {
977	1010	// Defer to the end of the file
978	1011	$this->mUnknownClasses[] = $token;
—	—	@@ -1001,7 +1034,7 @@
1002	1035	if ( !is_null( $this->mParent ) ) {
1003	1036	return $this->mParent;
1004	1037	}
1005		~~- $this->warning( "Use of parent in orphan class {$this->mClass} in line $token[2]" );~~
	1038	+ $this->warning( 'orphan-parent', "Use of parent in orphan class {$this->mClass} in line $token[2]" );
1006	1039	return "-";
1007	1040	}
1008	1041
—	—	@@ -1042,9 +1075,23 @@
1043	1076	$cv->setGenerateParentList( true );
1044	1077	array_shift( $argv );
1045	1078	}
	1079	+
	1080	+foreach ( $argv as $arg ) {
	1081	+ if ( preg_match( '/^-W(no-)?(.*)/', $arg, $m ) ) {
	1082	+ if ( !isset( CheckVars::$enabledWarnings[ $m[2] ] ) ) {
	1083	+ var_dump($m);
	1084	+ die( "Wrong warning name $arg\n" );
	1085	+ }
	1086	+ CheckVars::$enabledWarnings[ $m[2] ] = strlen( $m[1] ) == 0;
	1087	+ }
	1088	+}
	1089	+
1046	1090	$cv->preloadFiles( array( $IP . '/includes/GlobalFunctions.php' ) );
1047	1091
1048	1092	foreach ( $argv as $arg ) {
	1093	+ if ( substr( $arg, 0, 2 ) == '-W' )
	1094	+ continue;
	1095	+
1049	1096	$cv->load( $arg );
1050	1097	$cv->execute();
1051	1098	}

Follow-up revisions

Revision	Commit summary	Author	Date
r89509	-Whelp to dump available warning keys from r89343	hashar	16:52, 5 June 2011

Comments

#Comment by 😂 (talk | contribs) 15:52, 2 June 2011

Not sure why you're disabling @ by default. It's really evil.

#Comment by Platonides (talk | contribs) 20:12, 2 June 2011

It generates a long list of warnings. The 105 instances I provided at r88187 only decreased to 97. If nobody is going to fix them, I prefer not to see them. Besides, I don't think that in some cases it's too evil (eg. an array access with constant key).

Status & tagging log

16:52, 5 June 2011 Hashar (talk | contribs) changed the status of r89343 [removed: new added: ok]