search

brick / math

Arbitrary-precision arithmetic library for PHP
MIT License
1.78k stars 75 forks source link

More static analysis (Psalm related changes) #43

Closed alexeyshockov closed 3 years ago

alexeyshockov commented 4 years ago

Fixes #41

alexeyshockov commented 3 years ago

@BenMorel, could you take a look at the coverage report? It says that the coverage decreased dramatically for NativeCalculator, but there are only few lines changed there...

BenMorel commented 3 years ago

@alexeyshockov regarding coverage, I've already contacted Coveralls for similar problems, and am waiting for their response. I wouldn't be too picky about coverage right now.

BenMorel commented 3 years ago

@alexeyshockov What's the point of using numeric-string everywhere, exactly? Does Psalm complain when using a generic string with GMP or BCMath?

According to the docs, numeric-string represents any string that passes the is_numeric() check, which is not restrictive enough for this project anyway, as we're only dealing with strings of digits with optional minus sign, while is_numeric() accepts other formats, like exponential notation.

BenMorel commented 3 years ago

Hi @alexeyshockov, any update on the points above?

bdsl commented 3 years ago

@BenMorel I tried a search and replace of 'numeric-string' to 'string' to see if Psalm complains. It does, 23 times.

According to psalm the bc functions need numeric-strings, as do the php *, +, - operators. I think it's fine to use it. It's not as restrictive as we might like but that's unavoidable. The string type is even less restrictive. Type systems rarely enforce all necessary restrictions.

BenMorel commented 3 years ago

@bdsl Thank you for your comments. @alexeyshockov Ping!

BenMorel commented 3 years ago

I upgraded Psalm to 4.1 and fixed all the Psalm issues in cbdd9605fcfc593b42f2b82735d39f27fc562bb6.

@alexeyshockov I did not use numeric-string anywhere, I preferred to suppress the error in BcMathCalculator, rather than letting numeric-string bubble up and create > 100 errors everywhere else in the code.

We have a strong requirement on the format of the internal numeric strings used in brick/math, something that cannot be enforced strictly enough by Psalm anyway, so I see little value in adding @psalm- specific annotations everywhere for something that brings next to no value.

Please feel free to open a new PR if you see something that can be improved!