I'm okay with this change, but i fear it might be a BC break.

wdyt @veewee?

Pull Request Test Coverage Report for Build 5589071290

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 99.042%

Totals
Change from base Build 5518675338:	0.0%
Covered Lines:	4137
Relevant Lines:	4177

💛 - Coveralls

I don't think this is a breaking change because of:
https://psalm.dev/r/9715a63a4f

If you create an union of T | never, it seems to infer to T internally.
Validated this change on my projects who use Option quite intensively and got 0 errors reported by psalm either.

However, I do think we also need to add this never return to the regular none() function:

namespace Psl\Option;

/**
 * Create an option with none value.
 *
 * @return Option<never>
 */
function none(): Option
{
    return Option::none();
}

I found these snippets:

<?php

/** @var string|never $x */
$x = 'dd';

/** @psalm-trace $x */

Psalm output (using commit 9c814c8):

INFO: Trace - 6:23 - $x: string

INFO: UnusedVariable - 4:1 - $x is never referenced or the value is not used

It may be a breaking change in some cases.

/**
 * @return Option<string>
 */
public function test(): Option
{
    /** @var Option<string> */ 
    return none();
}

I'm not currently using Psalm so I'm unsure if this is a linting error or just a feature to remove "dead" code that does not belong to the static analysis part, but there is a redundant @var now: vimeo/psalm#2150

/**
 * @return Option<string>
 */
public function test(): Option
{
    /** @phpstan-ignore-next-line */ 
    return none();
}

In this example, PHPStan would throw an No error to ignore is reported [...] .

In both cases, I'm not sure if I would consider these BC changes in static analysis, or just regular fixes, but it will change a couple of baselines too.

Edit:

However, I do think we also need to add this never return to the regular none() function:

I think it would not be necessary, at least in PHPStan. Maybe Psalm would require it, idk

In psalm, you have to define it because of this:
https://psalm.dev/r/068ab0cbf1

If you don't specify the template, it will take mixed.
Maybe phpstan infers it from the caller instead of the callee or maybe it defaults to never?

I've reported an issue before in psalm regarding this 'optional' generic:
vimeo/psalm#8487
To me, it also makes more sense to default to never instead of mixed.

You might be able to use @psalm-var instead of @var?
https://psalm.dev/docs/annotating_code/supported_annotations/#psalm-var-psalm-param-psalm-return-psalm-property-psalm-property-read-psalm-property-write-psalm-method

However, I do think we also need to add this never return to the regular none() function:
I think it would not be necessary, at least in PHPStan. Maybe Psalm would require it, idk

It would rather be for consistency than for fixing a SAT complaint. If Option::none() returns Option<never>, then none() should also return Option<never>.
This should work just fine in your SAT:

https://psalm.dev/r/c6ab6659f3

I found these snippets:

<?php
/** @template T */
class Option {}

/** @return Option<never> */
function never(): Option {
    /** @var Option<never> */
    return new Option();
}

/** @return Option<never> */
function mixed(): Option {
    return new Option();
}


/** @return Option<string> */
function stringy(): Option {
    return new Option();
}

Psalm output (using commit 9c814c8):

INFO: MixedReturnTypeCoercion - 13:12 - The type 'Option<mixed>' is more general than the declared return type 'Option<never>' for mixed

INFO: MixedReturnTypeCoercion - 11:13 - The declared return type 'Option<never>' for mixed is more specific than the inferred return type 'Option<mixed>'

INFO: MixedReturnTypeCoercion - 19:12 - The type 'Option<mixed>' is more general than the declared return type 'Option<string>' for stringy

INFO: MixedReturnTypeCoercion - 17:13 - The declared return type 'Option<string>' for stringy is more specific than the inferred return type 'Option<mixed>'

<?php
/** @template T */
class Option {}

/** @return Option<never> */
function none(): Option {
    // Note : you won't need this var when returning Option::none()
    /** @var Option<never> */
    return new Option();
}

/**
 * @template T
 * @param T $x
 * @return Option<T>
 */
function some($x): Option {
    return new Option();
}

/** @return Option<literal-string> */
function maybe() {
    return rand(0,1) ? some('hello') : none();
}

Psalm output (using commit 9c814c8):

INFO: UnusedParam - 17:15 - Param x is never referenced in this method

You are right @veewee, the Option<never> in Psl\Option\none is necessary in PHPStan! Updating the PR.

I'm not scanning your project with PHPStan as I guess you are not trying to be PHPStan compliant. If you think it can be interesting to pass both static analysis, I could start a draft if you are ok with that 😊

@devnix would welcome that, there's no harm in having it here too.