Skip to content

use ValueLessParameter concept for requiredArgumentCount#6310

Merged
anonrig merged 1 commit intomainfrom
yagiz/remove-valueless
Mar 16, 2026
Merged

use ValueLessParameter concept for requiredArgumentCount#6310
anonrig merged 1 commit intomainfrom
yagiz/remove-valueless

Conversation

@anonrig
Copy link
Copy Markdown
Contributor

@anonrig anonrig commented Mar 12, 2026

The previous isValuelessArg trait in web-idl.h only handled TypeHandler and Arguments, missing InjectConfiguration types like CompatibilityFlags::Reader. Instead of maintaining a manual list, thread TypeWrapper through requiredArgumentCount and use the ValueLessParameter
concept to automatically detect all injected parameter types.

@anonrig anonrig requested a review from kentonv March 12, 2026 18:44
@anonrig anonrig requested review from a team as code owners March 12, 2026 18:44
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Mar 12, 2026

Only the two places we already identified. This is clean.

Let me now think through the overall review:

  1. Correctness: The refactor correctly replaces a manually maintained list (isValuelessArg for TypeHandler<T> and Arguments<T>) with a concept-based approach (ValueLessParameter) that automatically detects all injected types. The Arguments<T> special case is correctly preserved via isArguments<>().

  2. Template instantiation ordering: The comment in the PR correctly explains that RequiredArgCount_ specializations in type-wrapper.h will be visible when resource.h templates are instantiated, because type-wrapper.h includes resource.h (so resource.h is parsed first with only the forward declaration), and by the time the resource.h templates are actually instantiated (deferred until after the whole TU is parsed), the specializations will be visible. This is standard C++ two-phase lookup behavior.

  3. Tests: The tests are well-written, covering TypeHandler<T>, Arguments<T>, the new InjectConfiguration case (via FakeConfig), and the ValueLessParameter concept directly. The MockTypeWrapper approach is clean and self-contained.

  4. Backward compatibility: No behavioral changes to external APIs — this is purely internal template metaprogramming. The .length property on functions will now correctly account for InjectConfiguration parameters, which was previously a bug (they would have been incorrectly counted as required JS arguments).

  5. No compat flag needed: This is a bugfix to an internal mechanism. The incorrect .length was an implementation defect, not an intentional behavior.

  6. No security concerns: No sandbox or capability changes.

  7. No wire format changes: No .capnp schema changes.

The PR is clean. The design is sound, the test coverage is thorough, and the migration is complete.

LGTM

github run

@anonrig anonrig enabled auto-merge March 12, 2026 18:52
@anonrig anonrig force-pushed the yagiz/remove-valueless branch from bff0eae to 0b7b2bc Compare March 12, 2026 20:27
@anonrig anonrig force-pushed the yagiz/remove-valueless branch from 0b7b2bc to 4f57171 Compare March 13, 2026 00:07
@anonrig anonrig requested review from jasnell and mikea March 13, 2026 01:23
@anonrig anonrig merged commit 168428a into main Mar 16, 2026
28 of 29 checks passed
@anonrig anonrig deleted the yagiz/remove-valueless branch March 16, 2026 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants