Skip to content
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

Fix !isset() with ArrayDimFetch #2798

Draft
wants to merge 16 commits into
base: 1.10.x
Choose a base branch
from
Draft

Fix !isset() with ArrayDimFetch #2798

wants to merge 16 commits into from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Nov 30, 2023

the current 1.10.x handles the !isset() context for variables only.
it works only correct for !isset($var) and its implications on the variable.

with this PR we add support for !isset($var['abc']) which means

  • we handle implications on $var's array-shape based on the $var['abc']-offset type (based on nullability and optional/non-optional offset state)

with this PR we know when

  • offsets can only be null or non-null in true/falsey-context
  • offsets may or will not exist in falsey-context
  • on array-shapes with just a single offset we know about the certainty implications on the var in falsey-context

the newly added tests in tests/PHPStan/Analyser/data/falsy-isset.php best reflect the new type inference implications.

closes phpstan/phpstan#9908
closes phpstan/phpstan#9426
closes phpstan/phpstan#8724
closes phpstan/phpstan#5128

@staabm
Copy link
Contributor Author

staabm commented Nov 30, 2023

the remaining error is strange

Offset 'input' does not exist on array{input: Bug8724b\Input}|array{label: string}.

I have not yet an idea about it... but maybe we should even expect this error?

type-wise the offset label always exists, which means the null-coalescing is unnecessary and the code right to ?? is actually dead code..?

/**
 * @param array{label:string}|array{input:Input} $data
 */
function test(array $data): void
{
	$label = $data['label'] ?? $data['input']->getLabel();
}

@staabm staabm marked this pull request as ready for review November 30, 2023 14:46
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@clxmstaab
Copy link
Contributor

clxmstaab commented Nov 30, 2023

ohh I get it now.. I think the PR fixes the initial reported snippet but not the one posted within a followup comment

@ondrejmirtes
Copy link
Member

Can you please explain in your own words what are the faults of the current handling (as of 1.10.x) and what approach this PR does to fix that?

@staabm
Copy link
Contributor Author

staabm commented Nov 30, 2023

I added a description to the PR

@ondrejmirtes
Copy link
Member

Alright, what about nested expressions like $foo['abc']['def']? Does it work correctly?

@staabm staabm marked this pull request as draft December 1, 2023 06:31
}

assertVariableCertainty(TrinaryLogic::createMaybe(), $a);
assertType("array{bar: 1}|array{bar?: null}", $a);
assertType("array{bar: 1}|array{bar: null}", $a);
Copy link
Contributor

@mvorisek mvorisek Dec 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect the type to be simplified to array{bar: 1|null}

staabm and others added 11 commits December 9, 2023 15:19
commit 389b651
Author: Markus Staab <[email protected]>
Date:   Tue Nov 28 15:27:18 2023 +0100

    Discard changes to src/Analyser/MutatingScope.php

commit 0860ca3
Author: Markus Staab <[email protected]>
Date:   Tue Nov 28 15:24:47 2023 +0100

    Discard changes to src/Node/Printer/Printer.php

commit f22efef
Author: Markus Staab <[email protected]>
Date:   Tue Nov 28 15:24:20 2023 +0100

    Discard changes to src/Node/IssetExpr.php

commit 2d0dbff
Author: Markus Staab <[email protected]>
Date:   Thu Nov 2 17:07:01 2023 +0100

    Discard changes to src/Analyser/TypeSpecifier.php

commit 9cfc551
Author: Markus Staab <[email protected]>
Date:   Thu Nov 2 17:06:28 2023 +0100

    Discard changes to tests/PHPStan/Analyser/TypeSpecifierTest.php

commit 2f75d65
Author: Markus Staab <[email protected]>
Date:   Tue Oct 31 11:44:35 2023 +0100

    move certainty into IssetExpr

commit 4c4caab
Author: Markus Staab <[email protected]>
Date:   Mon Oct 30 21:13:03 2023 +0100

    NotIssetExpr -> IssetExpr

commit 8c0eaf9
Author: Markus Staab <[email protected]>
Date:   Fri Oct 6 00:03:38 2023 +0200

    fix

commit 700c744
Author: Markus Staab <[email protected]>
Date:   Thu Oct 5 23:58:39 2023 +0200

    Added regression test

commit 99580ee
Author: Markus Staab <[email protected]>
Date:   Thu Oct 5 23:50:14 2023 +0200

    Added regression test

commit 8ad159f
Author: Markus Staab <[email protected]>
Date:   Thu Oct 5 23:41:10 2023 +0200

    Added regression test

commit 0665831
Author: Markus Staab <[email protected]>
Date:   Thu Oct 5 22:17:14 2023 +0200

    fix

commit 270500e
Author: Markus Staab <[email protected]>
Date:   Thu Oct 5 22:08:00 2023 +0200

    cs

commit b7609eb
Author: Markus Staab <[email protected]>
Date:   Thu Oct 5 22:00:36 2023 +0200

    support regular variables

commit b0d8ee0
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 21:36:05 2023 +0200

    fix collision

commit f3672dd
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 21:31:23 2023 +0200

    cs

commit 08bb671
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 21:31:02 2023 +0200

    fix

commit 8c73fb3
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 17:50:24 2023 +0200

    fix regression in bug-7224

commit aa6c58e
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 17:06:20 2023 +0200

    Added regression test

commit 6dfc1e3
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 17:02:43 2023 +0200

    Added regression test

commit 6623061
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 17:00:36 2023 +0200

    Added regression test

commit 9a00419
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 16:58:45 2023 +0200

    Added regression test

commit b1fc1dd
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 16:57:18 2023 +0200

    Added regression test

commit 745e24b
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 16:54:03 2023 +0200

    Added regression test

commit 069d259
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 15:32:51 2023 +0200

    cleanup after review

commit de1eab9
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 10:11:26 2023 +0200

    fix

commit 347e5a8
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 09:58:58 2023 +0200

    cs

commit b5b7d59
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 09:54:50 2023 +0200

    fix tests

commit 053b291
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 09:52:37 2023 +0200

    fix

commit 7b2b956
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 09:27:17 2023 +0200

    fix

commit 4c4b293
Author: Markus Staab <[email protected]>
Date:   Wed Oct 4 09:14:17 2023 +0200

    more tests

commit f26b556
Author: Markus Staab <[email protected]>
Date:   Tue Oct 3 22:04:59 2023 +0200

    not isset

commit e626610
Author: Markus Staab <[email protected]>
Date:   Tue Oct 3 22:02:59 2023 +0200

    Revert "drive by cleanup"

    This reverts commit 73105182565b9539d24a842b82b7416dc101f2f4.

commit 6949740
Author: Markus Staab <[email protected]>
Date:   Tue Oct 3 22:02:08 2023 +0200

    impl

commit 26d6756
Author: Markus Staab <[email protected]>
Date:   Tue Oct 3 18:55:21 2023 +0200

    Create UnsetExpr.php

commit ec321e8
Author: Markus Staab <[email protected]>
Date:   Tue Oct 3 18:36:42 2023 +0200

    more tests

commit 800a678
Author: Markus Staab <[email protected]>
Date:   Tue Oct 3 13:21:01 2023 +0200

    add falsy-isset tests

commit 2f8eaa1
Author: Markus Staab <[email protected]>
Date:   Sat Sep 30 18:59:19 2023 +0200

    fix test expectations

commit 459fa95
Author: Markus Staab <[email protected]>
Date:   Sat Sep 30 15:01:08 2023 +0200

    Fix !isset() with ArrayDimFetch

commit 9c1c44e
Author: Markus Staab <[email protected]>
Date:   Sat Sep 30 14:55:24 2023 +0200

    Added regression test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants