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

Php storm flags unwrap method as unreachble statement #16

Closed
JeremyM1985 opened this issue May 3, 2024 · 5 comments · Fixed by #17
Closed

Php storm flags unwrap method as unreachble statement #16

JeremyM1985 opened this issue May 3, 2024 · 5 comments · Fixed by #17

Comments

@JeremyM1985
Copy link

The @psalm-return never-return annotation for the unwrap() in class None, flags any proceeding code as unreachable when using Option as a reutrn type.

public function getMemberById(MemberId $memberId): Option
{
    if ($this->hasMemberWithId($memberId)) {
        return new some($this->members[$memberId->toString()]);
    }

    return new None();
}

//

$optional = $this->getMemberById($memberId);

if ($optional->isSome()) {
    $this->member= $optional->unwrap();


    // any further code here is marked as unreachable statement

}

Using the native php never return type (from 8.1) instead of the annotation seems to solve my issue.

Maybe you have an idea? or perhaps i should return union type Some|None instead?

@prewk
Copy link
Owner

prewk commented May 3, 2024

Yeah I can see why that would happen.. I don't know what's kosher here, to me it seems it should say that it returns T as its interface tells us, but throw at runtime anyway.

@Lctrs can you shed some light here?

I'm out of the loop with PHP, haven't been in the ecosystem since 2018 basically, but happy to help in any way I can.

@Lctrs
Copy link
Contributor

Lctrs commented May 4, 2024

Arguably, I would say that PHPStorm is not wrong to tell you this code is unreachable as it might be at this point given the types it knows at this point for the $optional variable - which, from its point of view, is still Some|None at this point as we never tell that is Option::isSome() returns true, it is a Some instance.

Given that Option is not a core PHP's concept (ie. not integrated in the language itself), I would say that mimicking Rust's API to the fullest was an error, and you should not use Option::isSome() or Option::isNone() methods but rather instanceof Some and instanceof None to check if it has a value or not to have IDEs fully understand the types of the variable at play here.

@prewk
Copy link
Owner

prewk commented May 4, 2024

Wouldn't letting None say it's returning T work as well?

@prewk prewk self-assigned this May 4, 2024
@Lctrs
Copy link
Contributor

Lctrs commented May 4, 2024

Yeah, I was talking about the current state of the lib.

@prewk prewk removed their assignment May 4, 2024
@prewk
Copy link
Owner

prewk commented May 4, 2024

Yeah as you implied, PHP can't infer stuff. I guess Psalm would be able to if it was advanced enough. What I don't understand is how phpStorm finds the None class and gets the types from it? It must be pretty confused. Shouldn't it just use the Option interface?

The Rust impl looks like this:

    pub const fn unwrap(self) -> T {
        match self {
            Some(val) => val,
            None => panic("called `Option::unwrap()` on a `None` value"),
        }
    }

If phpStorm indeed does read the types in Some and None, and not just in Option, I assume the correct thing would be to have identical types in all three?

Lctrs added a commit to Lctrs/option that referenced this issue May 5, 2024
* remove handling of `$pass` properties, as this is not the role of `Option`'s subclasses to pass them around.
* simplify phpdoc types, making `Option` the only source of truth for most methods
* added some static analysis assert for `Option#isSome()` and `Option#isNone()` (closes prewk#16)
* added `Option#inspect()` and `Option#filter()` from their Rust counterparts
* migrating from phpspec to PHPUnit
* added a Makefile to help during development
* added a coding standard via `easy-coding-standard`
* added mutation testing coverage via `infection`
* added some static analysis tests
* modernize github actions setup
* added dependabot config
* upgrading all dependencies
Lctrs added a commit to Lctrs/option that referenced this issue May 5, 2024
* dropping support for `PHP < 8.1.0` 
* remove handling of `$pass` properties, as this is not the role of `Option`'s subclasses to pass them around.
* simplify phpdoc types, making `Option` the only source of truth for most methods
* added some static analysis assert for `Option#isSome()` and `Option#isNone()` (closes prewk#16)
* added `Option#inspect()` and `Option#filter()` from their Rust counterparts
* migrating from phpspec to PHPUnit
* added a Makefile to help during development
* added a coding standard via `easy-coding-standard`
* added mutation testing coverage via `infection`
* added some static analysis tests
* modernize github actions setup
* added dependabot config
* upgrading all dependencies
@Lctrs Lctrs mentioned this issue May 5, 2024
Lctrs added a commit to Lctrs/option that referenced this issue May 5, 2024
* dropping support for `PHP < 8.1.0`
* remove handling of `$pass` properties, as this is not the role of `Option`'s subclasses to pass them around.
* simplify phpdoc types, making `Option` the only source of truth for most methods
* added some static analysis assert for `Option#isSome()` and `Option#isNone()` (closes prewk#16)
* added `Option#inspect()` and `Option#filter()` from their Rust counterparts
* migrating from phpspec to PHPUnit
* added a Makefile to help during development
* added a coding standard via `easy-coding-standard`
* added mutation testing coverage via `infection`
* added some static analysis tests
* modernize github actions setup
* added dependabot config
* upgrading all dependencies
Lctrs added a commit to Lctrs/option that referenced this issue May 5, 2024
* dropping support for `PHP < 8.1.0`
* remove handling of `$pass` properties, as this is not the role of `Option`'s subclasses to pass them around.
* simplify phpdoc types, making `Option` the only source of truth for most methods
* added some static analysis assert for `Option#isSome()` and `Option#isNone()` (closes prewk#16)
* added `Option#inspect()` and `Option#filter()` from their Rust counterparts
* migrating from phpspec to PHPUnit
* added a Makefile to help during development
* added a coding standard via `easy-coding-standard`
* added mutation testing coverage via `infection`
* added some static analysis tests
* modernize github actions setup
* added dependabot config
* upgrading all dependencies
Lctrs added a commit to Lctrs/option that referenced this issue May 6, 2024
* dropping support for `PHP < 8.1.0`
* simplify phpdoc types, making `Option` the only source of truth for most methods
* added some static analysis assert for `Option#isSome()` and `Option#isNone()` (closes prewk#16)
* added `Option#inspect()` and `Option#filter()` from their Rust counterparts
* migrating from phpspec to PHPUnit
* added a Makefile to help during development
* added a coding standard via `easy-coding-standard`
* added mutation testing coverage via `infection`
* added some static analysis tests
* modernize github actions setup
* added dependabot config
* upgrading all dependencies
@prewk prewk closed this as completed in #17 May 7, 2024
prewk pushed a commit that referenced this issue May 7, 2024
* dropping support for `PHP < 8.1.0`
* simplify phpdoc types, making `Option` the only source of truth for most methods
* added some static analysis assert for `Option#isSome()` and `Option#isNone()` (closes #16)
* added `Option#inspect()` and `Option#filter()` from their Rust counterparts
* migrating from phpspec to PHPUnit
* added a Makefile to help during development
* added a coding standard via `easy-coding-standard`
* added mutation testing coverage via `infection`
* added some static analysis tests
* modernize github actions setup
* added dependabot config
* upgrading all dependencies
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 a pull request may close this issue.

3 participants