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

$this cannot be referenced in immutable enums? #7629

Closed
Ocramius opened this issue Feb 10, 2022 · 13 comments
Closed

$this cannot be referenced in immutable enums? #7629

Ocramius opened this issue Feb 10, 2022 · 13 comments

Comments

@Ocramius
Copy link
Contributor

Ocramius commented Feb 10, 2022

Perhaps a misunderstanding question, but https://psalm.dev/r/83ff12ab7a should probably work:

<?php

/** @psalm-immutable */
enum MyEnum
{
    case FOO;
    case BAR;
    
    /** @psalm-pure */
    public function someSwitch(): int
    {
        return match ($this) {
              self::FOO => 1,
              self::BAR => 2,  
        };
    }
}
Psalm output (using commit c885dbc): 

ERROR: [ImpureVariable](https://psalm.dev/235) - 12:23 - Cannot reference $this in a pure context

Shouldn't an immutable object be allowed in a pure function? 🤔

Related: #7438

/cc @Lucian-Olariu @mitzaalex

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/83ff12ab7a
<?php

/** @psalm-immutable */
enum MyEnum
{
    case FOO;
    case BAR;
    
    /** @psalm-pure */
    public function someSwitch(): int
    {
        return match ($this) {
              self::FOO => 1,
              self::BAR => 2,  
        };
    }
}
Psalm output (using commit c885dbc):

ERROR: ImpureVariable - 12:23 - Cannot reference $this in a pure context

@orklah
Copy link
Collaborator

orklah commented Feb 10, 2022

I think I remember an discussion about this where @weirdan told that an immutable class can contain a mutable object (as a property)

So if this is the starting point, the mutable object in the class could change between calls, making the function potentially impure by virtue of receiving something that might change: https://psalm.dev/r/ac96697cf2

I may be wrong on that though

However, not sure how exactly we could abuse your example with an enum. I didn't tweak with that enough

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/ac96697cf2
<?php

/** @psalm-immutable */
class A{
    public function __construct(public stdClass $b){}
}

/** 
 * @psalm-pure 
 */
function pure(A $a): stdClass{
    return $a->b;
}

$a = new A(new stdClass());
$a->b->test = 12;
echo pure($a)->test;
$a->b->test = 15;
echo pure($a)->test;
Psalm output (using commit c885dbc):

INFO: MixedArgument - 17:6 - Argument 1 of echo cannot be mixed, expecting string

INFO: MixedArgument - 19:6 - Argument 1 of echo cannot be mixed, expecting string

@Ocramius
Copy link
Contributor Author

told that an immutable class can contain a mutable object (as a property)

Uhhh, that seems extremely wrong upfront :| Perhaps 5.0 should make this a bit more "safe", and prevent non-@psalm-immutable references inside object state?

@orklah
Copy link
Collaborator

orklah commented Feb 10, 2022

I couldn't find the discussion so maybe I imagined it... :p

Found that though, which would seem to contradict the current behaviour: #4126 (comment)

@Ocramius
Copy link
Contributor Author

Seems wrong, since @psalm-immutable can contain mutable state (according to current semantics). I understand why Psalm decides to prevent access to $this in the context of a @psalm-immutable object, as there is no guarantee of immutability right now, but we kinda need that guarantee.

Perhaps a migration to a new annotation, at this point?

@weirdan
Copy link
Collaborator

weirdan commented Feb 10, 2022

Discussed in #6881

@AndrolGenhald
Copy link
Collaborator

AndrolGenhald commented Feb 10, 2022

I'm in favor of @psalm-immutable-strict or similar to allow reading but not writing $this properties, and requiring all properties (and consts now...) to be immutable. I think that would solve #7438 as well, it seems to be the same issue as #6881 if I understand correctly.

I actually wouldn't mind allowing the tag on class consts as well, I'm not really a fan of PHP's decision to allow mutable objects as consts, but tagging the const as immutable would allow users to use const objects while still requiring immutability. On the other hand, maybe it's not really worth it since you could just mark the class you're assigning the const to as immutable-strict and call it good. Now I'm thinking about adding a config option to require const objects to be immutable-strict.

@Baptouuuu
Copy link

Isn't the problem in the given example the @psalm-pure annotation ? If I'm not mistaken all methods of an immutable object are by definition pure and here the @psalm-pure override the default behaviour and forbid the use of $this.

See https://psalm.dev/r/24faca0cfb

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/24faca0cfb
<?php

/** 
 * @psalm-immutable 
 */
enum MyEnum
{
    case FOO;
    case BAR;
    
    public function someSwitch(): int
    {
        echo 'foo';
        
        return match ($this) {
              self::FOO => 1,
              self::BAR => 2,  
        };
    }
}
Psalm output (using commit e7f05c3):

ERROR: ImpureFunctionCall - 13:9 - Cannot call echo from a mutation-free context

@Ocramius
Copy link
Contributor Author

@Baptouuuu if the $this is immutable, or if you call an immutable method on that, it should be OK.

@Ocramius
Copy link
Contributor Author

This seems to be another variation of @psalm-pure vs @psalm-mutation-free.

Annotating the class as @psalm-immutable, then skipping @psalm-pure on instance methods seems to work:

https://psalm.dev/r/07285c1b10

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/07285c1b10
<?php

/** @psalm-immutable */
enum MyEnum
{
    case FOO;
    case BAR;
    
    public function someSwitch(): int
    {
        return match ($this) {
              self::FOO => 1,
              self::BAR => 2,  
        };
    }
}
Psalm output (using commit 9770e11):

No issues!

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

No branches or pull requests

5 participants