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

ParamNameMismatch not reported for __construct #10784

Closed
kkmuffme opened this issue Mar 4, 2024 · 5 comments · Fixed by #10821
Closed

ParamNameMismatch not reported for __construct #10784

kkmuffme opened this issue Mar 4, 2024 · 5 comments · Fixed by #10821
Labels

Comments

@kkmuffme
Copy link
Contributor

kkmuffme commented Mar 4, 2024

https://psalm.dev/r/f49829b72e

Besides named argument calls, this is also an issue with promoted properties: https://psalm.dev/r/3f6f25190f
which is the reason this should report an error

Copy link

psalm-github-bot bot commented Mar 4, 2024

I found these snippets:

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

class A
{
	public function __construct(
		string $name,
		string $email,
	) {}

	public function foo(
		string $name,
		string $email,
	) {}
}

class B extends A
{
	public function __construct(
		string $names,
		string $email,
	) {}

	public function foo(
		string $names,
		string $email,
	) {}
}
Psalm output (using commit 3600d51):

INFO: MissingReturnType - 10:18 - Method A::foo does not have a return type, expecting void

ERROR: ParamNameMismatch - 24:10 - Argument 1 of B::foo has wrong name $names, expecting $name as defined by A::foo

INFO: MissingReturnType - 23:18 - Method B::foo does not have a return type, expecting void
https://psalm.dev/r/3f6f25190f
<?php

class A
{
	public function __construct(
		public string $name,
		public string $foo,
	) {}
}

class B extends A
{
	public function __construct(
		public string $names,
		public string $foo,
	) {}
}
Psalm output (using commit 3600d51):

No issues!

@weirdan
Copy link
Collaborator

weirdan commented Mar 4, 2024

Constructors generally don't have to be compatible. However we should report name mismatches if the constructor is declared consistent by using @psalm-consistent-constructor: https://psalm.dev/r/80275d44f3

Copy link

psalm-github-bot bot commented Mar 4, 2024

I found these snippets:

https://psalm.dev/r/80275d44f3
<?php
/** @psalm-consistent-constructor */
class A
{
	public function __construct(
		string $aaaa,
	) {}
}

class B extends A
{
	public function __construct(
		string $xxx,
	) {}
}
Psalm output (using commit 3600d51):

No issues!

@weirdan weirdan added the bug label Mar 4, 2024
@kkmuffme
Copy link
Contributor Author

kkmuffme commented Mar 4, 2024

But isn't this an issue with named arguments then? Only if @no-named-arguments is used, it shouldn't report an error, shouldn't it?

EDIT:
since this has changed with named args in PHP 8 while the concept was created before, I think the logic needs to be adjusted so that:

  • if @no-named-arguments is used, the param names are not validated, else it's validated by default (different from current behavior, where @no-named-arguments has no impact on the __construct at all in psalm)
  • if @psalm-consistent-constructor is used, the types are validated (otherwise not), this has no impact on names
  • if we have both only the types are validated (= current behavior of @pslam-consistent-constructor, which now additionally requires @no-named-arguments)

@weirdan
Copy link
Collaborator

weirdan commented Mar 5, 2024

No, it should be like the following:

  • when neither of @psalm-consistent-constructor / @no-named-arguments are present, no consistency (neither names nor types) is enforced
  • when @psalm-consistent-constructor is present without @no-named-arguments, both names and types have to match
  • when both are present, only types have to match (much like the current behaviour of @psalm-consistent-constructor)

kkmuffme added a commit to kkmuffme/psalm that referenced this issue Mar 13, 2024
kkmuffme added a commit to kkmuffme/psalm that referenced this issue Mar 13, 2024
weirdan pushed a commit to kkmuffme/psalm that referenced this issue Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants