Invert meaning of dupe_key

[danog]

For example in https://psalm.dev/r/09048f8f8a, one issue was hiding the other.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/09048f8f8a

<?php

interface a {
    public function test(): mixed;
}

class b implements a
{
    public function test(string $foo) {
        return 'foo';
    }
}

Psalm output (using commit ab2b77d):

ERROR: MethodSignatureMismatch - 7:7 - Method b::test with return type '' is different to return type 'mixed' of inherited method a::test

[danog]

I wonder whether it would be worth it to make the dupe key a mandatory argument for all issues in v5, this seems like a common issue inside psalm...
Or maybe use the description as a dupe key, instead.

[danog]

This effectively renders the dupe_key useless, maybe it's worth deprecating for v5.
Personally I consider it just a useless and dangerous redundancy that can hide some issues that other static analysis tools might detect, see https://phpstan.org/r/050dffdd-aaab-431a-87cc-9261a166ce9c vs https://psalm.dev/r/66d598df84

[orklah]

Do you have a before/after for your snippet? I'm not sure I see what more it will display with your change?

[AndrolGenhald]

@orklah It should also mention something about $foo since the interface declaration doesn't have any arguments. Since it's the same issue with a different message it's only showing one instead of both.

[orklah]

Indeed. So it's caused by the fact we emit the same error on the same place.

I think an easier answer would be to try to report those errors on the faulty param/return instead of messing with the dupes, no?

[AndrolGenhald]

Perhaps, but are there any cases where an error with a different message shouldn't show up?

I'm guessing the reason it doesn't use the location of the param/return is because they might not exist. For the return value in this case, where would you put the error? On ) {?
No clue why this is an error on the class rather than the method though, that makes no sense to me.

[orklah]

I guess return type could go on method name if needed. Params type on the params and it should be ok.

We can see dropping the dupe feature has a dramatical effect on phpunit, I don't think this is the way

[AndrolGenhald]

What about this? Seems like that should probably use the same location as a missing return type, whether it's the entire declaration or just the method name.

Just so we're on the same page, I think these improvements you're suggesting are a good idea as well, I'm just not so sure this PR is a bad idea either. I think it's at least worth seeing if it improves other cases as well.

Unfortunately I don't think our current tests work well for testing this issue, I believe most of them stop as soon as the first issue is added. I think we would need to test it on a real-world project that has a lot of suppressions and see if it turns up valid issues that were clobbered due to the duplicate issue name.

Edit: Looks like it might be excessively verbose for MissingThrowsDocblock, but I'm not sure it's a bad thing. It seems better to have it show each one that's missing rather than only one at a time as you fix them. This is more of a problem.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/39e90deadf

<?php

class Foo
{
    public function foobar(int $a, string $b): void {}
}

class Bar extends Foo
{
    public function foobar(int $a) {}
}

Psalm output (using commit c7d938b):

ERROR: MethodSignatureMismatch - 10:5 - Method Bar::foobar with return type '' is different to return type 'void' of inherited method Foo::foobar

[danog]

... This is more of a problem.

There even more issues like this, now that I'm actually looking for them during tests, that's why I sent this PR :D
I fixed the unit tests, the only thing left to do is to updating the baseline for linked projects.

[AndrolGenhald]

I fixed the unit tests, the only thing left to do is to updating the baseline for linked projects.

I think we shouldn't be updating the tests to ignore that, the point of the dupe key is to prevent duplicate issues. The duplicate issue is what should be fixed. Ideally I think we should go through issue by issue and make sure there can't be duplicates, but that's a lot of work. Another option would be to change the dupe key for specific issues like MissingThrowsDocblock to make sure non-duplicate issues of the same type for the same location are allowed.

[danog]

I think we should go through issue by issue [...], but that's a lot of work

That's precisely what I thought, and why I made this PR, instead.
By checking against the issue message instead of an optional duplicate key, we can be 100% sure there will be no duplicate issue, and that no issue was mistakenly dropped.

Really, I don't understand why the redundant duplicate key was even used in the first place, when the issue message is already perfect for distinguishing and removing duplicate issues.

[AndrolGenhald]

...we can be 100% sure there will be no duplicate issue...
Really, I don't understand why the redundant duplicate key was even used in the first place, when the issue message is already perfect for distinguishing and removing duplicate issues.

I'm guessing it was due to stuff like this

ERROR: DocblockTypeContradiction - src/Logging/TeamCityLogger.php:100:21 - Docblock-defined type string for $expectedString is never null (see https://psalm.dev/155)
                if ($expectedString === null || empty($expectedString)) {

ERROR: DocblockTypeContradiction - src/Logging/TeamCityLogger.php:100:21 - Cannot resolve types for $expectedString - docblock-defined type string does not contain null (see https://psalm.dev/155)
                if ($expectedString === null || empty($expectedString)) {

Those two issues have different phrasing, but they're redundant. We should only be showing one of them.

I'm guessing for really messy codebases there will end up being a lot of redundant issues that have different phrasing.

[danog]

Those two issues have different phrasing, but they're redundant.

Ah yes, in this case it does make sense, but I'd still like to remove the dupe_key completely or at least make it non-optional, because there are a lot of valid and different issues that are removed.

In practice, almost nothing changes in messy codebases where all errors are either suppressed or baselined, while testing and reproducing issues on psalm.dev becomes much easier (and as a bonus, Psalm won't seem to underperform in comparison to other tools, ie https://psalm.dev/r/66d598df84 vs https://phpstan.org/r/050dffdd-aaab-431a-87cc-9261a166ce9c)!

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/66d598df84

<?php

class Foo implements Iterator
{
    public function current(string $foo) {
        return 'foo';
    }
    
    public function next($a) {
    }
    
    public function key() {}
    
    public function valid(): string
    {
        return 'foo';
    }
    
    public function rewind(): void
    {
        
    }
}

Psalm output (using commit bf22dcf):

ERROR: MethodSignatureMustProvideReturnType - 5:21 - Method Foo::current must have a return type signature!

ERROR: MethodSignatureMustProvideReturnType - 9:21 - Method Foo::next must have a return type signature!

ERROR: MethodSignatureMustProvideReturnType - 12:21 - Method Foo::key must have a return type signature!

INFO: MissingParamType - 9:26 - Parameter $a has no provided type

[danog]

Changed the semantics of this PR to simply invert the meaning of dupe_key.
Originally, the dupe_key was used to distinguish and emit different errors emitted on the same line.
With this PR, the dupe_key is used to indicate and remove duplicate errors emitted on the same line.

[orklah]

This has become a BC break then, plugins will not expect this change of behaviour.

This could be changed in master though.

However, if we do that, I'd expect us to be able to remove any dupe key currently in the code (because the goal is practically inverted, so old dupe keys are outdated).

Then, there's a work to be done to add dupe keys in order to remove legitimate dupes. For example, those in phpunit right now.

[danog]

Done, I haven't actually removed all dupe keys as they might still indicate valid duplicates, I've just replaced all null dupe keys with textual dupe keys.

[danog]

Rebased, the new psalm issue seems to be caused by an update of the language server, and can be reproduced on master.
Can I haz merge :3

[AndrolGenhald]

Seems good to me, I have a feeling there may be more duplicates to deal with in the future but at least a duplicate issue is more visible than a missing issue, and they should be simple fixes.

[orklah]

Thanks!

This may end up being reverted if we have bad feedbacks on 5.0 though. The amount of new issues in PHPUnit is kinda worrying

[AndrolGenhald]

The amount of new issues in PHPUnit is kinda worrying

I was actually considering that earlier, since they were all MissingThrowsDocblock it sort of feels like we should have duplicate issue handling where if ExceptionB is a child of ExceptionA then the MissingThrowsDocblock for ExceptionB should be considered a duplicate of the MissingThrowsDocblock for ExceptionA. It sounds like it could be difficult though, and that check isn't even enabled by default.

[pilif]

commit f056072 of this PR is causing issue #9535, though it might be that this was just hiding another bug.