String that are representations of integers are transformed into integers when used as array keys

[mathroc]

🐞 Describe the Bug

That's a tricky PHP behavior, but this array ["10" => 1] is evaluated as [10 => 1]

🔄 Steps to Reproduce

running mago analyse on the code sample doesn't detect an issue but executing that code will actually throw a TypeError

⚙️ Configuration (mago.toml)

# Mago configuration file
# For more information, see https://mago.carthage.software/#/getting-started/configuration
php_version = "8.4.0"


[source]
paths = ["src/"]


[format]
print_width = 120
tab_width = 4
use_tabs = false

[linter]
default_plugins = true
plugins = ["symfony", "php-unit"]

📜 Command Output

dev ➜ /var/www (mago) $ MAGO_LOG=trace mago analyse index2.php
DEBUG Configuration specifies 16 threads.
DEBUG Configuration specifies a stack size of 37748736 bytes.

╔════════════════════════════════════════════════════════════════════════════════╗
║                           ⚠️  EXPERIMENTAL ANALYZER ⚠️                           ║
╠════════════════════════════════════════════════════════════════════════════════╣
║ The analyzer is a work in progress. You may encounter:                         ║
║   - False positive errors in your code.                                        ║
║   - Unexpected panics or crashes.                                              ║
║                                                                                ║
║ Please report any issues to the Mago repository:                               ║
║   https://github.com/carthage-software/mago/issues                             ║
╚════════════════════════════════════════════════════════════════════════════════╝

DEBUG Compiling codebase...
TRACE Populating codebase with symbol references...
TRACE Codebase population complete.
DEBUG Analyzing sources...
TRACE Analyzing source `index2.php`...
TRACE Analysis for source `index2.php` completed in 0.000236604 seconds, with 0 issues found.
DEBUG Analysis completed for 1 sources.
 INFO No issues found.
dev ➜ /var/www (mago) $ php index2.php

Fatal error: Uncaught TypeError: h(): Return value must be of type true, false returned in /var/www/index2.php:10
Stack trace:
#0 /var/www/index2.php(15): h('10')
#1 {main}
  thrown in /var/www/index2.php on line 10

📂 PHP Code Sample (If Applicable)

<?php

declare(strict_types=1);

function h(string $s): true
{
    $a = [$s => 1];

    foreach ($a as $k => $v) {
        return is_string($k);
    }
}


h("10");

🖥️ Operating System

Linux

📦 How did you install Mago?

Homebrew (brew install mago)

📝 Additional Context

good luck with that one, I'm not sure how it should be solved 😅

[azjezz]

This is easy to catch, but the problem is would it make sense to do so.

we know numeric strings will be coerced to integers, but non numeric ones won't, so we can infer the key as string|int, but this might annoy people who are confident they strings are never numeric, and we currently do not have a flag on the string type that says "never numeric", unless it is a literal string in which case we can tell

[mathroc]

yes, that's exactly the issue, it may work to introduce a "non-integer-string", but that becomes cumbersome. (And should it also have all the non-empty- and lowercase- variant?)

but I wanted to raise this anyway since we've been bitten by this a few times, e.g. an array representing an email headers as a array<non-empty-string,array<string>|string when iterating over the keys, we had a case where we received an email that had a header named "0", does it make sense? probably not, but that broke that email delivery for a while 😓

anyway, feel free to close if it's too edge-case-y

[azjezz]

when iterating over the keys, we had a case where we received an email that had a header named "0", does it make sense? probably not, but that broke that email delivery for a while 😓

not sure if the solution above will even fix this, array<non-empty-string, array<string>|string> is a valid type, we can't say that it is impossible, and keys are string|int.

However, we could introduce these two behaviors behind a feature flag, that way it won't bother people, and you would still have the option for more aggresive assertion.

[azjezz]

yes, that's exactly the issue, it may work to introduce a "non-integer-string", but that becomes cumbersome. (And should it also have all the non-empty- and lowercase- variant?)

Adding a new type is actually the easy part, it's just about introducing a new flag here

mago/crates/codex/src/ttype/atomic/scalar/string.rs

Lines 21 to 32 in ba0a2f7

	pub struct TString {
	/// Describes the literal nature, if known. `None` means not known to be literal (general string).
	pub literal: Option<TStringLiteral>,
	/// Is this string *guaranteed* (by analysis or literal value) to be numeric according to PHP rules?
	pub is_numeric: bool,
	/// Is this string *guaranteed* (by analysis or literal value) to be truthy (non-empty and not "0")?
	pub is_truthy: bool,
	/// Is this string *guaranteed* (by analysis or literal value) to be non-empty?
	pub is_non_empty: bool,
	/// Is this string guaranteed to be lowercase (e.g., from a literal like "hello")?
	pub is_lowercase: bool,
	}

or turning pub is_numeric: bool into pub is_numeric: Option<bool>

[mathroc]

array<non-empty-string, array<string>|string> is a valid type, we can't say that it is impossible

absolutely

and keys are string|int.

well, that's fair 🤔

when I filled the issue, what I would have liked is that this code:

function f(string $headerName): mixed {
  // whatever
}

/**
 * @param array<non-empty-string,string|list<string>> $headers
 */
function f(array $headers): void {
  foreach ($headers as $headerName => $headerValue) {
    f($headerName);
  }
}

triggers a warning saying that $headerName is int|non-empty-string but f() expects a string

But you're right, maybe we can take a different approach and assume that array<non-empty-string, array<string>|string> indeed produces only string keys, that moves the problem at when we add keys to the array

adding a string key to a array<string,T> array would make it array<string|int,T> since you can't guarantee that the key won't be cast into an int

and instead the $headers array should be typed as array<non-empty-string|int,…>

---

btw, I think "is_numeric" is not the correct flag, is_numeric("3.5") returns true, but it stays a string when used as an array key

🙈