Conversation
Codecov Report
@@ Coverage Diff @@
## master #34023 +/- ##
=============================================
- Coverage 64.77% 53.09% -11.68%
=============================================
Files 1198 61 -1137
Lines 69418 7193 -62225
Branches 1276 1276
=============================================
- Hits 44964 3819 -41145
+ Misses 24085 3005 -21080
Partials 369 369
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #34023 +/- ##
============================================
- Coverage 65.68% 64.76% -0.93%
+ Complexity 18735 18348 -387
============================================
Files 1221 1198 -23
Lines 70793 69432 -1361
Branches 1288 1276 -12
============================================
- Hits 46503 44969 -1534
- Misses 23913 24094 +181
+ Partials 377 369 -8
Continue to review full report at Codecov.
|
be76a5d to
bafef21
Compare
| public function updateShares(array $add, array $remove) { | ||
| /** @var CalDavBackend $calDavBackend */ | ||
| $calDavBackend = $this->caldavBackend; | ||
| \assert($calDavBackend instanceof CalDavBackend); |
There was a problem hiding this comment.
phan does not understand (or believe or...) a variable doc like above. So it thinks that $calDavBackend can be just any old BackendInterface and it complains that BackendInterface has no method called updateShares
The "easy" fix is to put this assert() here. Then phan really believes it is a CalDavBackend and so knows about method updateShares.
The question I have is why is the constructor here got BackendInterface $caldavBackend. Maybe it could/should be CalDavBackend $caldavBackend ?
|
|
||
| $view = $this->server->tree->getView(); | ||
| $tree = $this->server->tree; | ||
| \assert($tree instanceof ObjectTree); |
There was a problem hiding this comment.
phan and PHPstorm tell me that $this->server->tree might not have a getView() method.
At run-time if it really does not have a getView() method, then I guess it is already going to explode.
Adding assert() here makes both phan and PHPstorm happy that getView() is going to exist.
As long as ObjectTree is really the only object type that happens here, then this assert() will only die when the line after would die anyway. But if there are other object types that have a getView() and happen here, then not so good.
There was a problem hiding this comment.
At run-time if it really does not have a getView() method, then I guess it is already going to explode.
This implicit assumption is asking for trouble. If Either the code only receives proper objects, or it needs to explicitly protect itself from doing invalid things
if (!$tree instanceof ObjectTree) { throw new \RuntimeException("...") }
d90ea24 to
f5df060
Compare
| */ | ||
| public function addFileFromStream($stream, $internalName, $size) { | ||
| if ($this->streamerInstance instanceof ZipStreamer) { | ||
| /* @phan-suppress-next-line PhanParamTooFew */ |
There was a problem hiding this comment.
phan does not work out that this is a ZipStreamer - it thinks it is a TarStreamer and complains that there must be 3 parameters. (Maybe an assert() will help it?)
|
This is a demo of what can be done to implement https://github.com/phan/phan/wiki/Annotating-Your-Source-Code has hints about putting Perhaps the return types of various methods could be tightened up to stop this problem? Perhaps there are other ways to "fix" this stuff? Or perhaps we ignore Anyway, this is a little demo of what can be done. Discussion @PVince81 @DeepDiver1975 @patrickjahns and anyone else. |
de4c126 to
87c9929
Compare
|
Please also see: #32493 Suggestion for the next iteration would actually be, to pair with one of the core developers and go over these findings and fix them. These changes should not be done lightly. Some of the changes even if they seem innocent, change object types and not all scenarios will be covered by unit tests here. Related to If we are doing code changes related to static code analysis, please include the relevant error the change is fixing to be able to discuss if the change is just hiding an error, or if the change actually solves a deeper issue. Example: |
I agree. For the demonstration changes I made here, I was a bit surprised that I did not break any unit tests. Maybe what I did was fine and only assumed/asserted the correct object types actually used by the code, or maybe the code path(s) were not covered by unit tests, maybe the pigs can help before they fly ;) I certainly did not feel comfortable asserting a particular object type in some of the situations where it was not obvious to me that it would be the only object type to ever be passed through that code path. |
…reamerInstance is instanceof ZipStreamer
87c9929 to
23e815a
Compare
|
See PR #35817 |
Get a snapshot of what the latest
phanversion is reporting