Verify method, function, property, constant and interface changes and report BC breaks #38

Ocramius · 2018-04-15T21:03:05Z

Fixes #7
Fixes #8
Fixes #9
Fixes #10
Fixes #11
Fixes #12
Fixes #41
Fixes #43
Fixes #46

requires Improved ReflectionType API so that a target reflection class can directly be retrieved, removed existing named constructor BetterReflection#415
class became interface
class became abstract
interface became final/regular class
interface became abstract class
method became final
method became abstract

… change

…essions

…erface in `BetterReflection`

…` for `ReflectionClass` (where applicable) to avoid passing around a `Reflector`

…ionClass` for `ReflectionClass` (where applicable) to avoid passing around a `Reflector`" This reverts commit e07a9ce.

…pe changes

…ionFunction`

…veraging the BetterReflection `AbstractReflectionFunction` existing API

…njected `FunctionBased` comparators

…uctor in `api-compare.php` (for now)

…of the other two

…n either way

… an exit code of `2` (as per mutation tests)

…ntee strictness on type checks, but rather fails silently

…hout any need for that

… given or not, there is no `null`

…th completely different checks

Ocramius · 2018-04-22T12:51:10Z

bin/api-compare.php

@@ -31,7 +40,210 @@
            new GetVersionCollectionFromGitRepository(),
            new PickLastMinorVersionFromCollection(),
            new Comparator(
-                new Comparator\BackwardsCompatibility\ClassBased\PropertyRemoved()


Note: this tree of checks is quite huge, but is pretty much describing what kinds of BC breaks we want to rely upon.

If we want to make it more "friendly", I can move the instantiation to Roave\ApiCompare\ApiCompare#defaultClassChecks(), Roave\ApiCompare\ApiCompare#defaultInterfaceChecks(), Roave\ApiCompare\ApiCompare#defaultTraitChecks() in a separate patch.

Ocramius · 2018-04-23T07:45:30Z

@asgrim this is ready for another round. Renamed few things, introduced traits checks.

asgrim · 2018-04-23T07:55:21Z

src/Command/ApiCompare.php

@@ -132,7 +129,7 @@ public function execute(InputInterface $input, OutputInterface $output) : int
            $outputFormats = $input->getOption('format') ?: [];
            Assert::that($outputFormats)->isArray();

-            if (in_array('markdown', $outputFormats, true)) {
+            if (ArrayHelpers::stringArrayContainsString('markdown', $outputFormats)) {


Uhh, probably a little OTT on the checking here... passing true includes type checks anyway, so the array passed doesn't even need to be all strings, if it matches, it matches, if not, it doesn't...

I want to avoid colliding with various tools that say different things here:

infection

ea inspections

phpstan strict checks

Segregating it out was easy peasy, so I just did it, and we have a single location where in_array() still exists.

Yes, this is indeed overblown, but I'd be OK with this particular addition, considering that it really is the only thing needed to make mutation tests work without problems (could've been much worse).

Another potential issue: contributors (me included) will likely default to just using in_array in future contributions, that may slip through the net :/ it's a non-standard approach, which is "unexpected" and inconsistent with current ... "ways" (for want of a better word)

that may slip through the net

Build will catch it now

Does the build fail with a message that indicates that ArrayHelpers::stringArrayContainsString should be used instead? (rhetorical question: it doesn't). The point is, you're solving a problem caused by the tooling (because in_array with strict param set to true is fine), and the solution is not what people will expect. I understand the technical reasoning here, and it's perfectly sound logic: my concern is that when you don't know what the issue is, the solution is unexpected and unconventional. I don't have a better answer, apart from this shouldn't be a problem and that the finger points back to the tools (phpstan, in this specific case)... :s

No, it won't fail with a matching error message, obviously.

the solution is not what people will expect.

Agreed, but I don't expect that to be a blocker, as a quick review can deal with most of these errors.

the solution is unexpected and unconventional.

Agreed again, as I said, the mutation testing outcome was much better than expected, so I'm quite happy with having this deviation from the standard compared to what it could have been if we had much more mutations.

apart from this shouldn't be a problem and that the finger points back to the tools (phpstan, in this specific case)...

Possible, but I don't think that should become a blocker for just a workaround in here. Assuming that we fix phpstan there, we can really just revert this addition (API is marked @internal anyway) and that's the end of the story :-P

asgrim · 2018-04-23T07:55:59Z

src/Comparator.php

@@ -19,12 +20,17 @@ class Comparator
    /** @var InterfaceBased */
    private $interfaceBasedComparisons;

+    /** @var TraitBased */
+    private $traitBasedComparisons;


Wish we'd added traits as a different PR :s this PR is getting way too big, stuff is gonna get missed

Agreed - did it mostly due to time constraints on my end, as I was in a hurry/constantly on the road yesterday.

asgrim · 2018-04-23T07:56:42Z

composer.json

@@ -21,7 +21,8 @@
        "phpunit/phpunit": "^7.0",
        "doctrine/coding-standard": "^4.0",
        "squizlabs/php_codesniffer": "^3.2",
-        "phpstan/phpstan": "^0.9.2"
+        "phpstan/phpstan": "^0.9.2",
+        "infection/infection": "^0.8.1"


Adding infection/infection could've been a separate PR :/

Same as #38 (comment) - indeed creeped in too much

asgrim · 2018-04-23T07:59:48Z

src/Comparator/BackwardsCompatibility/ClassBased/ClassBecameTrait.php

+{
+    public function compare(ReflectionClass $fromClass, ReflectionClass $toClass) : Changes
+    {
+        Assert::that($fromClass->getName())->same($toClass->getName());


Is this not an assumption that's made everywhere anyway...? i.e. afaik, we don't detect git renames

Aye, should probably drop these assertions from all ClassBased checks

asgrim · 2018-04-23T08:00:48Z

src/Comparator/BackwardsCompatibility/ClassBased/ClassBecameTrait.php

+        return Changes::fromArray([Change::changed(
+            sprintf('Class %s became a trait', $fromClass->getName()),
+            true
+        ),


Alignment looks wonky here?

asgrim · 2018-04-23T08:27:27Z

src/Comparator/BackwardsCompatibility/InterfaceBased/InterfaceBecameClass.php

+    /**
+     * According to the current state of the PHP ecosystem, we only have traits, interfaces and classes
+     */
+    private function isClass(ReflectionClass $class) : bool


Does ReflectionClass really not have an isClass method... seems like an oversight >.<

Doesn't have one, no. That's because we mimicked the existing ext-reflection API, without designing our own API from scratch upfront.

asgrim · 2018-04-23T08:27:51Z

src/Comparator/BackwardsCompatibility/InterfaceBased/InterfaceBecameTrait.php

+        return Changes::fromArray([Change::changed(
+            sprintf('Interface %s became a trait', $fromClass->getName()),
+            true
+        ),


Something weird causing these alignment issues in phpcs I expect...

Verified this: it's an issue in one of the sniffs.

Basically, single-value array should be on a single line, but then the value turns out to be multi-line here. Trying to fix it leads to a phpcs violation being reported, so I'll keep these wonky ones as they are here, and talk to the doctrine folks about it.

asgrim · 2018-04-23T08:32:09Z

src/Comparator/BackwardsCompatibility/TraitBased/TraitBecameClass.php

+        }
+
+        return Changes::fromArray([Change::changed(
+            sprintf('Interface %s became a class', $fromTrait->getName()),


Trait %s became a class

asgrim · 2018-04-23T08:32:27Z

src/Comparator/BackwardsCompatibility/TraitBased/TraitBecameInterface.php

+        }
+
+        return Changes::fromArray([Change::changed(
+            sprintf('Interface %s became an interface', $fromTrait->getName()),


Trait %s became an interface

asgrim · 2018-04-23T08:35:12Z

src/Support/ArrayHelpers.php

+     */
+    public static function stringArrayContainsString(string $value, array $arrayOfStrings) : bool
+    {
+        Assert::that($arrayOfStrings)->all()->string();


Really not sure we need this... seems to just be avoiding false positives; don't know specifically what mutations phpstan performs, but adding an integer onto this array shouldn't be a problem if the expected value is still in there (because we (were) using "strict" mode)... Think this conversation should be moved to a discussion upstream in phpstan, not solved with a wrapper IMO

See #38 (comment) for reasoning for this class first.

As for the assertion:

seems to just be avoiding false positives;

We use the in_array()-ish check mostly where reflection returns something to us. As PHP has no way to check type invariants on array values, having a type assertion is still probably a good idea. Can drop it, of course.

Ref: #38 (comment)

… review Ref: #38 (comment)

Ocramius · 2018-04-25T19:45:51Z

@asgrim feedback incorporated into the patch, but I'll keep the in_array() replacement in it for now

asgrim

OK 👍 let's

Ref: Roave/BackwardCompatibilityCheck#38 (comment)

… review Ref: Roave/BackwardCompatibilityCheck#38 (comment)

Ocramius added enhancement WIP labels Apr 15, 2018

Ocramius added this to the 0.0.1 milestone Apr 15, 2018

Ocramius self-assigned this Apr 15, 2018

Ocramius changed the title ~~Verify parameter type contravariance and return type covariance in method signature~~ Verify method, function, property, constant and interface changes and report BC breaks Apr 16, 2018

Ocramius added 22 commits April 17, 2018 00:35

Adding an utility to check if covariance is being respected in a type…

42def53

… change

Simplified nested conditional that could have been a boolean expression

709b22a

Simplified more nested conditionals that could have been boolean expr…

a2741a9

…essions

Completed covariance and contravariance type comparators

7c73cd1

Adding IDE hints for the reflection issues around the Reflector int…

ee3bfce

…erface in `BetterReflection`

Adding IDE hints for the reflection issues around the Reflector int…

f5d93f0

…erface in `BetterReflection`

Considering type nullability in variance and covariance checks

b522b08

Using an internal ReflectionType to preload target `ReflectionClass…

cd198fa

…` for `ReflectionClass` (where applicable) to avoid passing around a `Reflector`

Revert "Using an internal ReflectionType to preload target `Reflect…

ed255c8

…ionClass` for `ReflectionClass` (where applicable) to avoid passing around a `Reflector`" This reverts commit e07a9ce.

Leveraging improved ReflectionType API from Roave/BetterReflection#415

e38ca3a

Type BC break detector that can detect variance issues with return ty…

1de6f6f

…pe changes

Coverage declarations

7916b00

Multi function-based checker

94b8dd8

Verifying covariance also on ReflectionMethod, not just on `Reflect…

ebf80fe

…ionFunction`

Parameter type contravariance checks

e53bbf2

Improved Change wording for return type covariance checks

4684c13

Naive RequiredParameterAmountIncreased BC checker

dc8e3f2

Massively simplified RequiredParameterAmountIncreased checker by le…

51c1d28

…veraging the BetterReflection `AbstractReflectionFunction` existing API

BC comparator detecting default value changes

d6c8dff

Cleaning up comparator so it detects class and function changes via i…

61a83cd

…njected `FunctionBased` comparators

Passing a dummy FunctionBased comparator to the Comparator constr…

32b58d5

…uctor in `api-compare.php` (for now)

BC compliance check that verifies is a class became final

6816ead

Ocramius added 14 commits April 22, 2018 13:27

BC compliance checks to verify if a class/trait/interface became one …

fe838c2

…of the other two

Removed unused imports

cde7708

Adapters to use class-based checks on interfaces/traits

e77df74

Verifying Changes#count() behavior

c94b665

Verifying that iterable and non-array scalars are not covariant i…

edb5259

…n either way

Removed redundant Comparator check, as detected by mutation tests

8f30e48

Verifying that a single change in the result is sufficient to trigger…

dc4914c

… an exit code of `2` (as per mutation tests)

Getting rid of possible issues with in_array(), which doesn't guara…

40d50e6

…ntee strictness on type checks, but rather fails silently

Using non-strict in_array() checks to avoid copying data around wit…

6f87995

…hout any need for that

Documented ArrayHelpers as @internal

2ac4760

Removed redundant getOption() !== null check - the option is either…

783e8b2

… given or not, there is no `null`

Adding infection/infection to the build matrix

f598aa8

Treating class, interface and trait as completely distinct symbols wi…

424aefc

…th completely different checks

Automated CS fixes as per our phpcs rules

92c07a7

Ocramius commented Apr 22, 2018

View reviewed changes

asgrim requested changes Apr 23, 2018

View reviewed changes

Ocramius added 3 commits April 25, 2018 21:41

#38 removed same-class assertions as per @asgrim's review

943dd95

Ref: #38 (comment)

#38 imported constants are before imported functions

8c62056

#38 s/interface became a class/trait became a class/ as per @asgrim's…

361754c

… review Ref: #38 (comment)

asgrim approved these changes Apr 26, 2018

View reviewed changes

asgrim merged commit ad68300 into master Apr 26, 2018

asgrim deleted the feature/return-type-contravariance-checking branch April 26, 2018 08:59

asgrim unassigned akrabat Apr 26, 2018

Ocramius mentioned this pull request Apr 27, 2018

Consider final in combination with protected to exclude some code analysis scenarios #31

Closed

uzibhalepu added a commit to uzibhalepu/BackwardCompatibilityCheck that referenced this pull request Aug 6, 2024

Re-introduced comment about why visibility checks work on strings too

e135cd2

Ref: Roave/BackwardCompatibilityCheck#38 (comment)

uzibhalepu added a commit to uzibhalepu/BackwardCompatibilityCheck that referenced this pull request Aug 6, 2024

Re-introduced comment about why visibility checks work on strings too

8d68e20

Ref: Roave/BackwardCompatibilityCheck#38 (comment)

uzibhalepu added a commit to uzibhalepu/BackwardCompatibilityCheck that referenced this pull request Aug 6, 2024

#38 removed same-class assertions as per @asgrim's review

387cf90

Ref: Roave/BackwardCompatibilityCheck#38 (comment)

uzibhalepu added a commit to uzibhalepu/BackwardCompatibilityCheck that referenced this pull request Aug 6, 2024

#38 s/interface became a class/trait became a class/ as per @asgrim's…

685b6c3

… review Ref: Roave/BackwardCompatibilityCheck#38 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify method, function, property, constant and interface changes and report BC breaks #38

Verify method, function, property, constant and interface changes and report BC breaks #38

Ocramius commented Apr 15, 2018 •

edited

Loading

Ocramius Apr 22, 2018 •

edited

Loading

Ocramius commented Apr 23, 2018

asgrim Apr 23, 2018

Ocramius Apr 23, 2018

asgrim Apr 23, 2018

Ocramius Apr 23, 2018

asgrim Apr 23, 2018

Ocramius Apr 23, 2018

asgrim Apr 23, 2018

Ocramius Apr 23, 2018

asgrim Apr 23, 2018

Ocramius Apr 23, 2018

asgrim Apr 23, 2018

Ocramius Apr 23, 2018

asgrim Apr 23, 2018

asgrim Apr 23, 2018

Ocramius Apr 23, 2018

asgrim Apr 23, 2018

Ocramius Apr 25, 2018

asgrim Apr 23, 2018

asgrim Apr 23, 2018

asgrim Apr 23, 2018

Ocramius Apr 23, 2018

Ocramius commented Apr 25, 2018

asgrim left a comment

Verify method, function, property, constant and interface changes and report BC breaks #38

Verify method, function, property, constant and interface changes and report BC breaks #38

Conversation

Ocramius commented Apr 15, 2018 • edited Loading

Ocramius Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Ocramius commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Apr 25, 2018

asgrim left a comment

Choose a reason for hiding this comment

Ocramius commented Apr 15, 2018 •

edited

Loading

Ocramius Apr 22, 2018 •

edited

Loading