Fix undef boolean attributes

[oalders]

Summary

Adds a new opt-in strict_boolean_attributes option (off by default) that controls how attributes without values are reported in start events. When enabled, only attributes that the HTML Living Standard defines as boolean (e.g. checked, disabled, selected, defer, inert) are reported with their name as the value; every other value-less attribute is reported as undef.

The default remains the historic name-as-value behavior, so existing consumers see no change.

Fixes #17.

Behavior

# Default (unchanged)
my $p = HTML::Parser->new;
# <input checked foo>  →  { checked => "checked", foo => "foo" }

# Strict
$p->strict_boolean_attributes(1);
# <input checked foo>  →  { checked => "checked", foo => undef }

The option affects both the attr and tokens argspecs. Matching against the spec list is ASCII case-insensitive (<input CHECKED> still matches). boolean_attribute_value still applies to recognized boolean attributes; non-boolean value-less attributes always become undef regardless of that setting.

Boolean attribute list

Sourced from the HTML Living Standard plus a few HTML4 legacy entries still found in the wild:

allowfullscreen, async, autofocus, autoplay, checked, compact, controls, default, defer, disabled, formnovalidate, hidden, inert, ismap, itemscope, loop, multiple, muted, nomodule, noresize, noshade, novalidate, nowrap, open, playsinline, readonly, required, reversed, selected, shadowrootclonable, shadowrootdelegatesfocus, shadowrootserializable, truespeed

allowpaymentrequest is intentionally excluded (removed from the spec).

Implementation notes

• New is_boolean_attribute() helper in hparser.c uses strnEQx() from util.c for the case-insensitive comparison; the spec list is a static const table with a BOOL_ATTR(s) macro that derives lengths from sizeof(s) - 1 so the array stays consistent on edits.
• Both the ARG_ATTR and ARG_TOKENS paths share the helper so the two argspecs agree in strict mode.
• The option is wired through the existing ALIAS-driven boolean accessor in Parser.xs and copied in dup_pstate for ithreads.

Test plan

• make test — all 51 test files, 526 subtests pass on perl 5.42.
• New t/strict-boolean-attributes.t covers:
  • Accessor semantics and default.
  • Default-mode preservation (no regression).
  • Strict-mode happy path on attr and tokens.
  • Regression for an uninitialized-SV bug in the prior implementation: prefix collisions like <x dxsabled> (length matches disabled, content differs after first char) now correctly resolve to undef.
  • ASCII case-insensitive matching.
  • boolean_attribute_value interaction in both modes.
  • Smoke test of every entry in the list.

[oalders]

Code Review: PR #36 — Fix undef boolean attributes

Summary

The PR aims to align HTML::Parser with the HTML spec's notion of boolean attributes by emitting undef for value-less attributes that are not in a known boolean list. The direction is reasonable, but the implementation has a critical correctness bug, a serious semantic break with no opt-in/opt-out, an incomplete and partly inaccurate attribute list, and inconsistent formatting that violates the surrounding C style. This is not ready to merge.

---

Critical (must fix before merge)

C1. found = 1 is set inside the per-character loop, leading to uninitialized attrval

hparser.c lines ~504-529. Re-indented for readability:

for (i = 0; boolean_attributes[i].len; i++) {
    if (attrlen == boolean_attributes[i].len) {
        ...
        while (len) {
            if (toLOWER(*attrname_s) != *t) break;
            attrname_s++; t++;
            if (!--len) {
                /* full match: set attrval */
                ...
            }
            found = 1;    /* <-- BUG: runs after EVERY matching character */
        }
    }
}
if (!found) attrval = newSV(0);

found = 1 lives inside the while (len) body, outside the full-match if (!--len) block. Consequence: any value-less attribute that shares a length with a boolean attribute and matches the first character but mismatches later will:

• never set attrval (the full-match body did not execute),
• still set found = 1, so the fallback attrval = newSV(0) is skipped,
• pass an uninitialized SV* to hv_store_ent / av_push.

Concrete trigger inputs:

• <x dxsabled> (8 chars, matches disabled length, first char matches, second mismatches)
• <x cxntrols> (matches controls length)
• <x autopxay> (matches autoplay length)

This is undefined behavior: crashes, leaks, or memory corruption depending on what's on the stack. ASAN/Valgrind will flag it. The new test happens to avoid this case (foo is length 3, no length-match), which is why CI is green.

Required fix: move found = 1; inside the if (!--len) { ... } block and break to exit the outer for. Add a regression test for the prefix-collision case.

C2. Silent, undocumented breaking change with no migration path

This breaks the documented contract. lib/HTML/Parser.pm POD currently states:

Boolean attributes' values are either the value set by $p->boolean_attribute_value, or the attribute name if no value has been set by $p->boolean_attribute_value.

After this PR that promise no longer holds for any attribute name not in the 25-entry list. Idioms that will silently break:

$attr->{align} || ''                      # was "align", becomes undef (probably fine)
$some_dict{ $attr->{foo} }                # warning + lookup with undef
"value=$attr->{custom}"                   # uninitialized-value warning
exists $attr->{x} && length $attr->{x}    # was true, now false

Custom data-* attributes (e.g. <div data-active>), Vue/Alpine/HTMX-style attributes (<div hx-boost>), Angular template attributes, and any future HTML attribute all flip from name => "name" to name => undef.

Required actions before merge:

1. Update the POD (lib/HTML/Parser.pm) to describe new behavior.
2. Decide on backward compatibility — either gate the new behavior behind an option (e.g., $p->strict_boolean_attributes(1), default off for one or two minor releases) or treat as a major version bump with a Changes entry.
3. Add a Changes entry under {{$NEXT}} referencing GH#17 and GH#36.
4. Fill in the empty PR description.

C3. Boolean attribute list is incomplete and includes a non-standard entry

The list is sourced from a 2017 blog post. Against the current HTML Living Standard:

• Missing standard booleans: defer (very common on <script>), inert (HTML living standard since 2022), shadowrootclonable, shadowrootdelegatesfocus, shadowrootserializable. HTML4 booleans nowrap, compact, noresize, noshade are missing if legacy compat matters.
• Non-standard: allowpaymentrequest was removed from the spec.
• Likely obsolete: truespeed is HTML4 <marquee>. Acceptable to keep if HTML4 compat matters; comment why.

---

Important (should fix)

I1. Indentation breaks the surrounding C style

Lines 504-528 mix tab counts and use space indentation alongside the function's tabs. The visual structure doesn't match the logical brace structure, which is what hid the C1 bug during review. Reformat to match literal_mode_elem (lines ~23-40) — that's the precedent this code follows.

I2. Reuse the existing strnEQx helper

util.c:23-39 exports strnEQx(s1, s2, n, ignore_case). Using it collapses the entire byte-by-byte compare:

for (i = 0; boolean_attributes[i].len; i++) {
    if (attrlen == boolean_attributes[i].len &&
        strnEQx(SvPVbyte_nolen(attrname),
                boolean_attributes[i].str,
                attrlen, 1)) {
        if (p_state->bool_attr_val)
            attrval = newSVsv(p_state->bool_attr_val);
        else
            attrval = newSVsv(attrname);
        found = 1;
        break;
    }
}

No shadowing, no manual loop, matches abstractions already in this codebase.

I3. int i shadows the outer loop variable

Line ~502: the inner int i; shadows the outer for (i = 1; i < num_tokens; i += 2). Rename to j or b. -Wshadow flags this.

I4. Duplicated len values are a maintenance hazard

{15, "allowfullscreen"} — a future edit can change one without the other. Use a macro:

#define BOOL(s) { sizeof(s) - 1, s }
static const struct boolean_attribute boolean_attributes[] = {
    BOOL("allowfullscreen"),
    ...
    { 0, NULL }
};

I5. Test coverage is too narrow

The single new test covers only the happy path. Add:

• Uppercase: <input CHECKED>
• Mixed case: <input Checked>
• Prefix collisions (regression for C1): <input dxsabled>, <input cxntrols>, <input autopxay> → all should yield undef
• boolean_attribute_value set explicitly: $p->boolean_attribute_value(1); parse("<input checked>") → checked => 1
• All 25 entries (smoke test)
• Same-length non-boolean (<x asyna> vs async)

I6. The ARG_TOKENS boolean branch is not touched

hparser.c line ~409: the ARG_TOKENS path still emits the attribute name unconditionally for boolean attrs. This creates a divergence:

• argspec => 'tokens' reports ["a", "checked", "checked"] (unchanged)
• argspec => 'attr' reports {checked => "checked", foo => undef}

Decide whether tokens should match, and either fix or document the divergence.

I7. Test harness diagnostic substitution needs a comment

t/cases.t and t/parser.t now substitute '<undef>' for undef values. Add a one-line comment so future maintainers understand the convention (especially since t/msie-compat.t has a coincidentally-similar literal in unrelated output).

---

Minor (nice to fix)

M1. const static → static const

Line 89. The codebase's other static-const tables use static const. Swap for consistency.

M2. Trailing whitespace on blank line(s)

Line ~528 inside the else block has tab-only indentation. Clean up.

M3. int attrlen declared with space indent

Line ~475 uses spaces, the rest of the function uses tabs.

M4. Messy commit history

ongoing work, not ideal fix - …, Fix test case. Squash into one clean commit referencing GH#17 before merge.

M5. Unrelated .gitignore change

Adding local/ is fine but unrelated; could be its own commit/PR.

M6. Move the rationale out of commit messages

ec71290's message admits "not ideal". If this lands, the rationale belongs in the PR description and Changes, not the commit message.

M7. Cite the spec, not a blog post

Replace the meiert.com comment with a link to the HTML spec's boolean-attribute list.

---

Production-readiness verdict

Not ready to merge. Blocking issues:

1. Memory safety bug (C1). Common inputs like <x dxsabled> produce an uninitialized SV in the resulting hash. Undefined behavior.
2. Undocumented breaking change (C2). Existing consumers (custom attributes, framework attributes, anything not in the 25-entry list) silently change from name => "name" to name => undef.

Plus an incomplete/inaccurate list, inconsistent C style, and test coverage that doesn't exercise the C1 bug.

Recommended action plan

1. Fix C1: move found = 1 inside the full-match block + break. Add prefix-collision tests.
2. Refactor to use strnEQx from util.c; rename the shadowed i.
3. Update the list: add defer, inert, drop allowpaymentrequest; cite the spec.
4. Gate the new behavior behind a strict_boolean_attributes option (default off) or commit to a major version bump.
5. Update POD in lib/HTML/Parser.pm and add a Changes entry under {{$NEXT}}.
6. Decide and document ARG_TOKENS behavior.
7. Reformat the new code to match the project's tab-based style.
8. Squash commits, write a real PR description, split unrelated changes.
9. Re-request review.

What was done well

• Targets a real long-standing issue (Attributes that have no value get their name as their value #17), rooted in the HTML spec.
• Modeled on the existing literal_mode_elem precedent.
• Test harness thoughtfully extended to display <undef>.
• Honest commit messages signaled uncertainty to reviewers — good signal.

[oalders]

Code Review (Round 2) — PR #36

What was done well

• C1 is conclusively dead. The prior bug pattern (length-match + first-char match + later mismatch leaving attrval uninitialized) is structurally impossible with the new is_boolean_attribute() helper: the function always returns 0 or 1, and both call sites always assign attrval/push to the AV in both arms. The dedicated regression test (<x dxsabled cxntrols autopxay aXync foox> in t/strict-boolean-attributes.t:34-43) exercises the exact pattern and passes.
• C2 is fully resolved. Default behavior is bit-for-bit identical to master; verified by reverting test expectations in t/cases.t, t/msie-compat.t, t/parser.t and observing full suite passes. The opt-in path is documented at three POD locations and a Changes entry that makes the opt-in nature unambiguous.
• Code quality is much improved. is_boolean_attribute() reuses strnEQx(), replaces broken indentation, kills the i shadowing, and is shared from both code paths. The BOOL_ATTR(s) macro keeps len and str in sync. static const ordering is correct. Sorted alphabetically. Comment explains why HTML4 legacies are kept.
• Tests are comprehensive (64 subtests after the latest commit). Coverage spans accessor semantics, default-mode regression guard, strict-mode happy path, C1 regression pattern, case-insensitive matching, boolean_attribute_value interaction, every spec-listed entry individually, and the ARG_TOKENS path.
• Threading bookkeeping is correct. dup_pstate carries strict_boolean_attributes forward.
• Zero-init is verified. Newz in _alloc_pstate gives strict_boolean_attributes = false by default; ALIAS case 14 toggles it via the existing boolean-accessor mechanism.

Critical

None.

Important

I1 (FIXED in follow-up commit e4bc5e3)

Boolean list was missing disablepictureinpicture and disableremoteplayback, both currently listed as boolean attributes in the HTML Living Standard. Added.

Minor

M1 — prev_token may still be &PL_sv_undef on the first iteration in ARG_TOKENS

prev_token is initialized to &PL_sv_undef. If the first token were ever a "boolean" (zero beg), SvPVbyte_nolen(&PL_sv_undef) and SvCUR(&PL_sv_undef) would be called. Empirically this isn't reachable for HTML start tags (token 0 is always the tagname), and the resulting ("", 0) would correctly fall through to newSV(0). Not a bug today, just a fragility point.

M2 — Intentional leftovers

• .gitignore local/ change is still in the diff (review M5).
• Five-commit history is not squashed (review M4). Recommend squash-on-merge via GitHub UI.

M3 (FIXED) — confusing test message

Cleaned up in e4bc5e3.

Verdict

Ready to merge. All critical and important issues are resolved. C-level correctness, threading, default-preservation, docs, and tests are solid. The strict-mode gating is exactly the right shape for a behavior-change PR in a mature module: opt-in, documented, no surprises for existing users.

Final test status: make test passes 528 subtests across 51 files on perl 5.42.