Skip to content

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Nov 8, 2024

When a type is variadic, and multiple named arguments are used, it's often hard to tell for which argument the error is coming.

Since we have the named argument, we could use it in this situation.

Example:

$this->doIpsum(1, 2, foo: $this->foo(), bar: $this->bar(), baz: $this->baz());

Before the error would be:

Parameter ...$args of method NamedArgumentsMethod\Foo::doIpsum() expects string, int given.

And one had to inspect the different method calls to see where it might originate.

After:

Named argument foo for variadic parameter ...$args of method NamedArgumentsMethod\Foo::doIpsum() expects string, int given.

It's immediately clear it's related to foo named argument.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Rebase this on top of 2.0.x
  2. I'd much rather do it in describeParameter private method, but please rethink how the message (messages) should change.

You have about 48 hours to do this change, otherwise we'd have to put it behind bleeding edge :)

@ruudk
Copy link
Contributor Author

ruudk commented Nov 8, 2024

I can do that, but let's first agree on the error message:

-Parameter ...$args of method NamedArgumentsMethod\Foo::doIpsum() expects string, int given.
+Named argument foo for parameter ...$args of method NamedArgumentsMethod\Foo::doIpsum() expects string, int given.

?

@ondrejmirtes
Copy link
Member

What about Named argument foo for variadic parameter ...$args in this case? To make it super obvious.

@ondrejmirtes
Copy link
Member

(Only add "variadic" before parameter only for this specific message, not always.)

@ruudk ruudk changed the base branch from 1.12.x to 2.0.x November 8, 2024 14:42
@ruudk ruudk force-pushed the use-named-argument-in-error branch 2 times, most recently from 93e808d to 991773b Compare November 8, 2024 14:45
@ruudk
Copy link
Contributor Author

ruudk commented Nov 8, 2024

Updated the PR

(Only add "variadic" before parameter only for this specific message, not always.)

Seeing this, I don't think it's what you had in mind.

How to deal with this when I need to do this in describeParameter?

Introduce a 3rd nullable argument and only set it for argument.type?

@ruudk ruudk force-pushed the use-named-argument-in-error branch from 991773b to 292ee14 Compare November 8, 2024 14:53
@ondrejmirtes
Copy link
Member

No, the point for doing it in describeParameter is that it should always change, for all its usages.

The current version is wrong for non-variadic parameters.

@ruudk
Copy link
Contributor Author

ruudk commented Nov 8, 2024

The current version is wrong for non-variadic parameters.

Could you tell me what's wrong? I'm lost now.

@ruudk ruudk force-pushed the use-named-argument-in-error branch from 292ee14 to 34570b7 Compare November 8, 2024 14:59
@ondrejmirtes
Copy link
Member

		if (is_int($positionOrNamed)) {
			$parts[] = 'Parameter #' . $positionOrNamed;
		} elseif (is_string($positionOrNamed)) {
			$parts[] = 'Named argument ' . $positionOrNamed . ' for variadic parameter';
		} else {
			$parts[] = 'Parameter';
		}

The elseif with message 'Named argument ' . $positionOrNamed . ' for variadic parameter' is triggered also for non-variadic parameters. That's wrong. You can see it from the tests you modified.

For example: Named argument b for variadic parameter $b - does not make sense, $b is not variadic, I can see that, because there's no ... in front of $b.

@ruudk
Copy link
Contributor Author

ruudk commented Nov 8, 2024

Wow, that doesn't make any sense at all indeed.

When a type is variadic, and multiple named arguments are used, it's often hard
to tell for which argument the error is coming.

Since we have the named argument, we could use it in this situation.
@ruudk ruudk force-pushed the use-named-argument-in-error branch from 34570b7 to e447874 Compare November 8, 2024 15:06
@ruudk ruudk requested a review from ondrejmirtes November 8, 2024 15:11
@ruudk
Copy link
Contributor Author

ruudk commented Nov 8, 2024

Alright, I think I have it now.

@ondrejmirtes ondrejmirtes merged commit a965e73 into phpstan:2.0.x Nov 8, 2024
415 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

@ruudk ruudk deleted the use-named-argument-in-error branch November 8, 2024 15:13
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 this pull request may close these issues.

2 participants