Proposal: sniff for invisible characters in hard-coded identifier names

[Otzie2023]

Background

PHP's scanner defines identifiers on bytes, not code points:

LABEL  [a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*

Any byte >= 0x80 is accepted, so a character that renders as nothing, or as
whitespace, becomes part of the name. $x and $x followed by U+00A0 are two
distinct variables that look identical in every editor.

This came out of a concept thread on internals
(https://news-web.php.net/php.internals/132344). The conclusion there was that
a language change was not the right vehicle and that this belongs in tooling.
@jrfnl invited me to bring it here.

What it would flag

Identifier tokens whose text contains a character that is invisible or renders
as whitespace. Live example, from the Alipay OpenAPI SDK, vendored into four
unrelated projects in my sample:

$chrtext\u{00A0} = null;
// ...
openssl_public_encrypt($block, $chrtext\u{00A0}, $res);

It works only because the typo is consistent throughout the function. Anyone
typing $chrtext normally gets a silently different variable, passed by
reference, that stays null.

How common

From a survey of the 5,000 most-downloaded Packagist packages, 4,863 resolvable,
520,802 PHP files (tooling and raw data: https://github.com/Otzie2023/PHP):

Non-ASCII identifiers	1,447
Containing an invisible character	68
Files scanned	520,802

Not all 68 are mistakes. markrogoyski/math-php writes variable names such as
⟮1<U+00A0>−<U+00A0>p⟯ˣ, with U+00A0 deliberately inside the name. That
matters for the design, below.

Proposed character set

Default_Ignorable_Code_Point, plus the format, control and separator general
categories. Version-independent and small — 17 ranges:

00AD, 034F, 061C, 115F..1160, 17B4..17B5, 180B..180F, 200B..200F,
202A..202E, 2060..206F, 3164, FE00..FE0F, FEFF, FFA0, FFF0..FFF8,
1BCA0..1BCA3, 1D173..1D17A, E0000..E0FFF

plus general categories Cf, Cc, Cs, Co, Cn, Zs, Zl, Zp. That
catches U+00A0 (which is Zs, not Default_Ignorable), the zero-width joiners,
the variation selectors and U+3000 IDEOGRAPHIC SPACE, which also occurs in the
corpus.

Deliberately not included: general UAX #31 conformance, NFC, or confusable
scripts. Those are separate questions with different answers, and I would
rather not smuggle them in here.

Design points I have an opinion on

No fixer. Stripping the character from one occurrence renames that
occurrence and breaks the code, because the other occurrences carry the same
character. It has to be reported and corrected by hand.

Warning, not error. math-php's usage is deliberate. A sniff that errors on
it would be wrong about that code, and the ratio in the corpus does not justify
error severity.

ASCII fast path. The first check is for any byte >= 0x80; an identifier of
pure ASCII returns immediately. That is every identifier in almost every file.

No ext/mbstring. PHPCS requires ext-tokenizer but not ext-mbstring.
preg_match('//u', $s) validates UTF-8 through PCRE, and code point iteration
can go through preg_split('//u', ...), so no new extension dependency is
needed. I have this working in the survey scanner.

Questions for you

1. Where does it belong — Generic/Sniffs/NamingConventions/, or somewhere
   else? Generic/Files/ByteOrderMarkSniff looked like the closest structural
   model.
2. Should it also cover hard-coded names as opposed to identifiers:
   ${'...'}, ->{'...'}, define(), class_alias() and friends? PHP treats
   names and identifiers as different things, and a name may legitimately be
   anything. In my corpus only 5 non-ASCII names of that kind exist across all
   520,802 files, all deliberate, so it may not be worth the surface area.
3. Should the character set be configurable, or fixed?
4. Is a separate sniff for identifiers that are not well-formed UTF-8 worth
   having? There are 2 in the corpus — one is symfony/cache, which declares a
   class whose entire name is the single byte 0xA9.

Happy to open a PR once the shape is agreed.

[jrfnl]

@Otzie2023 Thank you for opening this issue to discuss the design ahead of the PR.

To start, let me first respond to your write up:

• How common: that's a good starting point to compare with to verify that the sniff works as intended. It may also yield some interesting cases to cover in the tests for the sniff 👍🏻
• Proposed character set: you are referencing a number of Unicode categories and unicode character identifiers. I think it might make things easier to discuss, and for other people to understand what we are talking about, if you could add some links (or additional detail) to the specifications for the categories/characters.
• Proposed character set: same with the UAX 31 reference, which now points to an issue in this repo, rather than to the spec you are referring to.
• No fixer: I 100% agree. I am also of the opinion that, for that same reason, a sniff should only flag the identifier names at the point of declaration (with variables potentially being the exception).
• Warning, not error: Again, I completely agree. Individual projects can always elevate this to an error via their ruleset if they so desire.
• ASCII fast path: that's an implementation detail, but one I support as performance for a CLI tool such as this is important.
• No ext/mbstring: Again, I agree, but I do have some caveats. PHPCS has a setting to tell it about the file encoding. This setting defaults to utf-8, but can be changed on the command line and via a custom ruleset. We may need to have a think about the implications of this, considering that PHP operates on the byte values, not the characters, while PCRE with the u modifier expects the $subject string to be utf-8 (which it may not be).
  PHP_CodeSniffer does already use the iconv extension (conditionally when available), so I suspect a choice may need to be made for the sniff to either not support scanning files with a non-utf8 encoding, or possibly to convert between encodings before passing the identifier name to PCRE (with the performance price to pay for that).
  I'd be interested to hear your thoughts on this.

Now, as for your questions:

1. Where does it belong — Generic/Sniffs/NamingConventions/, or somewhere else? Generic/Files/ByteOrderMarkSniff looked like the closest structural model.

I think we need to discuss which repo it belong in first.

There are basically two repos which contain sniffs in this organisation: this one and PHPCSExtra.

Pro of putting the sniff in this repo: users wanting to use the sniff would not need to add the extra dependency on PHPCSExtra.
Pro of putting the sniff in PHPCSExtra: the availability of the utility methods from PHPCSUtils, some of which will definitely make it more straight-forward to write this sniff.
For example: figuring out the name of a class constant has become a lot harder since the introduction of typed constants, but is a solved problem with the PHPCSUtils Constants::getProperties() method.

In either case, I think the NamingConventions category sounds like the correct one.

For PHPCS, the rule would belong in Generic, like you already surmised. For PHPCSExtra, the rule would belong in Universal (which is basically the counterpart of Generic).

I suggest you have a look at what PHPCSUtils has to offer and I look forward to hearing your thoughts on the repo positioning once you had a chance to look that over.

2. Should it also cover hard-coded names as opposed to identifiers: ${'...'}, ->{'...'}, define(), class_alias() and friends? PHP treats
   names and identifiers as different things, and a name may legitimately be
   anything. In my corpus only 5 non-ASCII names of that kind exist across all
   520,802 files, all deliberate, so it may not be worth the surface area.

I do think it would be interesting to include that, but not with the same priority. This can always be added at a later stage.

Note: define(), class_alias() and potentially other function calls would be relatively straight-forward to analyse by using the PHPCSUtils PassedParameters methods.

3. Should the character set be configurable, or fixed?

I don't have an opinion on this yet. I think we need to define the characters the sniff will look for in detail before decided this.

4. Is a separate sniff for identifiers that are not well-formed UTF-8 worth
   having? There are 2 in the corpus — one is symfony/cache, which declares a
   class whose entire name is the single byte 0xA9.

I do not think this needs a separate sniff. This can be handled in the same sniff, using a separate error code and I do think it would be worth having as I think that would be an error in most cases (providing the file encoding is utf-8...) and it will be helpful for projects to see this flagged.

I hope this feedback helps and I look forward to continuing this discussion.

[Otzie2023]

Thank you for your feedback

Links and the UAX #31 slip

For reference, these are the sources I was referring to:

• General categories (Cf, Cc, Cs, Co, Cn, Zs, Zl, Zp) —
  UAX #44, General_Category values,
  derived data in DerivedGeneralCategory.txt.
• Default_Ignorable_Code_Point —
  UAX #44,
  derived data in DerivedCoreProperties.txt.
• Identifier syntax generally — UAX #31.

One maintenance point that is probably worth documenting: UAX #31 §2.3 says that Default_Ignorable_Code_Point does not have the same stability guarantee as XID_Start / XID_Continue. So the range list would need to be refreshed when the UCD is updated. It has only grown occasionally, but it is not a frozen list.

Declaration versus usage

Agreed. And I think the variable exception you pointed out is more than just a convenience — it is really the important distinction here.

For classes, interfaces, traits, enums, functions, methods and constants, a usage containing an invisible character that does not match a declaration will fail loudly at runtime: class not found, undefined function, etc. The developer gets an error. So checking the declaration is enough.

Variables have no declaration, and a mistyped usage fails silently — it creates a new variable. That is exactly the Alipay case: $chrtext with a trailing U+00A0 is assigned, then passed by reference to openssl_public_encrypt(). It works only because every occurrence carries the character. Anyone typing the name normally gets a different variable that stays null, with no error anywhere.

So variables really do need to be checked at usage. To avoid producing N reports for the same rename, I’d lean towards reporting once per distinct name per file, at the first occurrence. I’m not particularly attached to that detail, though, so it’s easy to change.

Encoding

This is the interesting part, and I don’t think “UTF-8 only” is safe.

There are two things that narrow the problem down quite a bit.

First, only a small number of encodings can produce a non-ASCII identifier at all. The LABEL rule requires every byte to be >= 0x80, so encodings that use low bytes inside a multibyte sequence cannot carry one.

Checked against PHP 8.3:

Encoding	Example	Parses
UTF-8	表 = E8 A1 A8	yes
ISO-8859-1	ä = E4	yes
EUC-JP	表 = C9 BD	yes
Shift-JIS	表 = 95 5C	no — 0x5C is the backslash
UTF-16	embedded NUL	no

Big5 and GBK fall into the Shift-JIS category as well: their trail bytes can overlap with 0x40–0x7E.

But the actual hazard does exist outside UTF-8, which is why I don’t think we should simply declare non-UTF-8 out of scope:

File encoding	Character	Bytes	PHP parses	Valid UTF-8
ISO-8859-1	$chrtext + U+00A0	…74 A0	yes	no
EUC-JP	$x + U+3000	…78 A1 A1	yes	no
UTF-8	$chrtext + U+00A0	…74 C2 A0	yes	yes

A UTF-8-only sniff would report the first two cases as a malformed encoding problem rather than an invisible character problem. That feels like the wrong message on exactly the kind of issue this sniff is intended to catch.

So my proposal would be:

1. ASCII fast path first. If all bytes in the token are < 0x80, return immediately. This is what keeps the rest affordable: in the 5,000-package corpus, only 1,447 identifier tokens across 520,802 files even make it to step 2.
2. When $config->encoding is utf-8 (the default), validate the token with preg_match('//u', …). If it fails, report the malformed-name error code and stop there.
3. For other encodings, when iconv is available, convert just the token text to UTF-8 and run the same code-point checks. Because of step 1, this only happens for a handful of very short strings rather than every token, so the performance cost should be fairly small.
4. For other encodings without iconv, report nothing for that file. A wrong positive seems worse than missing the check.

PHPCS already guards iconv with function_exists() in src/Tokenizers/Tokenizer.php and in the XML-family reports, so this would be following an existing pattern rather than introducing a new one.

The caveat is that encoding is a per-run setting, not a per-file setting. So mixed-encoding projects can still get this wrong for some files. I don’t think the sniff can do better than the encoding it is given, but it is worth documenting.

Repo positioning

After looking through PHPCSUtils, my honest reading is that the utilities are doing real work here, rather than just being a convenience. So I’d put this in PHPCSExtra / Universal.

The main reason is that PHPCSUtils already models the exact semantics this sniff needs:

• NamingConventions::PHP_LABEL_REGEX is the LABEL rule itself.
• NamingConventions::isEqual() documents and implements PHP’s split case-sensitivity. Its own docblock says ASCII characters are case-insensitive while anything from 0x80 up is case-sensitive. That is exactly the byte-level behaviour the sniff has to account for, and it is already captured correctly in one place.
• ObjectDeclarations::getDeclaredConstants() and Constants::getProperties() handle the typed-constant name lookup you mentioned.
• PassedParameters::getParameter() covers the later define() / class_alias() phase cleanly.

Hand-rolling declaration-name detection across typed constants, enums, and whatever PHP 8.x adds next feels like exactly the kind of thing that can go subtly wrong. And if it goes wrong here, we end up missing precisely the declarations the sniff is supposed to catch.

I’d rather rely on code that already gets those semantics right.

That said, the dependency question is yours to weigh. I don’t have a strong view on how much extra friction that requirement creates for users.

Names as well as identifiers

Agreed that this is lower priority, and PassedParameters makes it relatively cheap to add later.

For completeness, the measurement is five non-ASCII hard-coded names across all 520,802 files, in two packages. All five are deliberate: three in halaxa/json-machine, where variables are named after the individual bytes of the UTF-8 BOM, and two in vendored rowbot/url WHATWG conformance tests.

So I wouldn’t prioritise that yet.

Malformed UTF-8

Agreed — same sniff, separate error code, error severity by default.

The qualification from the encoding section still applies: “malformed” really means “not well-formed in the encoding PHPCS was told the file uses”, and only step 2 or 3 above can establish that.

Character set, configurable or not

Agreed that this is something we should settle first.

My starting point would be the Default_Ignorable ranges plus the eight general categories above. In the corpus, that catches things like U+00A0 NO-BREAK SPACE, U+3000 IDEOGRAPHIC SPACE and U+200D ZERO WIDTH JOINER.

I’ll write out the full list, including the names and what each entry catches, in a separate comment so it can be reviewed on its own.

In the meantime, I’m happy to start working on tests from the corpus cases.

[Otzie2023]

Here is the character description in written form

Working definition: a code point that is invisible, or renders as whitespace, and can actually occur inside a PHP identifier. The second half matters — anything below U+0080 that is not [a-zA-Z0-9_] terminates the identifier, so only code points from U+0080 up are reachable. The list below is therefore restricted to those.

The set

Default_Ignorable_Code_Point — 4,174 code points in 17 ranges:

U+00AD               SOFT HYPHEN
U+034F               COMBINING GRAPHEME JOINER
U+061C               ARABIC LETTER MARK
U+115F..U+1160       HANGUL CHOSEONG FILLER .. HANGUL JUNGSEONG FILLER
U+17B4..U+17B5       KHMER VOWEL INHERENT AQ .. KHMER VOWEL INHERENT AA
U+180B..U+180F       MONGOLIAN FREE VARIATION SELECTOR ONE .. FOUR
U+200B..U+200F       ZERO WIDTH SPACE .. RIGHT-TO-LEFT MARK
U+202A..U+202E       LEFT-TO-RIGHT EMBEDDING .. RIGHT-TO-LEFT OVERRIDE
U+2060..U+206F       WORD JOINER .. NOMINAL DIGIT SHAPES
U+3164               HANGUL FILLER
U+FE00..U+FE0F       VARIATION SELECTOR-1 .. VARIATION SELECTOR-16
U+FEFF               ZERO WIDTH NO-BREAK SPACE
U+FFA0               HALFWIDTH HANGUL FILLER
U+FFF0..U+FFF8       (unnamed)
U+1BCA0..U+1BCA3     SHORTHAND FORMAT LETTER OVERLAP .. UP STEP
U+1D173..U+1D17A     MUSICAL SYMBOL BEGIN BEAM .. END PHRASE
U+E0000..U+E0FFF     (tag characters and unnamed)

Cc — the C1 controls, 32 code points, none of them default-ignorable:

U+0080..U+009F

Cf — format characters that are not default-ignorable, 32 code points:

U+0600..U+0605       ARABIC NUMBER SIGN .. ARABIC NUMBER MARK ABOVE
U+06DD               ARABIC END OF AYAH
U+070F               SYRIAC ABBREVIATION MARK
U+0890..U+0891       ARABIC POUND MARK ABOVE .. ARABIC PIASTRE MARK ABOVE
U+08E2               ARABIC DISPUTED END OF AYAH
U+FFF9..U+FFFB       INTERLINEAR ANNOTATION ANCHOR .. TERMINATOR
U+110BD              KAITHI NUMBER SIGN
U+110CD              KAITHI NUMBER SIGN ABOVE
U+13430..U+1343F     EGYPTIAN HIEROGLYPH VERTICAL JOINER .. END WALLED ENCLOSURE

Zs — space separators, 16 code points:

U+00A0               NO-BREAK SPACE
U+1680               OGHAM SPACE MARK
U+2000..U+200A       EN QUAD .. HAIR SPACE
U+202F               NARROW NO-BREAK SPACE
U+205F               MEDIUM MATHEMATICAL SPACE
U+3000               IDEOGRAPHIC SPACE

Zl U+2028 LINE SEPARATOR and Zp U+2029 PARAGRAPH SEPARATOR.

Total: 4,256 code points.

I verified that these are genuinely reachable rather than merely theoretical. All of U+0085 (Cc), U+00A0 (Zs), U+2028 (Zl), U+2029 (Zp), U+3000 (Zs), U+0600 (Cf, not DI), U+200D (DI) and U+E0001 (DI) are accepted by PHP inside an identifier.

Of these, three actually occur in the 5,000-package corpus: U+00A0, U+3000 and U+200D.

Three things I proposed in the opening comment and now want to withdraw

Writing the list out made these points obvious, which is really the point of writing it out.

Cn (unassigned) — drop. 814,730 code points, and membership depends entirely on which Unicode version the tables come from. A file using a character assigned in a newer Unicode version than the one PHPCS was built against would be flagged even though the code is legitimate. For a warning-level sniff, that is the worst failure mode, and it is the same version-instability trap I ran into during the survey.

Cs (surrogates) — drop, unreachable. Well-formed UTF-8 cannot contain them. A CESU-8 encoded surrogate fails the preg_match('//u') check and lands under the malformed-name error code instead, which is the correct place for it.

Co (private use) — drop, different problem. 137,468 code points, and they are reachable, but they are not invisible — they render as whatever the font supplies, which is unpredictable rather than absent. If that is worth flagging, it deserves its own error code and its own justification, not a free ride on this one.

One judgement call I would like a second opinion on

The 32 non-default-ignorable Cf characters are format controls that do affect the rendering of adjacent text — U+0600 ARABIC NUMBER SIGN and the Egyptian hieroglyph joiners are not invisible in the same way that U+200D is.

I have included them on the grounds that none of them has any business inside an identifier and that all of them are, at best, unobvious there. But it is a defensible line to draw differently.

Implementation, which turned out simpler than I expected

PCRE in PHP supports \p{Default_Ignorable_Code_Point} directly, so no bundled range table is needed at all. The whole check is:

private const INVISIBLE = '/[\p{Cf}\p{Cc}\p{Zs}\p{Zl}\p{Zp}\p{Default_Ignorable_Code_Point}]/u';

I checked that the property is discriminating rather than matching everything: it matches U+00AD, U+200D, U+FE0F, U+E0001 and U+FEFF, and does not match a, U+00E4, U+3042, U+00A0 or U+0600 — the last two being why Zs and Cf are included in the class separately.

Against the corpus cases, the combined expression flags chrtext<U+00A0>, zwj<U+200D>test and the U+3000 U+3000 1 identifier, and does not flag chrtext or größe.

This also removes a maintenance concern I raised earlier: the Default_Ignorable_Code_Point set is not covered by a Unicode stability guarantee, but if the property comes from PCRE rather than from a table we maintain, it tracks the PHP build like every other \p{} usage in the codebase instead of requiring its own update cycle.

Configurable or fixed

Now that the list is concrete, I do have a view. I would make the set fixed rather than configurable, because the argument for each member is the same, and a project that wants a different set almost certainly wants a different sniff.

What I would consider instead is a narrow escape hatch: a property listing individual code points that a project explicitly accepts. That would allow something like markrogoyski/math-php, which deliberately uses U+00A0 inside variable names, to allow that one code point while keeping the rest of the check intact. That is a much smaller surface than making the whole set configurable, and it fails safe — you have to explicitly name what you are allowing through.

[jrfnl]

@Otzie2023 Thank you for your extensive answer.

One maintenance point that is probably worth documenting: UAX #31 §2.3 says that Default_Ignorable_Code_Point does not have the same stability guarantee as XID_Start / XID_Continue. So the range list would need to be refreshed when the UCD is updated. It has only grown occasionally, but it is not a frozen list.

Not for now, but we may be able to come up with an automated check in CI which could run via a cronjob and check the character count (?) and fail the job when it is not the same as the current/expected count. That way the project gets automatic notification if the range list has changed (instead of having to keep an eye on it ourselves, which is easily forgotten).

There may also be other notification mechanisms we could use, the above is just one idea to explore.

Declaration versus usage

Agreed. And I think the variable exception you pointed out is more than just a convenience — it is really the important distinction here.

For classes, interfaces, traits, enums, functions, methods and constants, a usage containing an invisible character that does not match a declaration will fail loudly at runtime: class not found, undefined function, etc. The developer gets an error. So checking the declaration is enough.

Not just that, but unless and until the declaration is changed, changing usages in a project is useless and dangerous.
Also keep in mind that projects may be referencing (using) identifier names defined in dependencies and the usage references cannot be changed until the dependency has changed the declarations in their own code and released a new version.

Variables have no declaration, and a mistyped usage fails silently — it creates a new variable.

Not necessarily - a mistyped usage will generally cause a "Undefined variable" notice (PHP 7) or warning (PHP 8), it will not automatically create a new variable.

The risky part is that variables can be initialized in so many different ways: plain assignments, multi-assignments, passing by reference, foreach(), etc and that a number these, in particular the passing by reference, is something PHPCS is not typically suitable for to detect.
Also keep in mind that global variables are available to all files used in a process, and PHPCS cannot trace whether a variable has been initialized elsewhere, or not, across files.

So variables really do need to be checked at usage.

Agreed, but with slightly different arguments 😉

To avoid producing N reports for the same rename, I’d lean towards reporting once per distinct name per file, at the first occurrence. I’m not particularly attached to that detail, though, so it’s easy to change.

We tend to try to avoid the "one error hiding behind another" pattern, and your suggestion would lead to that. What I mean by that is: the first error in the file is flagged, someone fixes it and then in the next PHPCS run, the next error gets flagged.

In my opinion, we serve users better by giving them all the errors in one go, allowing them to fix everything in one go. In this case, this would mean flagging every error in each file.

Encoding

This is the interesting part, and I don’t think “UTF-8 only” is safe.
...
A UTF-8-only sniff would report the first two cases as a malformed encoding problem rather than an invisible character problem. That feels like the wrong message on exactly the kind of issue this sniff is intended to catch.

I never meant that as "safe", I meant it as a potentially valid implementation choice in the full knowledge that that would still leave an "uncovered" area of potential problems the sniff would not detect.

So, the way I intended my previous comment was that the sniff would bow out early if a project indicated to PHPCS that it uses a non-utf-8 file encoding, In other words, the sniff would not report the wrong message, it would stay completely silent instead.

And while not the preferred option, I do think it could be a valid implementation choice if supporting non-utf8 files for this sniff would get too complex. After all, in my experience, projects using non-utf-8 file encodings AND using PHPCS - are quite rare nowadays and mostly proprietary projects (so you would not find them in the Packagist top 5000).

So my proposal would be:

I would make step 4, step 0, but that is an implementation detail.
It would work by bowing out early if encoding is not utf-8 - or a subset thereof - and iconv is not available - we may even be able to unregister the sniff completely in that case, so that check would only be done once per run.

The caveat is that encoding is a per-run setting, not a per-file setting.

Sort of, encoding can be, and typically is, set from within a project ruleset, which would make it the default for all runs for that project.

So mixed-encoding projects can still get this wrong for some files. I don’t think the sniff can do better than the encoding it is given, but it is worth documenting.

I agree, that is something which can't be helped and is the responsibility of the project using PHPCS, not something PHPCS can solve.

... and while writing this down, it also makes me wonder how PHP handles this, also in combination with a potential encoding execution directive - and potential mismatches in that.

Let me spell out what I mean by that:

• Say a project has encoded all their files in BIG5...
• And tells PHPCS as much by passing the encoding configuration setting as BIG5...
• But doesn't have a declare(encoding='Big5'); at the top of each file....
• How does PHP handle that when interpreting identifier names ?

Or the reverse: the project has declare(encoding='...') statements at the top of each file, but the file encoding doesn't match the declare statements ?
Again: my question there is - how does PHP interpret identifier names in that case ?

Is that maybe another sniff we should add ? A sniff to check that if a PHPCS configuration sets an encoding which is not a subset of/compatible with UTF-8, that each file in the project has a declare(encoding='...'); statement at the top of the file ?

Repo positioning

After looking through PHPCSUtils, my honest reading is that the utilities are doing real work here, rather than just being a convenience. So I’d put this in PHPCSExtra / Universal.

Exactly, glad you like what you see in PHPCSUtils. There are still more utilities in PHPCSUtils you'll want to use (ObjectDeclarations::getName() comes to mind, but there are more), but we'll get to that when this moves to implementation.

I'm perfectly fine with this going into PHPCSExtra. And if needs be, also happy to add additional utility methods to PHPCSUtils

I suggest I transfer this issue to the PHPCSExtra repo, but only after we've hashed out this discussion to allow people to follow along for now. Is that okay with you ?

[jrfnl]

I haven't fully formed an opinion on the exact character proposal as per #1491 (comment) yet. Still gathering my thoughts.

I wonder if @heiglandreas might be interested to also have a look and share an opinion ? (time permitting, of course)

[heiglandreas]

The proposal has - from what I have read so far, If I missed something, please do correct me! - one big conceptual.flaw: It assumes that the files containing the variable names are in UTF-8. While I propose that I also know that that isn't always the case. And when using that approach on a non-UTF-8 file it will render false positives.

In addition IMO also the newasy-way for everything below 0x80 is probably flawed as there are some zero-width bytes that we should probably also not allow (who needs a bell character or ACK in a variable name)

I am not at a computer right now and will therefore uave to check deeper on tuesday...

[heiglandreas]

The proposal has - from what I have read so far, If I missed something, please do correct me! - one big conceptual.flaw: It assumes that the files containing the variable names are in UTF-8. While I propose that I also know that that isn't always the case. And when using that approach on a non-UTF-8 file it will render false positives.

This has already.been addressed. The question is how the sniff detects non-UTF8 files within a project...

In addition IMO also the newasy-way for everything below 0x80 is probably flawed as there are some zero-width bytes that we should probably also not allow (who needs a bell character or ACK in a variable name)

I am not at a computer right now and will therefore uave to check deeper on tuesday...

I definitely like using the Built-in character-reference of the PCRE extension as that means that we do not have to provide our own range-definition.

The question though is how up-to-date those definitions are when a new language with possibly new whitespace-characters gets added to Unicode. This should most of the time not be an issue but needs perhaps just a clarification.

[Otzie2023]

Four of the open points turned out to be measurable rather than arguable, so I went ahead and measured them. Everything below was tested against PHP 8.3.6 with PCRE 10.42. I have only tested one build, though, which matters for the last section.

Variables: you are right, with one refinement

a mistyped usage will generally cause a "Undefined variable" notice (PHP 7)
or warning (PHP 8), it will not automatically create a new variable

Correct, and I overstated this point. Reading an undefined variable produces a warning. But passing one by reference creates it silently, without any diagnostic at all:

[1] reading      $undefined            -> Warning: Undefined variable
[2] internal fn  openssl_public_encrypt($x, $undefined, $y)  -> created, no warning
[3] userland fn  function f(&$v){}  f($undefined)            -> created, no warning
[4] preg_match("/x/", "x", $undefined)                       -> created, no warning

That is exactly the path you singled out as the risky one, and it is exactly what happens in the Alipay case: openssl_public_encrypt() has a second parameter passed by reference. So the silent failure is real, but it is narrower than I originally described. It is specifically the by-reference case, not undefined variables in general.

Reporting every occurrence

I am convinced on both of your arguments, and I think the second one is the stronger:

projects may be referencing (using) identifier names defined in dependencies
and the usage references cannot be changed until the dependency has changed
the declarations

That settles it independently of the noise question. Flagging a usage where the declaration lives in vendor/ produces a report that the user cannot actually act on. And I withdraw the "once per name per file" idea as well. The "one error hiding behind another" pattern is a fair objection, and reporting everything in one go is clearly more useful to the user.

Encoding

Your restructuring is better than mine, and I am happy with it: make the bail-out step 0 and unregister the sniff for the run rather than checking every file individually. The sniff remaining silent on non-UTF-8 files is also defensible. I had interpreted "UTF-8 only" as "assume UTF-8 and report anyway", which is what I was arguing against, but that is not what you meant.

I would still like to keep the iconv conversion on the table as an option rather than making it a requirement, precisely because of how cheap the ASCII fast path is. In the 5,000-package corpus, only 1,447 identifier tokens across 520,802 files ever reach the point where a conversion would be necessary. But that is an optimisation of coverage, not of correctness, and in either case the sniff never emits a wrong message.

Your declare(encoding=...) question

This one had a definite answer, and I did not expect it to be this one.

I built an ISO-8859-1 file containing $b<0xE4>r and then read back the raw bytes of the resulting variable name:

Configuration	Identifier bytes
default PHP	62 E4 72 — raw, unchanged
declare(encoding="ISO-8859-1"), default PHP	62 E4 72 — and a warning
zend.multibyte=1 + zend.script_encoding=ISO-8859-1	62 C3 A4 72 — transcoded to UTF-8
zend.multibyte=1 + declare(encoding="ISO-8859-1")	62 C3 A4 72 — transcoded to UTF-8

The warning in row two is:

Warning: declare(encoding=...) ignored because Zend multibyte feature is
turned off by default

So, for your two scenarios:

Files in BIG5, with PHPCS set to encoding=BIG5, but no declare(). PHP reads the bytes as-is, so the identifier in the symbol table contains the raw BIG5 bytes. This is mostly academic for BIG5 specifically, because its trail bytes span 0x40–0x7E and such an identifier will not parse at all — the same applies to Shift-JIS.

declare(encoding=...) is present, but does not match the file. With zend.multibyte off, which is the default, the directive does nothing and PHP says so. With it enabled, PHP transcodes the source from the declared encoding, so a mismatch results in mojibake identifiers rather than an error.

For the sniff you floated — checking that non-UTF-8 projects have declare(encoding=...) in every file — I would gently argue against it. That directive is a no-op unless zend.multibyte=1, which is off by default, and PHPCS cannot see that ini setting. A sniff that encourages people to add a directive which does nothing on a stock PHP build seems more likely to confuse than to help.

The useful consequence for this sniff is much simpler: what matters is the bytes actually present in the file, which is what the PHPCS encoding setting describes, and the sniff reads the file. So the behaviour is correct regardless of how the project has zend.multibyte configured.

@heiglandreas on bytes below 0x80

there are some zero-width bytes that we should probably also not allow (who
needs a bell character or ACK in a variable name)

I checked this, and the good news is that they cannot get in. The LABEL rule is [a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*, so any byte below 0x80 that is not [a-zA-Z0-9_] terminates the identifier rather than becoming part of it. I tested BEL, ACK, NUL, ESC, VT, FF and DEL. Every one of them is a parse error; none is accepted inside a name.

That is what makes the ASCII fast path safe rather than merely fast: there is nothing below 0x80 for it to skip over.

The C1 controls are a different matter. U+0080..U+009F are >= 0x80 and are accepted inside identifiers, which is why \p{Cc} is included in the character class.

@heiglandreas on how current the PCRE tables are

The question though is how up-to-date those definitions are when a new
language with possibly new whitespace-characters gets added to Unicode.

This is also measurable, and the lag is real. PHP 8.3.6 here ships with PCRE 10.42 (2022-12-11):

Code point	Assigned in	\p{L}	\p{Cn}
U+0870 ARABIC LETTER ALEF	Unicode 14.0	yes	no
U+10EFD ARABIC SMALL LOW WORD	Unicode 15.1	no	yes
U+105C0 TODHRI LETTER	Unicode 16.0	no	yes
U+11BC0 SUNUWAR LETTER	Unicode 16.0	no	yes

So this PCRE knows about Unicode 14.0, but not 15.1 or 16.0. And since PHPCS supports PHP 7.2 upwards, the spread across the actual user base is wider still.

Two things follow from that.

This is the concrete reason Cn has to stay out of the set. A Sunuwar or Todhri letter can be perfectly legal in a PHP identifier, but this PCRE build sees it as \p{Cn}. Including Cn would therefore flag legitimate code as containing an invisible character, and it would do so differently depending on which PHP version the user happens to run. I argued that abstractly in my previous comment; here we have a concrete example.

The remaining sets fail in the safe direction. Cf, Cc, Zs, Zl, Zp and Default_Ignorable_Code_Point are conservative and change relatively slowly. A stale PCRE means that a newly assigned default-ignorable character would be missed, rather than falsely flagged. For a warning-level sniff, a missed detection is by far the better failure mode.

On the CI idea

we may be able to come up with an automated check in CI which could run via a
cronjob and check the character count

Since the property comes from PCRE rather than from a table we maintain ourselves, there is no list for us to let drift. But the numbers above suggest a slightly different variant that would be worth having: count the code points matched by the character class for each PHP version in the test matrix and record the result.

That would make both kinds of change visible: an update to PCRE that changes the set, and the spread between the oldest and newest supported PHP versions. The latter is the one most likely to turn into a "works on my machine" report.

Transfer to PHPCSExtra

Yes, and thank you. Moving it once the discussion has settled sounds like the right approach, so that people following along here do not lose the thread.