Skip to content

Add phan as static code analyser#31807

Merged
DeepDiver1975 merged 16 commits intomasterfrom
experimental-phan
Jun 21, 2018
Merged

Add phan as static code analyser#31807
DeepDiver1975 merged 16 commits intomasterfrom
experimental-phan

Conversation

@patrickjahns
Copy link
Contributor

@patrickjahns patrickjahns commented Jun 16, 2018

Description

https://github.com/phan/phan is a static code analysis tool that can help us improve our code basis and help find bugs.

Right now the .phan/config is very lenient - we should first get it to be ✅ for this lenient definition, merge / introduce it to our code basis and then gradually increase the ruleset to further improve our code base.

note: phan is currently introduced as phar - this allows us for backporting it to stable10 - without having dependencie issues, as phan officialy needs to run on php7+ ( however it can analyse code in 7 to be backward compatible to php5.6 )

Motivation and Context

Improve code quality and ownCloud as project

pending fixes

lib/private/Encryption/Util.php:123 PhanUndeclaredClassConstant Reference to constant ID from undeclared class \OCA\Encryption\Crypto\Encryption

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • QA / code related

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@patrickjahns patrickjahns self-assigned this Jun 16, 2018
@patrickjahns patrickjahns force-pushed the experimental-phan branch 3 times, most recently from 20a7d77 to 04adc14 Compare June 16, 2018 18:38
@codecov
Copy link

codecov bot commented Jun 16, 2018

Codecov Report

Merging #31807 into master will increase coverage by 1%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #31807     +/-   ##
===========================================
+ Coverage     63.25%   64.26%     +1%     
  Complexity    18459    18459             
===========================================
  Files          1158     1097     -61     
  Lines         69328    62193   -7135     
  Branches       1261        0   -1261     
===========================================
- Hits          43854    39967   -3887     
+ Misses        25104    22226   -2878     
+ Partials        370        0    -370
Flag Coverage Δ Complexity Δ
#javascript ? ?
#phpunit 64.26% <0%> (-0.23%) 18459 <0> (ø)
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/Upload/AssemblyStreamZsync.php 88.37% <ø> (ø) 33 <0> (ø) ⬇️
lib/private/Memcache/Redis.php 0% <ø> (ø) 27 <0> (ø) ⬇️
apps/files_external/lib/Lib/Storage/Swift.php 66.16% <ø> (ø) 0 <0> (ø) ⬇️
apps/files_external/lib/config.php 35% <ø> (ø) 0 <0> (ø) ⬇️
apps/files_trashbin/lib/Trashbin.php 77.35% <ø> (ø) 146 <0> (ø) ⬇️
apps/dav/lib/CardDAV/SyncService.php 47.61% <ø> (ø) 27 <0> (ø) ⬇️
lib/private/Files/Storage/DAV.php 80.67% <ø> (ø) 0 <0> (ø) ⬇️
lib/private/Share20/DefaultShareProvider.php 98.33% <ø> (ø) 114 <0> (ø) ⬇️
lib/private/legacy/user.php 22.98% <ø> (ø) 72 <0> (ø) ⬇️
apps/dav/lib/Upload/ChunkingPluginZsync.php 100% <ø> (ø) 16 <0> (ø) ⬇️
... and 88 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3eb3bd9...a479f8a. Read the comment docs.

use OCP\IRequest;
use OCP\IUserManager;
use OCP\Share;
use OCP\Share\IShare;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phan was complaining about this missing import:

apps/federatedfilesharing/lib/RequestHandler.php:574 PhanUndeclaredClassMethod Call to method getId from undeclared class \OCA\FederatedFileSharing\IShare (Did you mean interface \Icewind\SMB\IShare)

*
* @param string $path
* @return \OpenCloud\OpenStack\ObjectStorage\Resource\DataObject|bool object
* @return DataObject | bool object
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes:

apps/files_external/lib/Lib/Storage/Swift.php:304 PhanUndeclaredClassMethod Call to method getLastModified from undeclared class \OpenCloud\OpenStack\ObjectStorage\Resource\DataObject
apps/files_external/lib/Lib/Storage/Swift.php:310 PhanUndeclaredClassMethod Call to method getMetadata from undeclared class \OpenCloud\OpenStack\ObjectStorage\Resource\DataObject
apps/files_external/lib/Lib/Storage/Swift.php:321 PhanUndeclaredClassMethod Call to method getContentLength from undeclared class \OpenCloud\OpenStack\ObjectStorage\Resource\DataObject
apps/files_external/lib/Lib/Storage/Swift.php:438 PhanUndeclaredClassMethod Call to method saveMetadata from undeclared class \OpenCloud\OpenStack\ObjectStorage\Resource\DataObject
apps/files_external/lib/Lib/Storage/Swift.php:467 PhanUndeclaredClassMethod Call to method copy from undeclared class \OpenCloud\OpenStack\ObjectStorage\Resource\DataObject
apps/files_external/lib/Lib/Storage/Swift.php:482 PhanUndeclaredClassMethod Call to method copy from undeclared class \OpenCloud\OpenStack\ObjectStorage\Resource\DataObject

// namespace
const ZSYNC_PROPERTYNAME = '{http://owncloud.org/ns}zsync';

/** @var OC\Files\View */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixes:

[php-phan:L12:73s] apps/dav/lib/Files/ZsyncPlugin.php:42 PhanUndeclaredClassMethod Call to method mkdir from undeclared class \OCA\DAV\Files\OC\Files\View
[php-phan:L13:73s] apps/dav/lib/Files/ZsyncPlugin.php:73 PhanUndeclaredClassMethod Call to method file_exists from undeclared class \OCA\DAV\Files\OC\Files\View
[php-phan:L14:73s] apps/dav/lib/Files/ZsyncPlugin.php:78 PhanUndeclaredClassMethod Call to method getFileInfo from undeclared class \OCA\DAV\Files\OC\Files\View
[php-phan:L15:73s] apps/dav/lib/Files/ZsyncPlugin.php:80 PhanUndeclaredClassMethod Call to method file_exists from undeclared class \OCA\DAV\Files\OC\Files\View
[php-phan:L16:73s] apps/dav/lib/Files/ZsyncPlugin.php:81 PhanUndeclaredClassMethod Call to method file_get_contents from undeclared class \OCA\DAV\Files\OC\Files\View
[php-phan:L17:73s] apps/dav/lib/Files/ZsyncPlugin.php:108 PhanUndeclaredClassMethod Call to method file_exists from undeclared class \OCA\DAV\Files\OC\Files\View
[php-phan:L18:73s] apps/dav/lib/Files/ZsyncPlugin.php:113 PhanUndeclaredClassMethod Call to method getFileInfo from undeclared class \OCA\DAV\Files\OC\Files\View
[php-phan:L19:73s] apps/dav/lib/Files/ZsyncPlugin.php:115 PhanUndeclaredClassMethod Call to method file_exists from undeclared class \OCA\DAV\Files\OC\Files\View
[php-phan:L20:73s] apps/dav/lib/Files/ZsyncPlugin.php:116 PhanUndeclaredClassMethod Call to method unlink from undeclared class \OCA\DAV\Files\OC\Files\View
[php-phan:L21:73s] apps/dav/lib/Files/ZsyncPlugin.php:134 PhanUndeclaredClassMethod Call to method is_file from undeclared class \OCA\DAV\Files\OC\Files\View
[php-phan:L22:73s] apps/dav/lib/Files/ZsyncPlugin.php:137 PhanUndeclaredClassMethod Call to method getFileInfo from undeclared class \OCA\DAV\Files\OC\Files\View
[php-phan:L23:73s] apps/dav/lib/Files/ZsyncPlugin.php:139 PhanUndeclaredClassMethod Call to method file_exists from undeclared class \OCA\DAV\Files\OC\Files\View

use OCP\Files\StorageNotAvailableException;
use OCP\Util;
use Sabre\DAV\Xml\Property\ResourceType;
use Sabre\HTTP\Client;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixes:

[php-phan:L18:73s] lib/private/Files/Storage/DAV.php:198 PhanUndeclaredClassMethod Call to method propfind from undeclared class \OC\Files\Storage\Client
[php-phan:L19:73s] lib/private/Files/Storage/DAV.php:255 PhanUndeclaredClassMethod Call to method propfind from undeclared class \OC\Files\Storage\Client
[php-phan:L20:73s] lib/private/Files/Storage/DAV.php:421 PhanUndeclaredClassMethod Call to method propfind from undeclared class \OC\Files\Storage\Client
[php-phan:L21:73s] lib/private/Files/Storage/DAV.php:447 PhanUndeclaredClassMethod Call to method proppatch from undeclared class \OC\Files\Storage\Client
[php-phan:L22:73s] lib/private/Files/Storage/DAV.php:449 PhanUndeclaredClassMethod Call to method propfind from undeclared class \OC\Files\Storage\Client
[php-phan:L23:73s] lib/private/Files/Storage/DAV.php:525 PhanUndeclaredClassMethod Call to method request from undeclared class \OC\Files\Storage\Client
[php-phan:L24:73s] lib/private/Files/Storage/DAV.php:557 PhanUndeclaredClassMethod Call to method request from undeclared class \OC\Files\Storage\Client
[php-phan:L25:73s] lib/private/Files/Storage/DAV.php:654 PhanUndeclaredClassMethod Call to method request from undeclared class \OC\Files\Storage\Client

@PVince81 @DeepDiver1975 - this should be the correct client to be parsed right? (at least that's what https://github.com/owncloud/core/blob/92b658a60e6c28ef9dd7594384180e0d73e96660/lib/public/Http/Client/IWebDavClientService.php typehints )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@patrickjahns patrickjahns requested a review from PVince81 June 16, 2018 21:09
@patrickjahns
Copy link
Contributor Author

Having a stub for a \OC_Theme fixes some bugs in legacy:

[php-phan:L19:66s] lib/private/legacy/defaults.php:86 PhanUndeclaredClassMethod Call to method __construct from undeclared class \OC_Theme
[php-phan:L20:66s] lib/private/legacy/defaults.php:107 PhanUndeclaredClassMethod Call to method getBaseUrl from undeclared class \OC_Theme
[php-phan:L21:66s] lib/private/legacy/defaults.php:119 PhanUndeclaredClassMethod Call to method getSyncClientUrl from undeclared class \OC_Theme
[php-phan:L22:66s] lib/private/legacy/defaults.php:131 PhanUndeclaredClassMethod Call to method getiOSClientUrl from undeclared class \OC_Theme
[php-phan:L23:66s] lib/private/legacy/defaults.php:143 PhanUndeclaredClassMethod Call to method getiTunesAppId from undeclared class \OC_Theme
[php-phan:L24:66s] lib/private/legacy/defaults.php:155 PhanUndeclaredClassMethod Call to method getAndroidClientUrl from undeclared class \OC_Theme
[php-phan:L25:66s] lib/private/legacy/defaults.php:167 PhanUndeclaredClassMethod Call to method getDocBaseUrl from undeclared class \OC_Theme
[php-phan:L26:66s] lib/private/legacy/defaults.php:179 PhanUndeclaredClassMethod Call to method getTitle from undeclared class \OC_Theme
[php-phan:L27:66s] lib/private/legacy/defaults.php:191 PhanUndeclaredClassMethod Call to method getName from undeclared class \OC_Theme
[php-phan:L28:66s] lib/private/legacy/defaults.php:203 PhanUndeclaredClassMethod Call to method getHTMLName from undeclared class \OC_Theme
[php-phan:L29:66s] lib/private/legacy/defaults.php:215 PhanUndeclaredClassMethod Call to method getEntity from undeclared class \OC_Theme
[php-phan:L30:66s] lib/private/legacy/defaults.php:227 PhanUndeclaredClassMethod Call to method getSlogan from undeclared class \OC_Theme
[php-phan:L31:66s] lib/private/legacy/defaults.php:239 PhanUndeclaredClassMethod Call to method getLogoClaim from undeclared class \OC_Theme
[php-phan:L32:66s] lib/private/legacy/defaults.php:251 PhanUndeclaredClassMethod Call to method getShortFooter from undeclared class \OC_Theme
[php-phan:L33:66s] lib/private/legacy/defaults.php:274 PhanUndeclaredClassMethod Call to method getLongFooter from undeclared class \OC_Theme
[php-phan:L34:66s] lib/private/legacy/defaults.php:287 PhanUndeclaredClassMethod Call to method buildDocLinkToKey from undeclared class \OC_Theme
[php-phan:L35:66s] lib/private/legacy/defaults.php:298 PhanUndeclaredClassMethod Call to method getMailHeaderColor from undeclared class \OC_Theme

using stub for this is sensible - but I would prefer of having a default theme app ?

Reference: http://tools.ietf.org/html/rfc7231#section-4.3.4
*/
throw new Exception\BadRequest('Content-Range on PUT requests are forbidden.');
throw new BadRequest('Content-Range on PUT requests are forbidden.');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solves [php-phan:L11:67s] apps/dav/lib/Connector/Sabre/AutorenamePlugin.php:73 PhanUndeclaredClassMethod Call to method __construct from undeclared class \OCA\DAV\Connector\Sabre\Exception\BadRequest

@PVince81 - please check if the intention is the same as back then (found you as sole author of this class)

@patrickjahns
Copy link
Contributor Author

Since we moved encryption app into its own repository - this needs to be addressed somehow @sharidas @PVince81

https://github.com/owncloud/core/blob/master/lib/private/Encryption/Util.php#L123

lib/private/Encryption/Util.php:123 PhanUndeclaredClassConstant Reference to constant ID from undeclared class \OCA\Encryption\Crypto\Encryption

*/

use OCA\Files_External\AppInfo\Application;
use OCP\Files\External\Backend\Backend;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix for

 apps/files_external/lib/config.php:54 PhanUndeclaredClassMethod Call to method checkDependencies from undeclared class \Backend (Did you mean class \OCA\DAV\DAV\Sharing\Backend or class \OCA\Files_External\Lib\Backend\Backend)

\OC::$server->getUserManager()->registerBackend(self::$_usedBackends[$backend]);
break;
case 'dummy':
/* @phan-suppress-next-line PhanUndeclaredClassMethod */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixes

lib/private/legacy/user.php:102 PhanUndeclaredClassMethod Call to method __construct from undeclared class \Test\Util\User\Dummy

right now its the right approach - ondering through if a dummy user backend should be provided by an app?

.drone.yml Outdated
TEST_SUITE: php-cs-fixer

php-phan:
image: owncloudci/php:71_add_phpast
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be switched once owncloud-ci/php#61 is merged


/**
* @var \Icewind\SMB\IFileInfo[]
* @var ICache
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix for:

apps/files_external/lib/Lib/Storage/SMB.php:293 PhanNonClassMethodCall Call to method clear on non-class type \Icewind\SMB\IFileInfo[]|array<string,\Icewind\SMB\IFileInfo>|array<string,\Icewind\SMB\Wrapped\FileInfo>

the implementation used is a OC\Cache\CappedMemoryCache -> it implements ICache and I rather propose to typehint the interface and not the implementation

* @param integer $timestamp when the file was deleted
* @param IStorage|null $sourceStorage
* @param bool $forceCopy true to only make a copy of the versions into the trashbin
* @throws Exceptions\CopyRecursiveException
Copy link
Contributor Author

@patrickjahns patrickjahns Jun 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exception was missing from doc block - made phpstorm a little happier

* @param string $owner owner user id
* @param string $ownerPath path relative to the owner's home storage
* @param integer $timestamp when the file was deleted
* @param IStorage|null $sourceStorage
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This takes care of

apps/files_trashbin/lib/Trashbin.php:414 PhanNonClassMethodCall Call to method deleteAllFileKeys on non-class type ?mixed

However - it was introduced by this pr #28899 - and it seems to be 🍝 - there is no proper check if the storage provided at this stage is actually an encrypted storage / or inherits an interface. Also no tests were provided with that patch

@PVince81 @sharidas - please shed some light on this code path, as I am very hesitant to simply add this typehint

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as I see there is only a null check, so if not null it assumed it's the IStorage.

this one https://github.com/owncloud/core/pull/28899/files#diff-6425524450ee2e3178dec599f98a6f63R370 has no null check because the value of $copyKeysResult is set in the other block that has the null check. Maybe the static code checker is not able to understand this kind of code path ?

We should probably change the way we wrote this because a human reading this will be confused as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code analysis tool checks what information is given - if there is no type hinting - or no proper type declaration, it will assume mixed. Which essentially will creep in bugs as developers do not know what values can/should be passed.

As there are not tests that explicitly cover this behavior - it is also not documented - so it is not understandable for humans ( and not for any tool).

So the typehinting assumed is correct right? That at some point later we should move forward with the refactoring - this is however out of scope for this PR

}
self::$cache->watch($this->getNameSpace() . $key);
if ($this->get($key) === $old) {
/** @phan-suppress-next-line PhanNonClassMethodCall */
Copy link
Contributor Author

@patrickjahns patrickjahns Jun 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adresses:

lib/private/Memcache/Redis.php:142 PhanNonClassMethodCall Call to method exec on non-class type bool
lib/private/Memcache/Redis.php:161 PhanNonClassMethodCall Call to method exec on non-class type bool

suggestions to supress came from phan maintainers directly - phan/phan#1804 - will later look into creating a plugin as suggested

public function cad($key, $old) {
self::$cache->watch($this->getNameSpace() . $key);
if ($this->get($key) === $old) {
/** @phan-suppress-next-line PhanNonClassMethodCall */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrickjahns
Copy link
Contributor Author

Fixing

lib/private/Files/External/LegacyUtil.php:211 PhanUndeclaredClassCatch Catching undeclared class \OC\Files\External\Exception (Did you mean class \Exception or class \Icewind\SMB\Exception\Exception)

needs input from @PVince81 and @DeepDiver1975 as they're the ones who moved the code to core

code in question: https://github.com/owncloud/core/blob/master/lib/private/Files/External/LegacyUtil.php#L196-L214

from my pov - the second catch block is useless - https://github.com/owncloud/core/blob/master/lib/private/Files/External/LegacyUtil.php#L211-L214 - as it catches Exception - which is not imported / used.
Either we catch \Exception or we refactor to only one catch block?

namespace OC\Share20;

use OCP\Files\File;
use OCP\Share\IShare;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixes:

lib/private/Share20/DefaultShareProvider.php:1232 PhanUndeclaredClassMethod Call to method getName from undeclared class \OC\Share20\IShare (Did you mean interface \Icewind\SMB\IShare)

class AssemblyStreamZsync extends AssemblyStream {

/** @var array */
/** @var IFile */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adresses:

apps/dav/lib/Upload/AssemblyStreamZsync.php:79 PhanNonClassMethodCall Call to method getSize on non-class type array

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will crash loudly with php7.2 latest :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

care to elaborate? because of the typehinting? - that shouldn't be a problem - but we can also get phan to run on php7.2 as targeted version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IljaN waiting on your explanation

$multiStatus = $xml->expect('{DAV:}multistatus', $body);

$result = [];
/** @phan-suppress-next-line PhanNonClassMethodCall */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this suppresses

apps/dav/lib/CardDAV/SyncService.php:203 PhanNonClassMethodCall Call to method getResponses on non-class type void
 apps/dav/lib/CardDAV/SyncService.php:207 PhanNonClassMethodCall Call to method getSyncToken on non-class type void

would require this commit to be fixed sabre-io/xml@3bac738#diff-de76feda74829916defbb14d089c7c08

$result[$response->getHref()] = $response->getResponseProperties();
}

/** @phan-suppress-next-line PhanNonClassMethodCall */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DeepDiver1975 DeepDiver1975 merged commit 9debccb into master Jun 21, 2018
@DeepDiver1975 DeepDiver1975 deleted the experimental-phan branch June 21, 2018 05:33
@PVince81 PVince81 mentioned this pull request Jun 27, 2018
4 tasks
@PVince81
Copy link
Contributor

can we backport this or will it have too big a hit ? @DeepDiver1975 @patrickjahns

@patrickjahns
Copy link
Contributor Author

@PVince81
From my POV we should backport this - changes are currently minimal, as this is on the most lenient setting.

Over-time we should gradually increase the strictness of php-phan to have a cleaner + more-stable code base

@PVince81
Copy link
Contributor

PVince81 commented Jul 31, 2018

Agreed. @patrickjahns will you take care (as you are the original submitter) or should I ?

@phil-davis
Copy link
Contributor

@patrickjahns @PVince81 backport is still outstanding.
Before or after 10.0.10?

@patrickjahns
Copy link
Contributor Author

I'd love to get it in - but then would need to be done by tonight/tomorrow

@PVince81
Copy link
Contributor

I'll backport this

@PVince81
Copy link
Contributor

stable10: #32492

@lock lock bot locked as resolved and limited conversation to collaborators Aug 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants