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

Implement template default types #3457

Merged
merged 12 commits into from
Oct 9, 2024

Conversation

jiripudil
Copy link
Contributor

Closes phpstan/phpstan#4801

I've finally picked up where @rvanvelzen left off in #1835. I've mainly added checks that the default type is valid and within bounds.

@phpstan-bot
Copy link
Collaborator

You've opened the pull request against the latest branch 2.0.x. PHPStan 2.0 is not going to be released for months. If your code is relevant on 1.12.x and you want it to be released sooner, please rebase your pull request and change its target to 1.12.x.

@ondrejmirtes
Copy link
Member

Wow, what a thing to drop on Friday afternoon! :) Do you think you could base it on 1.12.x? IMHO you shouldn't have any conflicts doing that. If it'd be too difficult then I'm happy to get this in 2.0, but please try to rebase it back on 1.12.x :)

@jiripudil jiripudil changed the base branch from 2.0.x to 1.12.x September 20, 2024 13:54
@jiripudil
Copy link
Contributor Author

Sure, rebased on 1.12.x :)

@@ -0,0 +1,82 @@
<?php

namespace TemplateDefault;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is realworld example where default type is helpful over default implied maximal type (the type after of)?

Copy link
Member

Choose a reason for hiding this comment

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

See the discussion at phpstan/phpstan#4801

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue did not answer my question. Default implied maximal type would solve the:

Consider this: If a value passed for $a is null, it's impossible to resolve TResult's type. Then default type should be used. Currently, the default type everywhere is mixed instead.

too and in much safer way without any extra coding needed as long as of type is specified.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I don't get it. Please show the example in phpstan/phpstan#4801 and discuss it there. I plan to merge this PR very close to as it is.

Other languages also offer "default types for @template".

@ondrejmirtes
Copy link
Member

I really like this PR! I just checked out the branch and I'm trying to break it locally. So far I found:

  1. This should report an error (the default in the middle doesn't make sense):
/**
 * @template T of int
 * @template U of string = 'foo'
 * @template V of string
 */
class Foo
{

}
  1. There are some messages that are not aware of default types. When the generics are missing completely: "Function aaa\foo() has parameter $d with generic class aaa\Foo but does not specify its types: T, U, V" (the message should somehow say that some of these types at the end of the list are optional). See 3) for a suggestion at the end.

  2. When the generics are there but not all of the required ones. Let's say I have class Foo with 3 @template and the last one is optional. Having Foo<1> leads to error message: Generic type aaa\Foo<int, string> in PHPDoc tag @param for parameter $d does not specify all template types of class aaa\Foo: T, U, V. I'm not even sure why it says aaa\Foo<int, string>. The original type is Foo<1>. The string at the 2nd position looks like a bug.

Personally, I'd like to reuse the same message pattern the function call check has:

'Function ' . $functionName . ' invoked with %d parameter, %d required.',
'Function ' . $functionName . ' invoked with %d parameters, %d required.',
'Function ' . $functionName . ' invoked with %d parameter, at least %d required.',
'Function ' . $functionName . ' invoked with %d parameters, at least %d required.',
'Function ' . $functionName . ' invoked with %d parameter, %d-%d required.',
'Function ' . $functionName . ' invoked with %d parameters, %d-%d required.',
. So the example from 3) should say something like:

Generic type aaa\Foo<int> in PHPDoc tag @param for parameter $d does not specify all template types of class aaa\Foo: T, U, V, 2-3 required..

Of course, the part , 2-3 required. should only go in when the class has at least one optional @template, for backward compatibility.

  1. @template tag with default does not make sense above a function, but leads to no error message:
/**
 * @template T of Foo = Foo
 * @param T $d
 */
function foo(Foo $d): void
{

}

This should be caught similarly how @template-covariant is caught: "Variance annotation is only allowed for type parameters of classes and interfaces..."

@jiripudil
Copy link
Contributor Author

jiripudil commented Sep 26, 2024

I just checked out the branch and I'm trying to break it locally. So far I found:

Yes please, keep them coming :)

I agree with your suggestions in 1, 2, and 3, and I'm going to take a look at them.

The string at the 2nd position looks like a bug.

These messages use the typeOnly verbosity to describe types, so while the template is resolved to its default ('foo'), ConstantStringType then describes itself as string:

static fn (): string => 'string',

@template tag with default does not make sense above a function, but leads to no error message:

Default types are useful even above functions/methods, I'll add a test case :)

@VincentLanglet
Copy link
Contributor

I think it may also close phpstan/phpstan#7105

Could this https://phpstan.org/r/b999dd1e-983e-4dd1-b22d-ee5c518cd79a be supported ?
The idea was to copy array behavior which have the "key" template optional.

But it kinda go against

  1. This should report an error (the default in the middle doesn't make sense):
/**
 * @template T of int
 * @template U of string = 'foo'
 * @template V of string
 */
class Foo
{

}

rvanvelzen and others added 10 commits October 7, 2024 14:47

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@jiripudil jiripudil force-pushed the template-default branch 2 times, most recently from 6da89de to d17d2fe Compare October 7, 2024 13:07
@ondrejmirtes
Copy link
Member

It's brilliant, I love it, thank you :)

@ondrejmirtes ondrejmirtes merged commit f9a2648 into phpstan:1.12.x Oct 9, 2024
472 of 499 checks passed
@ondrejmirtes
Copy link
Member

I feel like we need to put this somewhere in the docs. In another of endless attempts to explain generics to people, I think we should create a brief "generics reference" in this place https://phpstan.org/writing-php-code/phpdoc-types#generics that would just list all the possible syntax in a dry way.

@ondrejmirtes
Copy link
Member

Could you please try send a PR that merges 1.12.x into 2.0.x? I'd expect the PR to contain a merge commit. It'd help me a lot, thanks!

I think the only thing you should be aware of is that I removed __set_state everywhere.

Also, you can run bin/make-optional-parameters-required.php and commit it for relevant places that you added here. Thanks!

@jiripudil jiripudil deleted the template-default branch October 9, 2024 09:32
@jiripudil
Copy link
Contributor Author

I feel like we need to put this somewhere in the docs. In another of endless attempts to explain generics to people, I think we should create a brief "generics reference" in this place https://phpstan.org/writing-php-code/phpdoc-types#generics that would just list all the possible syntax in a dry way.

Yep, I haven't yet figured out how to explain generics and not lose the audience (or get lost myself) along the way. Perhaps a dry reference could be a good starting point

Could you please try send a PR that merges 1.12.x into 2.0.x?

👉 #3560

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.

Add support for template default types
6 participants