-
Notifications
You must be signed in to change notification settings - Fork 660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
glob wrong return type #9820
Comments
I found these snippets: https://psalm.dev/r/726b2ba5b5<?php
$result = glob( '' );
/** @psalm-trace $result */;
|
Wrong, i didn't touch the return type the Bug was added by your PR first YOU need to make a Type Provider that still allows for empty string pattern |
The return type should be |
There is nothing related to the PR you linked, since
It's kinda tiring to see you crying about bug on three different thread with wrong understanding of bug and feature when you took bad shortcuts though. Feel free to add a returntype provider if you need one, but currently the return type is perfectly valid. |
I found these snippets: https://psalm.dev/r/422a56fc89<?php
$result = glob( 'a' );
/** @psalm-trace $result */;
|
The "false" is not the issue
In case of an empty string glob we can infer that the array is always empty (if there is no |
I found these snippets: https://psalm.dev/r/422a56fc89<?php
$result = glob( 'a' );
/** @psalm-trace $result */;
|
are you 100% sure that and yeah, you need to check for |
This is what I just said a minute before you comment #9820 (comment) |
a little bit sandboxing:
|
So it's an improvement => feature. There is no bug currently. Therefore was just unnecessary false accusation. |
Yes, this should be handled in the return type provider if you want to take it exactly. Honestly, a simple return type provider without checking the |
Yes, it's a bug: |
I found these snippets: https://psalm.dev/r/4e94df6823<?php
$result = glob( '' );
if ( $result !== false ) {
$result[] = rand( 0, 5 );
/** @psalm-trace $result */;
}
|
The special-case return types are a simple limitation of the callmap system. The most recent change seems to align with the input allowed by php. The callmaps do sometimes ignore very unlikely output (and maybe input), but not sure this falls into that category. |
Hey guys, calm down a bit! It's an old debate whether it's better to create a few false positive to prevent a lot of errors. However, if the false positives are enough to make user raise issue in the few days following a change, we might have gone a bit too far. So I'd rather go back to the initial state where Psalm does not raise errors that will confuse people first. If we can improve on that, even better Please don't fight about this (frankly, I don't have time to read pages of debate to extract the bits of relevant information) |
How about adding Considering PHP-Versions 8.2.0 - 8.2.6, 7.4.33, 7.0.0 - 7.0.33, 7.1.0 - 7.1.33, 7.2.0 - 7.2.34, 7.3.0 - 7.3.33, 7.4.0 - 7.4.32, 8.0.0 - 8.0.28, 8.1.0 - 8.1.19 I would propose: https://psalm.dev/r/258bab2644 (note that With that stub and the fix for the regression (see https://psalm.dev/r/1db5aa2914) the callmap should look like:
Is that a definition everyone agrees on? I personally must say, that I do not agree with the assumption, that there are no empty pathnames. On some exotic system there might be a
Until we all agree on a better type, I would kindly ask @orklah to revert the PR #9752 (and the improvement PR #9814 which did fixes some issues with the newly introduced typing but not all). @kkmuffme mind creating a PR with the stub typing suggested (given you agree with that typing). Best would bea PR with the second version of the stub, because that is less strict and IMO better less strict than wrong. We could vote in that PR for which version to choose, either accepting there might be empty-strings or assuming that will never happen. |
I found these snippets: https://psalm.dev/r/258bab2644<?php
/**
* @psalm-template P of string
* @psalm-template F of int-mask<GLOB_MARK, GLOB_NOSORT, GLOB_NOCHECK, GLOB_NOESCAPE, GLOB_BRACE, GLOB_ONLYDIR, GLOB_ERR>
* @psalm-param P $_pattern
* @psalm-param F $_flags
* @psalm-return (
* P is ''
* ? (F is int-mask<GLOB_MARK, GLOB_NOSORT, GLOB_NOESCAPE, GLOB_BRACE, GLOB_ONLYDIR, GLOB_ERR>
* ? false|list<never>
* : false|list{0:''}
* )
* : (P is non-empty-string
* ? (F is int-mask<GLOB_MARK, GLOB_NOSORT, GLOB_NOESCAPE, GLOB_BRACE, GLOB_ONLYDIR, GLOB_ERR>
* ? false|list<non-empty-string>
* : false|list{0:non-empty-string, ...<non-empty-string>}
* )
* : (F is int-mask<GLOB_MARK, GLOB_NOSORT, GLOB_NOESCAPE, GLOB_BRACE, GLOB_ONLYDIR, GLOB_ERR>
* ? false|list{0?:string, ...<non-empty-string>}
* : false|list{0:string, ...<non-empty-string>}
* )
* )
* )
*
*/
function glob_ (string $_pattern, int $_flags = 0): array|false {
return false;
}
/** @var int-mask<GLOB_NOCHECK> */
$_maybeFlag = 0;
$_emptyNoFlag = glob_( '' );
$_emptyNoCheckFlag1 = glob_( '', GLOB_MARK );
$_emptyNoCheckFlag2 = glob_( '' , GLOB_NOSORT | GLOB_NOESCAPE);
$_emptyNoCheckFlag3 = glob_( '' , GLOB_BRACE | GLOB_ONLYDIR | GLOB_ERR);
$_emptyCheckFlag1 = glob_( '' , GLOB_NOCHECK);
$_emptyCheckFlag2 = glob_( '' , GLOB_NOCHECK | GLOB_MARK);
$_emptyCheckFlag3 = glob_( '' , GLOB_NOCHECK | GLOB_BRACE | GLOB_ONLYDIR | GLOB_ERR | GLOB_NOSORT | GLOB_NOESCAPE);
$_emptyMaybeCheckFlag = glob_( '' , $_maybeFlag);
$_nonEmptyNoFlag = glob_( 'a' );
$_nonEmptyNoCheckFlag1 = glob_( 'a' , GLOB_MARK );
$_nonEmptyNoCheckFlag2 = glob_( 'a' , GLOB_NOSORT | GLOB_NOESCAPE);
$_nonEmptyNoCheckFlag3 = glob_( 'a' , GLOB_BRACE | GLOB_ONLYDIR | GLOB_ERR);
$_nonEmptyCheckFlag1 = glob_( 'a', GLOB_NOCHECK);
$_nonEmptyCheckFlag2 = glob_( 'a' , GLOB_NOCHECK | GLOB_MARK);
$_nonEmptyCheckFlag3 = glob_( 'a' , GLOB_NOCHECK | GLOB_BRACE | GLOB_ONLYDIR | GLOB_ERR | GLOB_NOSORT | GLOB_NOESCAPE);
$_nonEmptyMaybeCheckFlag = glob_( 'a' , $_maybeFlag);
/** @var string */
$string = '';
$_stringNoFlag = glob_( $string );
$_stringNoCheckFlag1 = glob_( $string , GLOB_MARK );
$_stringNoCheckFlag2 = glob_( $string , GLOB_NOSORT | GLOB_NOESCAPE);
$_stringNoCheckFlag3 = glob_( $string , GLOB_BRACE | GLOB_ONLYDIR | GLOB_ERR);
$_stringCheckFlag1 = glob_( $string , GLOB_NOCHECK);
$_stringCheckFlag2 = glob_( $string , GLOB_NOCHECK | GLOB_MARK);
$_stringCheckFlag3 = glob_( $string , GLOB_NOCHECK | GLOB_BRACE | GLOB_ONLYDIR | GLOB_ERR | GLOB_NOSORT | GLOB_NOESCAPE);
$_stringMaybeCheckFlag = glob_( $string , $_maybeFlag);
/** @psalm-trace
$_emptyNoFlag $_emptyNoCheckFlag1 $_emptyNoCheckFlag2 $_emptyNoCheckFlag3 $_emptyCheckFlag1 $_emptyCheckFlag2 $_emptyCheckFlag3 $_emptyMaybeCheckFlag
$_nonEmptyNoFlag $_nonEmptyNoCheckFlag1 $_nonEmptyNoCheckFlag2 $_nonEmptyNoCheckFlag3 $_nonEmptyCheckFlag1 $_nonEmptyCheckFlag2 $_nonEmptyCheckFlag3 $_nonEmptyMaybeCheckFlag
$_stringNoFlag $_stringNoCheckFlag1 $_stringNoCheckFlag2 $_stringNoCheckFlag3 $_stringCheckFlag1 $_stringCheckFlag2 $_stringCheckFlag3 $_stringMaybeCheckFlag */
https://psalm.dev/r/1db5aa2914<?php
glob('', 0);
https://psalm.dev/r/ce62bf2569<?php
/**
* @psalm-template P of string
* @psalm-template F of int-mask<GLOB_MARK, GLOB_NOSORT, GLOB_NOCHECK, GLOB_NOESCAPE, GLOB_BRACE, GLOB_ONLYDIR, GLOB_ERR>
* @psalm-param P $_pattern
* @psalm-param F $_flags
* @psalm-return (
* P is ''
* ? (F is int-mask<GLOB_MARK, GLOB_NOSORT, GLOB_NOESCAPE, GLOB_BRACE, GLOB_ONLYDIR, GLOB_ERR>
* ? false|list{0?:''}
* : false|list{0:''}
* )
* : (F is int-mask<GLOB_MARK, GLOB_NOSORT, GLOB_NOESCAPE, GLOB_BRACE, GLOB_ONLYDIR, GLOB_ERR>
* ? false|list<string>
* : false|non-empty-list<string>
* )
* )
*
*/
function glob_ (string $_pattern, int $_flags = 0): array|false {
return false;
}
/** @var int-mask<GLOB_NOCHECK> */
$_maybeFlag = 0;
$_emptyNoFlag = glob_( '' );
$_emptyNoCheckFlag1 = glob_( '', GLOB_MARK );
$_emptyNoCheckFlag2 = glob_( '' , GLOB_NOSORT | GLOB_NOESCAPE);
$_emptyNoCheckFlag3 = glob_( '' , GLOB_BRACE | GLOB_ONLYDIR | GLOB_ERR);
$_emptyCheckFlag1 = glob_( '' , GLOB_NOCHECK);
$_emptyCheckFlag2 = glob_( '' , GLOB_NOCHECK | GLOB_MARK);
$_emptyCheckFlag3 = glob_( '' , GLOB_NOCHECK | GLOB_BRACE | GLOB_ONLYDIR | GLOB_ERR | GLOB_NOSORT | GLOB_NOESCAPE);
$_emptyMaybeCheckFlag = glob_( '' , $_maybeFlag);
$_nonEmptyNoFlag = glob_( 'a' );
$_nonEmptyNoCheckFlag1 = glob_( 'a' , GLOB_MARK );
$_nonEmptyNoCheckFlag2 = glob_( 'a' , GLOB_NOSORT | GLOB_NOESCAPE);
$_nonEmptyNoCheckFlag3 = glob_( 'a' , GLOB_BRACE | GLOB_ONLYDIR | GLOB_ERR);
$_nonEmptyCheckFlag1 = glob_( 'a', GLOB_NOCHECK);
$_nonEmptyCheckFlag2 = glob_( 'a' , GLOB_NOCHECK | GLOB_MARK);
$_nonEmptyCheckFlag3 = glob_( 'a' , GLOB_NOCHECK | GLOB_BRACE | GLOB_ONLYDIR | GLOB_ERR | GLOB_NOSORT | GLOB_NOESCAPE);
$_nonEmptyMaybeCheckFlag = glob_( 'a' , $_maybeFlag);
/** @var string */
$string = '';
$_stringNoFlag = glob_( $string );
$_stringNoCheckFlag1 = glob_( $string , GLOB_MARK );
$_stringNoCheckFlag2 = glob_( $string , GLOB_NOSORT | GLOB_NOESCAPE);
$_stringNoCheckFlag3 = glob_( $string , GLOB_BRACE | GLOB_ONLYDIR | GLOB_ERR);
$_stringCheckFlag1 = glob_( $string , GLOB_NOCHECK);
$_stringCheckFlag2 = glob_( $string , GLOB_NOCHECK | GLOB_MARK);
$_stringCheckFlag3 = glob_( $string , GLOB_NOCHECK | GLOB_BRACE | GLOB_ONLYDIR | GLOB_ERR | GLOB_NOSORT | GLOB_NOESCAPE);
$_stringMaybeCheckFlag = glob_( $string , $_maybeFlag);
/** @psalm-trace
$_emptyNoFlag $_emptyNoCheckFlag1 $_emptyNoCheckFlag2 $_emptyNoCheckFlag3 $_emptyCheckFlag1 $_emptyCheckFlag2 $_emptyCheckFlag3 $_emptyMaybeCheckFlag
$_nonEmptyNoFlag $_nonEmptyNoCheckFlag1 $_nonEmptyNoCheckFlag2 $_nonEmptyNoCheckFlag3 $_nonEmptyCheckFlag1 $_nonEmptyCheckFlag2 $_nonEmptyCheckFlag3 $_nonEmptyMaybeCheckFlag
$_stringNoFlag $_stringNoCheckFlag1 $_stringNoCheckFlag2 $_stringNoCheckFlag3 $_stringCheckFlag1 $_stringCheckFlag2 $_stringCheckFlag3 $_stringMaybeCheckFlag */
|
Actually the assumption of |
There can't be, bc if you check php-src, it will immediately return if the pattern is empty. So even if there were a file system that had this, PHP glob wouldn't be able to find the empty files. |
Sorry, I did not read all of that. Thanks for the summary @ygottschalk Could someone make a PR with whatever you agree on so we have at least the good things? Then if someone is willing, another PR to get to the last mile to get optimal results is welcome :) |
What about using the pattern But as I said, idk if there exists that kind of weird system. I am fine with not considering this valid, but I wanted to point out this possibility, so someone with more knowledge on that topic than me could step in and share his/her wisdom.
Why? The callmap type I provided is a fallback for the stub (see psalm.dev links), which checks for pattern beeing |
some local docker tests:
i don't know what flag3 makes it different from the other ones to make it return empty array instead? |
Would guess that is the one. Edit: Or it may be |
my current testing shows exactly that:
|
Current version in PR does assume that:
Current version does not respect |
i don't know if both flags collide: i even tried to read from a directory that doesn't exist (inside a broken symbolic link) |
@orklah If you can live with the made assumptions this is ready to merge, else please provide feedback so I can build up on that |
https://psalm.dev/r/726b2ba5b5
Result is not
list<non-empty-string>
when the argument is empty string and no flags are present. (but should bearray<never, never>
)Glob needs a return type provider added to ensure the "non-empty-string" is not lost when a valid pattern is passed.
Bug added in #9814 @Hanmac
The text was updated successfully, but these errors were encountered: