Disallow float assignment, property type, parameter type, return type
#1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akrabat
reviewed
Jan 8, 2019
Ocramius
changed the title
float assignment, property type, parameter type, return typefloat assignment, property type, parameter type, return type
|
I think it should remain on 7.1 - so that all users that can currently use
PHPStan can also use this package. I get the argument on why require the
latest version etc., but TBH there’s not much compelling stuff in 7.2 and
7.3 for more comfortable development of libraries. (7.4 will be a different
story).
On Tue, 8 Jan 2019 at 09:24, Marco Pivetta ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In .gitlab-ci.yml
<#1 (comment)>:
> @@ -0,0 +1,24 @@
+image: php:7.1-cli-alpine
This is being removed anyway, since CI will be deferred to other tooling
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGZuAvp76tgUsyoIZlpvEDGvpFSsKrDks5vBFWlgaJpZM4Z0913>
.
--
Ondřej Mirtes
|
|
I'm generally going to restrict this to the minimum possible scope: this is for a customer project first, and secondarily for a larger audience. |
… chars per line limit
…s `int` or `mixed` Note that `mixed` is the top type, so it also includes `float`: we can't do much about that, since a lot of PHP code relies on it.
This verifies an edge case that may happen due to incomplete parsing, or incorrect AST in general being fed to the rule.
…ction signature
This fixes an escaped mutation, specifically:
```
1) /home/ocramius/Documents/roave/no-floaters/src/DisallowFloatInFunctionSignatureRule.php:62 [M] Continue_
--- Original
+++ New
@@ @@
$errors[] = sprintf('Parameter #%d $%s of function %s() cannot have %s as its type - floats are not allowed.', $i + 1, $parameter->getName(), $functionReflection->getName(), $parameter->getType()->describe(VerbosityLevel::typeOnly()));
}
if (!FloatTypeHelper::isFloat($functionVariant->getReturnType())) {
- continue;
+ break;
}
$errors[] = sprintf('Function %s() cannot have %s as its return type - floats are not allowed.', $functionReflection->getName(), $functionVariant->getReturnType()->describe(VerbosityLevel::typeOnly()));
}
```
…hod signature
This fixes an escaped mutation, specifically:
```
2) /home/ocramius/Documents/roave/no-floaters/src/DisallowFloatInMethodSignatureRule.php:41 [M] Continue_
--- Original
+++ New
@@ @@
foreach ($method->getVariants() as $methodVariant) {
foreach ($methodVariant->getParameters() as $i => $parameter) {
if (!FloatTypeHelper::isFloat($parameter->getType())) {
- continue;
+ break;
}
$errors[] = sprintf('Parameter #%d $%s of method %s::%s() cannot have %s as its type - floats are not allowed.', $i + 1, $parameter->getName(), $method->getDeclaringClass()->getDisplayName(), $method->getName(), $parameter->getType()->describe(VerbosityLevel::typeOnly()));
}
```
…sed by `break`/`continue` substitution While the code is technically correct, verifying all variations of a method is not really viable: modifying the implementation to avoid looping quirks in first place is safer.
…d by `break`/`continue` substitution While the code is technically correct, verifying all variations of a method is not really viable: modifying the implementation to avoid looping quirks in first place is safer.
…or class-related AST node)
… dependencies are included
…plicable The library is only supposed to be used as a set of imported rules, and nothing else. Inheritance, as well as importing support classes, is not supposed to be done by consumers.
Ocramius
changed the title
float assignment, property type, parameter type, return typefloat assignment, property type, parameter type, return type
|
Note: build failure is expected on |
malukenho
reviewed
Jan 8, 2019
Ocramius
commented
Jan 8, 2019
|
amazing work - we'd have a use for this. probably not? |
|
I think it should work, try it 😊
On Tue, 8 Jan 2019 at 17:00, Ben Davies ***@***.***> wrote:
amazing work - we'd have a use for this.
Can you account for cases with the result of an operation will result in a
float?
https://3v4l.org/9h7mn
probably not?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGZuKUkCB_dMUzM9ZjnwK6a8yGLrScQks5vBMCEgaJpZM4Z0913>
.
--
Ondřej Mirtes
|
|
@bendavies your example with @phpstan and these rules applied leads to following: |
😍 |
|
For the record, the second and the third error is from phpstan/phpstan-strict-rules, not plain PHPStan :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This WIP patch disallows
floaton:TODO for @Ocramius:
@internalto prevent usage in downstream consumersroave/backwards-compatibility-checkto CIdoctrine/coding-standardCS checkNote: initial author of this is @ondrejmirtes (commissioned by @Ocramius), so this patch is a takeover of the PoC.