Warn when queries cannot be resolved unexpectedly

[hemberger]

Preface

While I think this represents a fully complete proposal, I had to make a lot of assumptions about how you might want this to be implemented. As such, I'm more than happy to modify it in any way you'd like -- or you're certainly welcome to edit this PR. The warning/tip messages would definitely benefit from your attention. If there's anything else you need from me, please let me know!

---

Fixes #504.

This adds new functionality to debug mode to warn about queries that cannot be resolved due to the presence of non-literal string variables in the query. This helps to identify places where you might expect a query to be resolvable, but an inline parameter or a variable query fragment prevent it.

Some scenarios where this might occur include string query fragments where using @phpstandba-inference-placeholder may be needed, e.g.

/** @var string $condition */
$query = 'SELECT * FROM table WHERE ' . $condition;

Or when not using prepared statements, e.g.

/** @var string $param */
$query = 'SELECT * FROM table WHERE foo=' . $conn->quote($param);

Since these queries cannot be analyzed, they will now emit the following warning:

Unresolvable Query: Cannot resolve query with variable type: string.
Tip: Consider replacing variable strings with prepared statements or @phpstandba-inference-placeholder.

Without this warning, errors in these queries will go undetected.

Implementation notes:

1. We make UnresolvableQueryException abstract, and add child classes for each type of unresolvable query error so that we can provide customized tips for each one.

2. Special care had to be taken to retain the ability to ignore queries that are entirely StringType, because it was explicitly desired for these queries to not return an error (even in debug mode). This is because such queries are likely part of a s/w layer that we know nothing about. Therefore, we early return before checking parameters for any query that is is a superset of StringType (which includes the previous MixedType condition, and any mix of the two). See cover more unresolvable-query cases #165.

[staabm]

I figure this one is hard to read - especially one does not immediately see that Mixed is contained.

wdyt about adding a new method on QueryReflection->isResolvable(Type):bool which can be asked about a given type is resolvable/analyzable? this would also have the benefit that custom implemented rules can re-use the logic without re-implementing stuff over and over.

[hemberger]

I think that's a good suggestion. I've pushed a fix that hopefully addresses this request. Let me know what you think.

[staabm]

I would prefer something like

Suggested change

	$isStringOrMixed = $type->isSuperTypeOf(new StringType());
	return $isStringOrMixed->negate();
	if ($type instanceof MixedType) {
	return TrinaryLogic::createNo();
	}
	return $type->isString();

[staabm]

I am pretty sure the logic in my suggestion is wrong, but I think you get the point.

otherwise the PR looks good, thank you

[hemberger]

So I'm actually having a bit of trouble with this logic. Do you think you could give me some pointers?

Basically, the logic that I'm trying to encapsulate is that if any part of the string is a literal string, then we don't want to skip the query, because there is literal SQL that we should be checking. Then either it will resolve and be checked, or we'll warn that it doesn't resolve (with this PR). For example, I'd want the warning for the following:

/** @var string $string */
$query = 'SELECT * FROM table WHERE ' . $string;

Pre-existing tests explicitly state that entirely non-literal string and mixed types should not be checked, e.g.:

/** @var string $string */
$query = $string;

with the reasoning that this is likely part of a s/w abstraction layer that phpstan-dba won't know about, which is reasonable.

But how does one distinguish between $string and $literal . $string? I was thinking that the former is a StringType and the latter is an IntersectionType, but many non-literal string variables are an IntersectionType with an Accessory, e.g. if $string is a non-empty-string. Is there any way to robustly do what I'm trying to do with the current phpstan type system, or is my idea fundamentally flawed?

To be clear, the edge cases that my attempts fail at look like, e.g.

/** @var non-empty-string $string */
$query = $string;

I still think this is an important feature, especially for people modernizing old codebases that didn't use prepared queries. Enabling this warning has uncovered many unresolvable queries that I didn't find by hand (including ones that were already migrated to prepared statements, but unexpectedly used a non-literal string in the query construction).

If there is no such solution, I would honestly be totally happy with an option to just warn for all unresolvable queries (with the idea that it's safer to explicitly ignore an unwanted warning than silently miss a bunch of real errors). But I look to you for guidance here, before I go too far off the deep end. :)

Thanks again for your time!

[staabm]

I agree with the goal and the results of this PR.

my suggestion was more about how to make the logic in isResolvable more readable without changing what it is doing.

but since you added extensive test coverage, I will just merge it, after merge conflicts are resolved.

sorry for the noise

[staabm]

thank you