Add new phan rule PhanMismatchReturn #4914

johnsaigle · 2019-06-27T19:26:48Z

Brief summary of changes

This PR adds the new phan rule PhanMismatchReturn which flags code that is declared to return a type but in reality returns a different type. e.g.

/**
* @return int
*/
function returnInt() {
    return 'hello';
}

Turns out we do this all over the place in LORIS so this PR is pretty big.

A lot of phan errors came up related to #4308 so this PR also fixes a lot of those.

There are also minor optimizations/rewrites of functions that were needlessly complicated. In pretty much every case the replacements should be logically equivalent and hopefully more maintainable.

johnsaigle · 2019-06-27T19:27:29Z

htdocs/survey.php

@@ -372,7 +372,7 @@ function display()
            $this->tpl_data['complete']  = true;

            $this->updateStatus('Complete');
-            $Responses      = $DB->update(


unused. same with below.

johnsaigle · 2019-06-27T19:28:18Z

modules/candidate_list/php/candidate_list.class.inc

@@ -152,8 +152,6 @@ class Candidate_List extends \NDB_Menu_Filter
                               'participantstatus' => $participant_status_options,
                               'useedc'            => $config->getSetting("useEDC"),
                              ];
-
-        return true;


When you see this line deleted in this PR in other files, it typically means that the function should actually return void.

johnsaigle · 2019-06-27T19:29:57Z

modules/create_timepoint/php/create_timepoint.class.inc

@@ -208,7 +208,9 @@ class Create_Timepoint extends \NDB_Form
                        WHERE CenterID =:cid",
                        array('cid' => $siteID)
                    );
-                    $psc_labelOptions[$siteID] = $center['Name'];
+                    if (!is_null($center)) {


Another theme in this PR is verifying that DB results are not null. Since the pselect* family does not have any return types it is possible for them to return null instead of the expected array of data. This causes phan to get upset when we try to access an array key without first ensuring that the DB result is not null.

johnsaigle · 2019-06-27T19:30:32Z

modules/data_release/ajax/FileUpload.php

@@ -43,7 +43,7 @@
    } else {
        $target_path = $base_path . $fileName;
        if (move_uploaded_file($_FILES["file"]["tmp_name"], $target_path)) {
-            $success = $DB->insert(


insert + update always return void so they should not be assigned to a value.

driusan · 2019-07-02T13:05:07Z

This should go to major, not the 21.0-release branch at this point.

johnsaigle · 2019-07-02T16:28:21Z

Has 21 been merged back into major? I'll mark this as Needs Rebase for now but I think changing the base branch right now will make all the changes meaningless.

johnsaigle · 2019-07-10T16:31:24Z

src/Middleware/MiddlewareChainerMixin.php:46 PhanTypeMismatchDeclaredReturn Doc-block of withMiddleware contains declared return type \LORIS\Middleware\MiddlewareChainerMixin which is incompatible with the return type \LORIS\Middleware\MiddlewareChainer declared in the signature
src/Middleware/MiddlewareChainerMixin.php:62 PhanTypeMismatchReturn Returning type \LORIS\Middleware\MiddlewareChainerMixin but withMiddleware() is declared to return \LORIS\Middleware\MiddlewareChainer
test/unittests/NDB_BVL_FeedbackTest.php:73 PhanTypeMismatchReturn Returning type \PHPUnit_Extensions_Database_DataSet_CompositeDataSet but getDataSet() is declared to return \PHPUnit\DbUnit\DataSet\IDataSet

These are the remaining phan errors after this rule is applied. I'm not exactly sure how to solve these ones so any help would be appreciated. Doing a straightforward change on the return types listed above just creates another kind of conflict.

Pinging @driusan because I think I'll need your help here

johnsaigle · 2019-07-10T17:33:17Z

modules/server_processes_manager/php/abstractserverprocess.class.inc

@@ -47,79 +47,79 @@ abstract class AbstractServerProcess
    /**
     * ID of the process when recorded in the database.
     *
-     * @var int.
+     * @var int


Newer versions of phan don't like the . after the type

johnsaigle · 2019-07-17T14:58:40Z

The only remaining phan issue (i.e. what is causing Travis to fail) is caused by a reference to DbUnit which is removed by #4433.

Until it's merged, this PR will be marked as Blocked.

johnsaigle · 2019-07-30T18:56:05Z

php/libraries/NDB_Factory.class.inc:112 PhanUndeclaredFunction Call to undeclared function \MockNDB_Config()
php/libraries/NDB_Factory.class.inc:147 PhanUndeclaredFunction Call to undeclared function \MockUser()

EDIT:

php/libraries/NDB_Factory.class.inc:122 PhanTypeMismatchReturn Returning type \MockNDB_Config but config() is declared to return \NDB_Config

@driusan These are my remaining phan errors. Phan doesn't seem to understand what we're doing were with PHPUnit.

I tried using the PHPUnit extensions for phan -- no change.

I also tried using Mock::generate(CLASS) but that doesn't work with our tests.

Any thoughts?

driusan · 2019-07-30T19:00:33Z

Are you sure those line numbers are right? In NDB_Factory for those lines I see:

line 112: return self::$config
line 147: */

The "MockUser" and "MockNDB_Config" references near those lines both have the new keyword before them, so I would expect phan to be smart enough to at least realize they aren't function calls.

johnsaigle · 2019-07-30T19:08:45Z

Yes, sorry I reverted wrong. My issue is this:

php/libraries/NDB_Factory.class.inc:122 PhanTypeMismatchReturn Returning type \MockNDB_Config but config() is declared to return \NDB_Config

If I add \MockNDB_Config to the @return it gives an error about this class not existing, e.g.

php/libraries/NDB_Factory.class.inc:107 PhanTypeMismatchDeclaredReturn Doc-block of config contains declared return type \MockNDB_Config which is incompatible with the return type \NDB_Config declared in the signature

php/libraries/NDB_Factory.class.inc:109 PhanUndeclaredTypeReturnType Return type of config() is undeclared type \MockNDB_Config

driusan · 2019-07-30T19:28:33Z

I don't think we'll ever be able to fix that since from what I can tell PHPUnit creates the mock classes using eval. https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/MockObject/MockClass.php#L42 .. the mock objects are dynamically generated code, they're not very amenable to static analysis.

Since PHP type hinting doesn't support sum types while doc comments do, our options are either:

Tell phan to ignore those 2 lines and add a comment explaining why, since we know that it's an instance of NDB_Config even if phan doesn't.
Remove the type hinting from the signature but leave it in the docblock, so that phan can still analyze the types but doesn't flag the mismatch as an error.

Short of refactoring the code, I think this is one of the rare cases where I'd prefer 1, but I have nothing against 2 either.

johnsaigle · 2019-07-30T19:33:12Z

I'd prefer approach 1) too. I'll add the suppression and a note explaining it

johnsaigle · 2019-08-05T18:59:07Z

I'm getting similar issues with:

php/libraries/NDB_Factory.class.inc:112 PhanTypeMismatchReturn Returning type null but config() is declared to return \NDB_Config
3926php/libraries/NDB_Factory.class.inc:157 PhanTypeMismatchReturn Returning type null but user() is declared to return \User

I'm going to also add suppressions for these because they're the same species of issue.

PapillonMcGill · 2019-08-26T13:31:00Z

modules/document_repository/php/document_repository.class.inc

                            " where id=$parent_id";
-                $value        = $DB->pselectRow($query, array());
+                $value     = $DB->pselectRow($query, array());


should use label instead of direct value in query.

modules/help_editor/php/helpfile.class.inc

modules/server_processes_manager/php/serverprocesslauncher.class.inc

driusan · 2019-08-26T19:44:20Z

src/Api/Endpoints/Project/Images.php

@@ -99,7 +99,7 @@ public function handle(ServerRequestInterface $request) : ResponseInterface
     *
     * @param ServerRequestInterface $request The incoming PSR7 request
     *
-     * @return array
+     * @return array|\Psr\Http\Message\ResponseInterface


This doesn't make sense to me. Why would a toArray function return a PSR ResponseInterface?

Phan wants this because of the Exception case that returns a 400 response.

Okay.. it's still a code smell, but it's fine for this PR since it's just documenting what's already there

johnsaigle commented Jun 27, 2019

View reviewed changes

johnsaigle added State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) State: Needs work PR awaiting additional work by the author to proceed labels Jul 2, 2019

johnsaigle added this to the 22.0.0 milestone Jul 3, 2019

johnsaigle mentioned this pull request Jul 10, 2019

[Test] Add phpstan tool to test suite and fix detected errors. #4928

Merged

johnsaigle commented Jul 10, 2019

View reviewed changes

johnsaigle changed the base branch from 21.0-release to major July 17, 2019 15:51

johnsaigle mentioned this pull request Jul 24, 2019

[Testing] Remove DB Unit package #4985

Merged

johnsaigle added [branch] major and removed State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed [branch] major labels Jul 24, 2019

johnsaigle changed the base branch from major to minor July 24, 2019 21:38

johnsaigle added the State: Needs work PR awaiting additional work by the author to proceed label Jul 29, 2019

Revert properly

47a19a2

johnsaigle added 4 commits July 30, 2019 15:52

Add one tiny suppression don't _worry_ about it

d94e433

Revert more

5463c40

See if this fixes travis..

6804a22

phpcbf

4656150

johnsaigle added 5 commits August 5, 2019 15:06

Add more suppressions

8624fb2

Remove early return cause Travis is failing

9c81639

add null check in config()

a32726d

Modify config loading

8d8eb6f

hopefully fix phan/phpcs errors

5e8f628

ridz1208 removed the [branch] minor label Aug 15, 2019

Fix bad UserPerms change

95e99c3

johnsaigle removed the State: Needs work PR awaiting additional work by the author to proceed label Aug 19, 2019

johnsaigle requested a review from driusan August 19, 2019 22:07

PapillonMcGill suggested changes Aug 26, 2019

View reviewed changes

driusan reviewed Aug 26, 2019

View reviewed changes

Remove unneccessary variable in helpfile

dc02e62

PapillonMcGill approved these changes Sep 9, 2019

View reviewed changes

johnsaigle requested review from driusan and ridz1208 September 11, 2019 18:17

driusan merged commit 0dacece into aces:minor Sep 18, 2019

johnsaigle mentioned this pull request Sep 18, 2019

toArray() function in Images.php under API should not return a ResponseInterface #5232

Closed

sruthymathew123 mentioned this pull request Nov 5, 2019

[Examiner] Fixing Errors while saving the form #5545

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new phan rule PhanMismatchReturn #4914

Add new phan rule PhanMismatchReturn #4914

johnsaigle commented Jun 27, 2019 •

edited

Loading

johnsaigle Jun 27, 2019

johnsaigle Jun 27, 2019

johnsaigle Jun 27, 2019

johnsaigle Jun 27, 2019 •

edited

Loading

driusan commented Jul 2, 2019

johnsaigle commented Jul 2, 2019

johnsaigle commented Jul 10, 2019 •

edited

Loading

johnsaigle Jul 10, 2019

johnsaigle commented Jul 17, 2019

johnsaigle commented Jul 30, 2019 •

edited

Loading

driusan commented Jul 30, 2019

johnsaigle commented Jul 30, 2019 •

edited

Loading

driusan commented Jul 30, 2019

johnsaigle commented Jul 30, 2019

johnsaigle commented Aug 5, 2019 •

edited

Loading

PapillonMcGill Aug 26, 2019

driusan Aug 26, 2019

johnsaigle Sep 9, 2019

driusan Sep 18, 2019

Add new phan rule PhanMismatchReturn #4914

Add new phan rule PhanMismatchReturn #4914

Conversation

johnsaigle commented Jun 27, 2019 • edited Loading

Brief summary of changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnsaigle Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

driusan commented Jul 2, 2019

johnsaigle commented Jul 2, 2019

johnsaigle commented Jul 10, 2019 • edited Loading

Choose a reason for hiding this comment

johnsaigle commented Jul 17, 2019

johnsaigle commented Jul 30, 2019 • edited Loading

driusan commented Jul 30, 2019

johnsaigle commented Jul 30, 2019 • edited Loading

driusan commented Jul 30, 2019

johnsaigle commented Jul 30, 2019

johnsaigle commented Aug 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnsaigle commented Jun 27, 2019 •

edited

Loading

johnsaigle Jun 27, 2019 •

edited

Loading

johnsaigle commented Jul 10, 2019 •

edited

Loading

johnsaigle commented Jul 30, 2019 •

edited

Loading

johnsaigle commented Jul 30, 2019 •

edited

Loading

johnsaigle commented Aug 5, 2019 •

edited

Loading