Fix signature compatibility errors detected by phan #3631
Conversation
This increases the coverage of phan to detect incompatible signatures between parent and child classes. (These are primarily caused by invalid doc comments, where phan extracts the variable types from, or previous ambiguity between how to denote something that returns no value. phan insists on "void", while the codebase had a mixture of "void", "none", or "null".) When there's disagreement between a child and parent class about what the return value should be, this change sides with the parent (which may not always be correct, but can be fixed in a future change if any errors are detected.)
| @@ -101,7 +101,7 @@ class Acknowledgements extends \NDB_Menu_Filter_Form | |||
| * Sets up all the class variables needed for the | |||
| * acknowledgements menu filter | |||
| * | |||
| * @return true on success | |||
| * @return void | |||
There was a problem hiding this comment.
This match the return type definition with parent but in both case, the code always return true. Should be change for @return bool ? while other class derived from the parent return void. Future PR fix?
There was a problem hiding this comment.
You can't change the return type of a child class from the parent class, that's what the signature compatibility checks is checking for.
I can remove the useless return statement from these two functions, but otherwise they'll be caught when "PhanTypeMismatchReturn" is removed from the ignore list. Fixing all the mismatched returns in the codebase is beyond the scope of this PR which is only trying to strengthen the signature compatibility analysis. Adding the PhanTypeMismatchReturn check to this PR would require 269 more changes:
vendor/bin/phan | wc -l
269
There was a problem hiding this comment.
(Fixed these 2 in the latest commit)
There was a problem hiding this comment.
@driusan thanks, I just wanted some clarification.
| @@ -157,7 +157,7 @@ class Acknowledgements extends \NDB_Menu_Filter_Form | |||
| * that are common to every type of page. May be overridden by a specific | |||
| * page or specific page type. | |||
| * | |||
| * @return none | |||
| * @return void | |||
php/libraries/Project.class.inc
Outdated
| @@ -194,23 +194,23 @@ class Project | |||
| * @note If the database value is null then the return value will be the sum of | |||
| * all this project's subproject recruitment target. | |||
| * | |||
| * @return string This project's name | |||
| * @return int This project's name | |||
There was a problem hiding this comment.
Probably the scope of another PR but should not that be "@return in This project's target
There was a problem hiding this comment.
Yes. I mostly ignored the comments/descriptions I noticed that made my head hurt in this PR for the sake of my sanity.
But I just removed this description entirely (other than the type annotation), since it doesn't really add any value that isn't already in the comment/function name.
This increases the coverage of phan to detect incompatible
signatures between parent and child classes. (These are primarily
caused by invalid doc comments, where phan extracts the variable
types from, or previous ambiguity between how to denote something
that returns no value. phan insists on "void", while the codebase
had a mixture of "void", "none", or "null".)
When there's disagreement between a child and parent class about
what the return value should be, this change sides with the parent
(which may not always be correct, but can be fixed in a future
change if any errors are detected.)