Skip to content

Commit bb7edee

Browse files
rlerdorfrlerdorf
authored
addresses #5542 (#5570)
* addresses #5542 * address review * address review * Add GlobalConstantDeclarationUndoTest
1 parent ce37a68 commit bb7edee

12 files changed

Lines changed: 433 additions & 29 deletions

‎NEWS.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ Phan 6.0.8-dev
44
--------------
55

66
Bug fixes:
7+
- Constants declared with `define()` no longer have their real type erased, and `constant('NAME')` no longer returns `mixed` for them. This detects e.g. `PhanImpossibleTypeComparison` for `STRING_CONST === $int`. The real type is widened to the non-literal type (`false` becomes `bool`, `120` becomes `int`) so that conditions on configuration constants such as `if (DEBUG_MODE)` are still not reported as redundant/impossible. When the same constant is passed to `define()` in several places (e.g. both branches of an `if`/`else`), the types of all of them are now unioned instead of only using the first one seen ([#5542](https://github.com/phan/phan/issues/5542))
78
- Fix false positives (e.g. `PhanTypeMismatchArgumentNullable`, `PhanPluginNeverReturnMethod`) when a branch calls a `never`-returning instance method on a local variable assigned with `new ClassName()` — the branch is now correctly treated as unreachable ([#5553](https://github.com/phan/phan/issues/5553))
89
- Fix crash (`ArgumentCountError: Too few arguments to function Phan\Language\Type::isCallable()`) when `is_callable()` narrowing is applied to a value with an intersection type, e.g. `(callable&T)|null` ([#5555](https://github.com/phan/phan/issues/5555), [#5557](https://github.com/phan/phan/pull/5557))
910
- Fix `@var Foo<T>` losing its template parameter on properties that also have a native type declaration (e.g. `/** @var Box<A> */ private Box $box;` was inferred as `Box|Box<A>` instead of `Box<A>`, making method calls on `$box` return `mixed` instead of the template argument) ([#5556](https://github.com/phan/phan/issues/5556))

‎phpunit.xml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@
8080
<file>tests/Phan/CLITest.php</file>
8181
<file>tests/Phan/ConfigTest.php</file>
8282
<directory>tests/Phan/Config</directory>
83+
<file>tests/Phan/GlobalConstantDeclarationUndoTest.php</file>
8384
</testsuite>
8485
</testsuites>
8586
<source>

‎src/Phan/CodeBase.php‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1506,11 +1506,39 @@ public function addGlobalConstant(GlobalConstant $global_constant): void
15061506
if ($this->undo_tracker) {
15071507
$this->undo_tracker->recordUndo(static function (CodeBase $inner) use ($global_constant): void {
15081508
Daemon::debugf("Undoing addGlobalConstant on %s\n", $global_constant->getFQSEN());
1509-
unset($inner->fqsen_global_constant_map[$global_constant->getFQSEN()]);
1509+
$inner->removeGlobalConstantDeclaration($global_constant->getFQSEN(), $global_constant->getDeclarationKey());
15101510
});
15111511
}
15121512
}
15131513

1514+
/**
1515+
* Removes a single declaration of a global constant (used to undo parsing a file in daemon mode).
1516+
*
1517+
* A constant declared with `define()` in several places tracks the additional declarations as alternates
1518+
* of the first one seen. If the declaration being removed is the one in the code base,
1519+
* an alternate declaration (from a file that was not changed) is promoted to replace it, if there is one.
1520+
*
1521+
* @param string $declaration_key the declaration key of the define() call (see GlobalConstant::getDeclarationKey()), or '' for a `const`
1522+
*/
1523+
public function removeGlobalConstantDeclaration(FullyQualifiedGlobalConstantName $fqsen, string $declaration_key): void
1524+
{
1525+
if (!$this->fqsen_global_constant_map->offsetExists($fqsen)) {
1526+
return;
1527+
}
1528+
$constant = $this->fqsen_global_constant_map[$fqsen];
1529+
if ($constant->getDeclarationKey() !== $declaration_key) {
1530+
$constant->removeAlternateDeclaration($declaration_key);
1531+
return;
1532+
}
1533+
$replacement = $constant->promoteAlternateDeclaration();
1534+
if ($replacement) {
1535+
Daemon::debugf("Promoting alternate declaration of %s from %s\n", $fqsen, $replacement->getDeclarationKey());
1536+
$this->fqsen_global_constant_map[$fqsen] = $replacement;
1537+
} else {
1538+
unset($this->fqsen_global_constant_map[$fqsen]);
1539+
}
1540+
}
1541+
15141542
/**
15151543
* @return bool
15161544
* True if a a global constant with the given FQSEN exists

‎src/Phan/Language/Element/GlobalConstant.php‎

Lines changed: 177 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,16 @@
44

55
namespace Phan\Language\Element;
66

7+
use Closure;
78
use InvalidArgumentException;
89
use Phan\AST\ASTReverter;
910
use Phan\Exception\FQSENException;
1011
use Phan\Language\Context;
1112
use Phan\Language\FQSEN\FullyQualifiedGlobalConstantName;
1213
use Phan\Language\Type;
1314
use Phan\Language\Type\BoolType;
15+
use Phan\Language\Type\FalseType;
16+
use Phan\Language\Type\TrueType;
1417
use Phan\Language\UnionType;
1518
use Phan\Library\StringUtil;
1619

@@ -20,7 +23,9 @@
2023
*/
2124
class GlobalConstant extends AddressableElement implements ConstantInterface
2225
{
23-
use ConstantTrait;
26+
use ConstantTrait {
27+
createRestoreCallback as private createConstantRestoreCallback;
28+
}
2429

2530
/** @var array<string,true> names of internal boolean constants whose values vary between builds */
2631
private const VOLATILE_BOOLEAN_CONSTANTS = [
@@ -31,7 +36,33 @@ class GlobalConstant extends AddressableElement implements ConstantInterface
3136
];
3237

3338
/**
34-
* Sets whether this is a global constant that should be treated as if the real type is unknown.
39+
* @var array<string,GlobalConstant>
40+
* Additional `define()` calls for this constant (other than the first one seen),
41+
* keyed by their declaration key (see getDeclarationKey()).
42+
*
43+
* PHP only allows a constant to be defined once, but when `define()` is used in mutually
44+
* exclusive branches (e.g. `if ($x) { define('C', 1); } else { define('C', true); }`),
45+
* any one of them may be the definition at runtime, so the union of all of their types is used.
46+
*/
47+
private array $alternate_declarations = [];
48+
49+
/**
50+
* @var string identifies the `define()` call that declared this constant ("file:line:hash of value"),
51+
* or '' for constants declared with `const` or by PHP.
52+
*/
53+
private string $declaration_key = '';
54+
55+
/**
56+
* @var UnionType|null the cached union of this constant's type and the types of all alternate declarations.
57+
*/
58+
private ?UnionType $combined_union_type = null;
59+
60+
/**
61+
* Sets whether this is a global constant that was declared with `define()` instead of `const`.
62+
*
63+
* The real type of a dynamic constant is widened to the non-literal type (e.g. `false` becomes `bool`),
64+
* because `define()` is typically used for configuration values that vary between deployments,
65+
* and warning about redundant/impossible conditions on those would be noise.
3566
*/
3667
public function setIsDynamicConstant(bool $dynamic_constant): void
3768
{
@@ -46,7 +77,7 @@ public function setIsDynamicConstant(bool $dynamic_constant): void
4677

4778
/**
4879
* @return bool
49-
* True if this is a global constant that should be treated as if the real type is unknown.
80+
* True if this is a global constant that was declared with `define()` instead of `const`.
5081
*/
5182
public function isDynamicConstant(): bool
5283
{
@@ -62,20 +93,159 @@ public function getUnionType(): UnionType
6293
if (null !== ($union_type = $this->getFutureUnionType())) {
6394
$this->setUnionType($union_type);
6495
}
96+
if (!$this->alternate_declarations) {
97+
return parent::getUnionType();
98+
}
99+
return $this->combined_union_type ??= $this->computeCombinedUnionType();
100+
}
65101

66-
return parent::getUnionType();
102+
private function computeCombinedUnionType(): UnionType
103+
{
104+
$result = parent::getUnionType();
105+
foreach ($this->alternate_declarations as $alternate) {
106+
$result = $result->withUnionType($alternate->getUnionType());
107+
}
108+
return $result;
67109
}
68110

69111
public function setUnionType(UnionType $type): void
70112
{
71-
// Note: in php 8.1, constants can also be objects if those objects are enums.
72-
// So the real type set includes every type.
73113
if ($this->isDynamicConstant()) {
74-
$type = $type->eraseRealTypeSet();
114+
$type = self::widenDynamicConstantType($type);
75115
}
116+
$this->combined_union_type = null;
76117
parent::setUnionType($type);
77118
}
78119

120+
/**
121+
* Sets a key identifying the `define()` call which declared this constant, e.g. "file:line:hash of the value".
122+
* The same call is re-registered in the analysis phase and gets the same key.
123+
*/
124+
public function setDeclarationKey(string $key): void
125+
{
126+
$this->declaration_key = $key;
127+
}
128+
129+
/**
130+
* @return string a key identifying the `define()` call which declared this constant, or '' if this was not declared with `define()`.
131+
*/
132+
public function getDeclarationKey(): string
133+
{
134+
return $this->declaration_key;
135+
}
136+
137+
/**
138+
* Records an additional `define()` call for this constant (in a different location from the one which declared it).
139+
* A declaration with the same declaration key replaces the previously recorded one.
140+
*/
141+
public function addAlternateDeclaration(GlobalConstant $alternate): void
142+
{
143+
$this->alternate_declarations[$alternate->declaration_key] = $alternate;
144+
$this->combined_union_type = null;
145+
}
146+
147+
/**
148+
* Forgets the alternate declaration with the given declaration key (used to undo parsing a file in daemon mode).
149+
*/
150+
public function removeAlternateDeclaration(string $key): void
151+
{
152+
unset($this->alternate_declarations[$key]);
153+
$this->combined_union_type = null;
154+
}
155+
156+
/**
157+
* Copies the alternate declarations from $other (e.g. when this constant replaces $other in the code base).
158+
*/
159+
public function copyAlternateDeclarationsFrom(GlobalConstant $other): void
160+
{
161+
if ($other === $this || !$other->alternate_declarations) {
162+
return;
163+
}
164+
$this->alternate_declarations = $other->alternate_declarations + $this->alternate_declarations;
165+
unset($this->alternate_declarations[$this->declaration_key]);
166+
$this->combined_union_type = null;
167+
}
168+
169+
/**
170+
* Removes and returns the first alternate declaration, giving it the remaining alternate declarations
171+
* (and the references to this constant), so that it can replace this constant in the code base.
172+
* Used when the file which declared this constant is changed in daemon mode but other files still define it.
173+
*/
174+
public function promoteAlternateDeclaration(): ?GlobalConstant
175+
{
176+
$key = \array_key_first($this->alternate_declarations);
177+
if ($key === null) {
178+
return null;
179+
}
180+
$replacement = $this->alternate_declarations[$key];
181+
unset($this->alternate_declarations[$key]);
182+
$replacement->alternate_declarations = $this->alternate_declarations;
183+
$replacement->combined_union_type = null;
184+
$replacement->copyReferencesFrom($this);
185+
$this->alternate_declarations = [];
186+
$this->combined_union_type = null;
187+
return $replacement;
188+
}
189+
190+
/**
191+
* Used by daemon mode (without pcntl) to restore this constant and its alternate declarations
192+
* to the state they had before analysis.
193+
* @internal
194+
*/
195+
public function createRestoreCallback(): ?Closure
196+
{
197+
$restore_own_type = $this->createConstantRestoreCallback();
198+
$alternates = $this->alternate_declarations;
199+
if (!$alternates) {
200+
return $restore_own_type;
201+
}
202+
$alternate_callbacks = [];
203+
foreach ($alternates as $alternate) {
204+
$callback = $alternate->createRestoreCallback();
205+
if ($callback) {
206+
$alternate_callbacks[] = $callback;
207+
}
208+
}
209+
return function () use ($restore_own_type, $alternates, $alternate_callbacks): void {
210+
if ($restore_own_type) {
211+
$restore_own_type();
212+
}
213+
foreach ($alternate_callbacks as $callback) {
214+
$callback();
215+
}
216+
$this->alternate_declarations = $alternates;
217+
$this->combined_union_type = null;
218+
};
219+
}
220+
221+
/**
222+
* Widens the real type set of a constant declared with `define()` to non-literal types
223+
* (e.g. `3` to `int`, `'name'` to `string`, `true` to `bool`).
224+
*
225+
* The phpdoc type keeps the literal value so that it can still be used e.g. for array shape keys.
226+
* The real type is widened because `define()` is typically used for values that vary between
227+
* deployments (feature flags, environment-specific settings, version constants),
228+
* where warnings about redundant/impossible conditions such as `if (DEBUG_MODE)` would be noise.
229+
* The non-literal real type is still enough to detect impossible comparisons between different types.
230+
*/
231+
public static function widenDynamicConstantType(UnionType $type): UnionType
232+
{
233+
$real_type_set = $type->getRealTypeSet();
234+
if (!$real_type_set) {
235+
return $type;
236+
}
237+
$new_real_type_set = [];
238+
foreach ($real_type_set as $real_type) {
239+
if ($real_type instanceof TrueType || $real_type instanceof FalseType) {
240+
$real_type = BoolType::instance($real_type->isNullable());
241+
} else {
242+
$real_type = $real_type->asNonLiteralType();
243+
}
244+
$new_real_type_set[] = $real_type;
245+
}
246+
return $type->withRealTypeSet(UnionType::getUniqueTypes($new_real_type_set));
247+
}
248+
79249
/**
80250
* @return FullyQualifiedGlobalConstantName
81251
* The fully-qualified structural element name of this

‎src/Phan/Parse/ParseVisitor.php‎

Lines changed: 55 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use ast\Node;
1010
use InvalidArgumentException;
1111
use Phan\Analysis\ScopeVisitor;
12+
use Phan\AST\ASTHasher;
1213
use Phan\AST\ASTReverter;
1314
use Phan\AST\ContextNode;
1415
use Phan\AST\UnionTypeVisitor;
@@ -1925,17 +1926,37 @@ public static function addConstant(
19251926
// define() is typically used to conditionally set constants or to set them to variable values.
19261927
// TODO: Could add 'configuration_constant_set' to add additional constants to treat as dynamic such as PHP_OS, PHP_VERSION_ID, etc. (convert literals to non-literal types?)
19271928
$constant->setIsDynamicConstant($is_fully_qualified);
1929+
if ($is_fully_qualified) {
1930+
// Include a hash of the value so that multiple define() calls on the same line get distinct keys,
1931+
// while the analysis phase re-registering the same call (with the same value) gets the same key.
1932+
$constant->setDeclarationKey($context->getFile() . ':' . $lineno . ':' . self::hashDefineValue($value));
1933+
}
19281934

1935+
// The constant with the same name which is already declared, if this is an additional define() call for it.
1936+
$primary_constant = null;
19291937
if ($code_base->hasGlobalConstantWithFQSEN($fqsen)) {
19301938
$other_constant = $code_base->getGlobalConstantByFQSEN($fqsen);
1931-
$other_context = $other_constant->getContext();
1932-
if (!$other_context->equals($context)) {
1933-
// Be consistent about the constant's type and only track the first declaration seen when parsing (or redeclarations)
1934-
// Note that global constants don't have alternates.
1935-
return;
1939+
$is_other_define = $is_fully_qualified && $other_constant->isDynamicConstant() && !$other_constant->isPHPInternal();
1940+
if ($is_other_define) {
1941+
$is_same_declaration = $other_constant->getDeclarationKey() === $constant->getDeclarationKey();
1942+
} else {
1943+
$is_same_declaration = $other_constant->getContext()->equals($context);
1944+
}
1945+
if (!$is_same_declaration) {
1946+
if (!$is_other_define) {
1947+
// Only track the first declaration seen when parsing (or redeclarations of it in the analysis phase).
1948+
// Note that global constants don't have alternates.
1949+
return;
1950+
}
1951+
// This is a different define() call for a constant that was already declared with define()
1952+
// (e.g. in the other branch of an if/else, or in a different config file).
1953+
// Any of the define() calls may be the one that runs, so union the types of all of them.
1954+
$primary_constant = $other_constant;
1955+
} else {
1956+
// Keep track of old references to the new constant
1957+
$constant->copyReferencesFrom($other_constant);
1958+
$constant->copyAlternateDeclarationsFrom($other_constant);
19361959
}
1937-
// Keep track of old references to the new constant
1938-
$constant->copyReferencesFrom($other_constant);
19391960

19401961
// Otherwise, add the constant now that we know about all of the elements in the codebase
19411962
}
@@ -1973,14 +1994,38 @@ public static function addConstant(
19731994
$constant->setIsDeprecated($comment->isDeprecated());
19741995
$constant->setIsNSInternal($comment->isNSInternal());
19751996

1976-
$code_base->addGlobalConstant(
1977-
$constant
1978-
);
1997+
if ($primary_constant) {
1998+
$primary_constant->addAlternateDeclaration($constant);
1999+
$undo_tracker = $code_base->getUndoTracker();
2000+
if ($undo_tracker) {
2001+
$key = $constant->getDeclarationKey();
2002+
$undo_tracker->recordUndo(static function (CodeBase $inner) use ($fqsen, $key): void {
2003+
$inner->removeGlobalConstantDeclaration($fqsen, $key);
2004+
});
2005+
}
2006+
} else {
2007+
$code_base->addGlobalConstant(
2008+
$constant
2009+
);
2010+
}
19792011

19802012
// Track constant declaration for incremental analysis
19812013
DependencyTracker::track($fqsen->__toString(), 'declares');
19822014
}
19832015

2016+
/**
2017+
* @param Node|mixed $value the value passed to define()
2018+
* @return string a hash of the value expression, ignoring line numbers
2019+
*/
2020+
private static function hashDefineValue(mixed $value): string
2021+
{
2022+
if ($value instanceof Node || \is_int($value) || \is_float($value) || \is_string($value) || $value === null) {
2023+
return ASTHasher::hash($value);
2024+
}
2025+
// bool or resource (only for internal constants, which are not merged)
2026+
return \is_bool($value) ? ($value ? 'true' : 'false') : \gettype($value);
2027+
}
2028+
19842029
/**
19852030
* @return Clazz
19862031
* Get the class on this scope or fail real hard

0 commit comments

Comments
 (0)