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

Prevent "unable to resolve template type T" in conditional branches #1239

Merged

Conversation

rvanvelzen
Copy link
Contributor

Part of phpstan/phpstan#3853

This prevents the error for template types that are not part of both conditional branches.

It isn't perfect, but I couldn't figure out how to decide which branch would be taken. There is still the error regarding the incorrect passed type in many cases (as shown in the added test case), so it might not be a big deal.

@ondrejmirtes
Copy link
Member

I have an idea, I'm gonna try something :)

@ondrejmirtes ondrejmirtes force-pushed the template-type-in-conditional-branc branch from 0834597 to 72cd27b Compare April 22, 2022 11:33
@ondrejmirtes
Copy link
Member

So I tried something - I thought that we should use the same logic to resolve the conditional type as it's currently used for other reasons. The problem was that we're also resolving template types at the same type which caused the logic in FunctionCallParametersCheck to not see what was originally there.

I divided this in two steps - first resolve the conditionals (that's what the "original parameters acceptor" contains), and only then resolve the template types.

@ondrejmirtes
Copy link
Member

I've attempted to do an additional commit - a cleanup which I will push now, but it breaks some cases - probably because the conditional is resolved at the wrong time and the template type needed for that is not yet known.

@ondrejmirtes
Copy link
Member

Could you please try to fix what's wrong with 478aa27?diff=unified&w=1? Or just tell me it's impossible and the previous commit is fine. But I don't like that this code is duplicated and executed twice:

			if ($type instanceof ConditionalTypeForParameter || $type instanceof ConditionalType) {
				$type = $traverse($type);

				if ($type instanceof ConditionalTypeForParameter && array_key_exists($type->getParameterName(), $passedArgs)) {
					$type = $type->toConditional($passedArgs[$type->getParameterName()]);
				}

				if ($type instanceof ConditionalType) {
					return $type->resolve();
				}

				return $type;
			}

@rvanvelzen
Copy link
Contributor Author

Yeah, the issue there is that the conditional needs the template types to be resolved to know which branch to take. However, ConditionalTypeForParameter will always be resolved after that (or not be resolvable at all), so only ConditionalType needs to be resolved. I agree that it's not nice though

@rvanvelzen
Copy link
Contributor Author

I have some ideas, will update soon

@rvanvelzen
Copy link
Contributor Author

Naming aside this seems like a reasonable way forward to me

@ondrejmirtes
Copy link
Member

I really like this! Please mark this as ready, I'll merge it :)

@rvanvelzen rvanvelzen marked this pull request as ready for review April 22, 2022 16:57
@ondrejmirtes ondrejmirtes merged commit 048a458 into phpstan:1.6.x Apr 22, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@rvanvelzen rvanvelzen deleted the template-type-in-conditional-branc branch April 25, 2022 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants