feat(generator): support map-like and pair-iterable types#533
feat(generator): support map-like and pair-iterable types#533
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements automatic generation of JSIterable implementations and asMap views for Web IDL types, significantly improving the Dart developer experience for interfaces like Headers, FormData, and URLSearchParams. The generator was updated to handle multiple inheritance for JS types, and the SDK requirement was bumped to 3.12.0. Review feedback highlights several correctness issues in the generated map view classes, specifically regarding improper type casting of keys, missing conversions to JS types for interop calls, and potential null-safety crashes in the operator [] and remove implementations.
- Automatically generate `JSIterable` implementations for types defined as iterable in Web IDL. - Generate `asMap` views for map-like types in `js_interop_gen` (e.g., `URLSearchParams`, `FormData`, `Headers`). - Generate `toDart` getters for pair-iterable types like `XRHand`. - Bump minimum SDK constraint to `^3.12.0-0` across packages to support new features like private named parameters. - Update CI workflows to test on `beta` and `dev` instead of `3.10`. - Fix test failures related to SDK version string checks. - Clean up refactorings in `translator.dart` to preserve `followedBy` style for review.
kevmoo
force-pushed
the
iterable_map_rebased
branch
from
0a14b97 to
f80ebe8
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the SDK requirement to 3.12.0 and introduces support for generating Dart-idiomatic Iterable and Map views for Web IDL iterable and maplike interfaces. The translator has been updated to generate toDart and asMap getters, along with private backing classes that implement MapBase or UnmodifiableMapBase. Feedback identifies a potential type mismatch in the synthesized clear method where interop keys are incorrectly converted to Dart types before being passed to delete, an unsafe assumption that all map-like interfaces with array values provide a getAll method, and redundant logic in the hasOperation helper that should be refactored.
| ? code.Code(''' | ||
| final keys = _jsObject.keys().toDartIterable.toList(); | ||
| for (final k in keys) { | ||
| _jsObject.delete(${_toDartCall('k', keyInteropType.symbol)}); |
There was a problem hiding this comment.
In the synthesized clear implementation, the code calls delete after converting the key to a Dart type using _toDartCall. However, if the delete method was generated by this PR (see line 887), it expects an interop type (e.g., JSString), not a Dart primitive (e.g., String). This will cause a type mismatch. Since k already has the interop type from toDartIterable on a JSIterator, you should pass k directly if the delete method expects an interop type.
| $keyTypeCheck | ||
| final value = _jsObject.get($keyConversionLocal); | ||
| if (value == null) return null; | ||
| return _jsObject.getAll($keyConversionLocal); |
There was a problem hiding this comment.
The generator assumes that any map-like type with a JSArray value type will have a getAll method. While this is true for specific types like FormData or URLSearchParams, it is not a standard part of the Web IDL maplike definition. If a new interface is defined as maplike<K, sequence<V>> without an explicit getAll operation, this generated code will fail at runtime. Consider checking if the operation exists before using it.
| bool hasOperation(String name) { | ||
| var c = interfacelike; | ||
| while (true) { | ||
| if (c.operations.containsKey(name)) return true; | ||
| if (c.inheritance != null) { | ||
| final superInterface = _interfacelikes[c.inheritance!]; | ||
| if (superInterface == null) break; | ||
| c = superInterface; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The hasOperation helper is defined here and again inside _extensionType (line 1338). This redundancy could lead to maintenance issues if the logic for looking up operations needs to change. Consider refactoring this into a shared helper or passing it as a closure.
JSIterableimplementations for types defined as iterable in Web IDL.asMapviews for map-like types injs_interop_gen(e.g.,URLSearchParams,FormData,Headers).toDartgetters for pair-iterable types likeXRHand.^3.12.0-0across packages to support new features like private named parameters.betaanddevinstead of3.10.translator.dartto preservefollowedBystyle for review.