Arithmetic operation between numeric and int or float gives int|float #6167
Conversation
|
Build is failing because Psalm don't like adding I kinda disagree with Psalm here, but it may just be because the flag is too strict for my liking. If so, the fix could be upstream by adding a float cast here: https://github.com/azjezz/psl/blob/1e1831efb36ec992947344d90a135e58aaf4cad9/src/Psl/Math/mean.php#L23 What do you think? |
|
I don't think adding a cast is nesscary, psalm should understand that.
does the issue persist if the type of $numbers changed to |
|
Psalm does not like Psalm understand what's going on, but strictBinaryOperands is really strict and prevent you from adding something that may be an int to a float. However, it's not what the documentation (or even the original feature request) describe: https://psalm.dev/docs/running_psalm/configuration/#strictbinaryoperands Not sure if the documentation is not up to date and the feature is intentional or if the documentation is on point and the implementation is lacking... |
|
Hm, i think we can keep the signature as is and cast $number to float then? you can go ahead an open a PR in https://github.com/azjezz/psl if you would like, or i will do so tomorrow morning :) |
It is. #24 says:
Personally I don't find the rationale given there very convincing. Yes, you shouldn't be adding dollars to cents, but you shouldn't be using float for money at all, and you shouldn't add float lbs to float kgs either. Value objects bundling value and unit are much better fit for that. As such I don't see any issue with arithmetic operations on mixed int/float operands, as long as the result type is inferred as float. |
Wow, somehow I missed that. Thanks
Yeah, value objects are the best, but for projects that didn't use them for the beginning, this config can make sense. Seems a lot of work for a small gain. I'll propose a PR to PSL to fix the issues and I suggest to leave it like that, unless there are complains down the road (but I don't expect a lot for an optional strict config) |
orklah
force-pushed
the
non-div-with-numeric-and-int
branch
from
3d3c78e to
e65e060
Compare
|
@weirdan the issue has been fixed in PSL, and we have added few features since 1.6, i recommend updating the branch instead. As for now, Psalm build is failing for PSL, this started happening today ( discovered in azjezz/psl#220 ), and the failing code is not in 1.6, so i suspect a PR made it in, and broke PSL 1.8 branch. I personally would rather psalm used azjezz/psl repository to avoid having to update the e2e repository from time to time, and i will do my best to ensure cases like this are fixed asap, if there's something i can do upstream to get rid of psalm/endtoend-test-psl repository ( maybe a special config file just for psalm E2E test? ), please let me know. |
|
@azjezz I'd like to keep end-to-end repo (but will update it). It's used as a regression test source, and using an active branch for that, a moving target of sorts, removes this advantage. |
I believe it started failing some time ago when Matt landed new template logic in master. At least the error is present in that revision as well and is gone one revision before that: $ git reset --hard acc7ee261c106b4ae91f333ff4a9939b6e858bf3
HEAD is now at acc7ee261 Fix #6066 - introduce more robust system for capturing template constraints (#6072)
$ fg
[1] - continued nvim ./bin/test-with-real-projects.sh
zsh: suspended nvim ./bin/test-with-real-projects.sh
$ ./bin/test-with-real-projects.sh psl
++ dirname ./bin/test-with-real-projects.sh
+ SCRIPT_DIR=./bin
++ readlink -f ./bin/../psalm
+ PSALM=/home/weirdan/src/psalm/psalm/psalm
++ readlink -f ./bin/../build/psalm.phar
+ PSALM_PHAR=/home/weirdan/src/psalm/psalm/build/psalm.phar
+ cd /tmp/
+ rm -Rf testing-with-real-projects
+ mkdir -p testing-with-real-projects
+ cd testing-with-real-projects
+ case $1 in
+ git clone git@github.com:psalm/endtoend-test-psl.git
Cloning into 'endtoend-test-psl'...
remote: Enumerating objects: 7164, done.
remote: Counting objects: 100% (1426/1426), done.
remote: Compressing objects: 100% (680/680), done.
remote: Total 7164 (delta 797), reused 1276 (delta 732), pack-reused 5738
Receiving objects: 100% (7164/7164), 1.35 MiB | 1.92 MiB/s, done.
Resolving deltas: 100% (5030/5030), done.
+ cd endtoend-test-psl
+ git checkout 1.8.x
Branch '1.8.x' set up to track remote branch '1.8.x' from 'origin'.
Switched to a new branch '1.8.x'
+ composer install --ignore-platform-reqs
No composer.lock file present. Updating dependencies to latest instead of installing from lock file. See https://getcomposer.org/install for more information.
Loading composer repositories with package information
Updating dependencies
Nothing to modify in lock file
Writing lock file
Installing dependencies from lock file (including require-dev)
Nothing to install, update or remove
1 package suggestions were added by new dependencies, use `composer suggest` to see details.
Generating autoload files
+ composer suggests --list
+ xargs composer require --ignore-platform-reqs
Using version ^1.1 for php-standard-library/psalm-plugin
./composer.json has been updated
Running composer update php-standard-library/psalm-plugin
Loading composer repositories with package information
Updating dependencies
Lock file operations: 31 installs, 0 updates, 0 removals
- Locking amphp/amp (v2.6.0)
- Locking amphp/byte-stream (v1.8.1)
- Locking composer/package-versions-deprecated (1.11.99.2)
- Locking composer/semver (3.2.5)
- Locking composer/xdebug-handler (2.0.1)
- Locking dnoegel/php-xdg-base-dir (v0.1.1)
- Locking felixfbecker/advanced-json-rpc (v3.2.1)
- Locking felixfbecker/language-server-protocol (1.5.1)
- Locking netresearch/jsonmapper (v4.0.0)
- Locking nikic/php-parser (v4.12.0)
- Locking openlss/lib-array2xml (1.0.0)
- Locking php-standard-library/psalm-plugin (1.1.1)
- Locking phpdocumentor/reflection-common (2.2.0)
- Locking phpdocumentor/reflection-docblock (5.2.2)
- Locking phpdocumentor/type-resolver (1.4.0)
- Locking psr/container (1.1.1)
- Locking psr/log (1.1.4)
- Locking sebastian/diff (4.0.4)
- Locking symfony/console (v5.3.2)
- Locking symfony/deprecation-contracts (v2.4.0)
- Locking symfony/polyfill-ctype (v1.23.0)
- Locking symfony/polyfill-intl-grapheme (v1.23.0)
- Locking symfony/polyfill-intl-normalizer (v1.23.0)
- Locking symfony/polyfill-mbstring (v1.23.0)
- Locking symfony/polyfill-php73 (v1.23.0)
- Locking symfony/polyfill-php80 (v1.23.0)
- Locking symfony/service-contracts (v2.4.0)
- Locking symfony/string (v5.3.3)
- Locking vimeo/psalm (4.8.1)
- Locking webmozart/assert (1.10.0)
- Locking webmozart/path-util (2.3.0)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 31 installs, 0 updates, 0 removals
- Installing composer/package-versions-deprecated (1.11.99.2): Extracting archive
- Installing symfony/polyfill-ctype (v1.23.0): Extracting archive
- Installing webmozart/assert (1.10.0): Extracting archive
- Installing webmozart/path-util (2.3.0): Extracting archive
- Installing symfony/polyfill-php80 (v1.23.0): Extracting archive
- Installing symfony/polyfill-mbstring (v1.23.0): Extracting archive
- Installing symfony/polyfill-intl-normalizer (v1.23.0): Extracting archive
- Installing symfony/polyfill-intl-grapheme (v1.23.0): Extracting archive
- Installing symfony/string (v5.3.3): Extracting archive
- Installing psr/container (1.1.1): Extracting archive
- Installing symfony/service-contracts (v2.4.0): Extracting archive
- Installing symfony/polyfill-php73 (v1.23.0): Extracting archive
- Installing symfony/deprecation-contracts (v2.4.0): Extracting archive
- Installing symfony/console (v5.3.2): Extracting archive
- Installing sebastian/diff (4.0.4): Extracting archive
- Installing openlss/lib-array2xml (1.0.0): Extracting archive
- Installing nikic/php-parser (v4.12.0): Extracting archive
- Installing netresearch/jsonmapper (v4.0.0): Extracting archive
- Installing felixfbecker/language-server-protocol (1.5.1): Extracting archive
- Installing phpdocumentor/reflection-common (2.2.0): Extracting archive
- Installing phpdocumentor/type-resolver (1.4.0): Extracting archive
- Installing phpdocumentor/reflection-docblock (5.2.2): Extracting archive
- Installing felixfbecker/advanced-json-rpc (v3.2.1): Extracting archive
- Installing dnoegel/php-xdg-base-dir (v0.1.1): Extracting archive
- Installing psr/log (1.1.4): Extracting archive
- Installing composer/xdebug-handler (2.0.1): Extracting archive
- Installing composer/semver (3.2.5): Extracting archive
- Installing amphp/amp (v2.6.0): Extracting archive
- Installing amphp/byte-stream (v1.8.1): Extracting archive
- Installing vimeo/psalm (4.8.1): Extracting archive
- Installing php-standard-library/psalm-plugin (1.1.1): Extracting archive
4 package suggestions were added by new dependencies, use `composer suggest` to see details.
Generating autoload files
composer/package-versions-deprecated: Generating version class...
composer/package-versions-deprecated: ...done generating version class
16 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
+ /home/weirdan/src/psalm/psalm/psalm --monochrome --config=tools/psalm/psalm.xml
Scanning files...
Analyzing files...
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 60 / 605 (9%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 120 / 605 (19%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 180 / 605 (29%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 240 / 605 (39%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 300 / 605 (49%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 360 / 605 (59%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 420 / 605 (69%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 480 / 605 (79%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 540 / 605 (89%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 600 / 605 (99%)
░░░░░
ERROR: InvalidArgument - ../../src/Psl/Regex/every_match.php:48:16 - Incompatible types found for T (see https://psalm.dev/004)
return Type\vec($capture_groups)->coerce($matching);
------------------------------
1 errors found
------------------------------
2 other issues found.
You can display them with --show-info=true
------------------------------
Psalm can automatically fix 5 of these issues.
Run Psalm again with
--alter --issues=MissingParamType --dry-run
to see what it can fix.
------------------------------
Checks took 3.49 seconds and used 239.630MB of memory
Psalm was able to infer types for 100.1330% of the codebase |
src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/NonDivArithmeticOpAnalyzer.php
Outdated
Show resolved
Hide resolved
orklah
force-pushed
the
non-div-with-numeric-and-int
branch
from
1b016aa to
55245cf
Compare
|
Thanks! |
This PR refine the result of any non-div arithmetic operation between numeric and int or float. The result is now
int|floatwhile it wasnumericbefore