Interop gen additional literal types#503
Conversation
Summary of ChangesHello @sagar-h007, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request enhances interop_gen to support more complex TypeScript literal types by leveraging the TypeScript TypeChecker, which is a solid improvement. The changes are well-structured and include relevant integration tests. I've identified one critical issue in the new TypeScript bindings that would lead to a compilation error due to an incorrect type definition. Additionally, I have a couple of medium-severity suggestions for the transformer logic to improve code clarity and efficiency by reducing redundancy. Once the critical issue is resolved, this will be a great addition.
|
I don't think there are much examples, or additions of code for other literal types, as needed for #465. You mentioned in one of the files that binary expressions are not allowed in TypeScript Declaration files: the reason for the issue is because these types were present in NPM packages I was testing with. |
|
@nikeokoronkwo I agree that these cases aren’t common in well-formed .d.ts files. The main goal here was to better handle real-world declarations coming from generated or loosely authored NPM typings, while still relying on the TypeScript TypeChecker and falling back safely where needed. The change is intentionally conservative and shouldn’t affect valid declaration files. |
| @@ -0,0 +1,8 @@ | |||
| export declare const negativeOne: -1; | |||
| // Binary expressions in type position are not valid TS in d.ts files usually. | |||
There was a problem hiding this comment.
Do we have an example we can test where it is valid?
There was a problem hiding this comment.
I looked into this but couldn’t find a realistic case where binary expressions are valid in .d.ts files. In practice, numeric literals like -1 seem to be the only form that’s commonly supported.
To avoid adding a test that isn’t representative or might be TS-version dependent, I limited coverage to those cases and added a note explaining the limitation.
| } | ||
|
|
||
| // Fallback to underlying type if not a literal | ||
| final underlyingTypeNode = typeChecker.typeToTypeNode( |
There was a problem hiding this comment.
Is there a test exercising this case?
There was a problem hiding this comment.
Not yet ,. I’ll add a test to explicitly exercise this fallback path.
There was a problem hiding this comment.
I might have missed it, but which test case covers this?
Use a shared helper to reduce duplication, remove unused TSTypeFlags, and add integration tests for false, null, and negative numeric literals.
srujzs
left a comment
There was a problem hiding this comment.
Mostly LGTM % my comments.
| // export declare const shiftLeft: 1 << 16; | ||
| // export declare const complex: 1 + 2 * 3; | ||
| export declare const existingLiteral: 1; | ||
| // export declare const existingLiteral: 1; |
There was a problem hiding this comment.
Did you mean to comment this test out?
There was a problem hiding this comment.
Yes, intentionally commented out to avoid duplicating coverage. Happy to re-enable if you think it improves clarity.
There was a problem hiding this comment.
Yeah, let's re-enable to add some coverage for non-negative numbers. Probably doesn't make a difference in the code but the original bug highlighted prefix unary expressions, so having that as a separate test case makes sense.
Refactor literal handling to use scoped, typed helpers instead of a generic value-based helper. Clean up unused TSTypeFlags and add integration tests for false, null, and resolved-type literals.
| // export declare const shiftLeft: 1 << 16; | ||
| // export declare const complex: 1 + 2 * 3; | ||
| export declare const existingLiteral: 1; | ||
| // export declare const existingLiteral: 1; |
There was a problem hiding this comment.
Yeah, let's re-enable to add some coverage for non-negative numbers. Probably doesn't make a difference in the code but the original bug highlighted prefix unary expressions, so having that as a separate test case makes sense.
| } | ||
|
|
||
| // Fallback to underlying type if not a literal | ||
| final underlyingTypeNode = typeChecker.typeToTypeNode( |
There was a problem hiding this comment.
I might have missed it, but which test case covers this?
|
It’s covered by the negativeOne (-1) case in |
|
Ack, I think the only thing left is the uncomment (and generate the expectation file) and we can go ahead and land this. |
|
@srujzs Done — re-enabled the test using a 1n BigInt to hit the fallback path and updated the expected output. All tests are passing. |
|
autosubmit label was removed for dart-lang/web/503, because This PR has not met approval requirements for merging. The PR author is not a member of dart-team and needs 1 more review(s) in order to merge this PR.
|
commit 48b7512 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Sun Mar 1 06:52:52 2026 +0000 Bump actions/stale from 10.1.1 to 10.2.0 in the github-actions group (dart-lang#520) Bumps the github-actions group with 1 update: [actions/stale](https://github.com/actions/stale). Updates `actions/stale` from 10.1.1 to 10.2.0 <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/actions/stale/releases">actions/stale's releases</a>.</em></p> <blockquote> <h2>v10.2.0</h2> <h2>What's Changed</h2> <h3>Bug Fix</h3> <ul> <li>Fix checking state cache (fix <a href="https://redirect.github.com/actions/stale/issues/1136">#1136</a>) and switch to Octokit helper methods by <a href="https://github.com/itchyny"><code>@itchyny</code></a> in <a href="https://redirect.github.com/actions/stale/pull/1152">actions/stale#1152</a></li> </ul> <h3>Dependency Updates</h3> <ul> <li>Upgrade js-yaml from 4.1.0 to 4.1.1 by <a href="https://github.com/dependabot"><code>@dependabot</code></a> in <a href="https://redirect.github.com/actions/stale/pull/1304">actions/stale#1304</a></li> <li>Upgrade lodash from 4.17.21 to 4.17.23 by <a href="https://github.com/dependabot"><code>@dependabot</code></a> in <a href="https://redirect.github.com/actions/stale/pull/1313">actions/stale#1313</a></li> <li>Upgrade actions/cache from 4.0.3 to 5.0.2 and actions/github from 5.1.1 to 7.0.0 by <a href="https://github.com/chiranjib-swain"><code>@chiranjib-swain</code></a> in <a href="https://redirect.github.com/actions/stale/pull/1312">actions/stale#1312</a></li> </ul> <h2>New Contributors</h2> <ul> <li><a href="https://github.com/itchyny"><code>@itchyny</code></a> made their first contribution in <a href="https://redirect.github.com/actions/stale/pull/1152">actions/stale#1152</a></li> </ul> <p><strong>Full Changelog</strong>: <a href="https://github.com/actions/stale/compare/v10...v10.2.0">https://github.com/actions/stale/compare/v10...v10.2.0</a></p> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/actions/stale/commit/b5d41d4e1d5dceea10e7104786b73624c18a190f"><code>b5d41d4</code></a> build(deps-dev): bump lodash from 4.17.21 to 4.17.23 (<a href="https://redirect.github.com/actions/stale/issues/1313">#1313</a>)</li> <li><a href="https://github.com/actions/stale/commit/dcd2b9469d2220b7e8d08aedc00c105d277fd46b"><code>dcd2b94</code></a> Fix punycode and url.parse Deprecation Warnings (<a href="https://redirect.github.com/actions/stale/issues/1312">#1312</a>)</li> <li><a href="https://github.com/actions/stale/commit/d6f8a33132340b15a7006f552936e4b9b39c00ec"><code>d6f8a33</code></a> build(deps-dev): bump js-yaml from 4.1.0 to 4.1.1 (<a href="https://redirect.github.com/actions/stale/issues/1304">#1304</a>)</li> <li><a href="https://github.com/actions/stale/commit/a21a0816299b11691f9592ef0d63d08e02f06d9d"><code>a21a081</code></a> Fix checking state cache (fix <a href="https://redirect.github.com/actions/stale/issues/1136">#1136</a>), also switch to octokit methods (<a href="https://redirect.github.com/actions/stale/issues/1152">#1152</a>)</li> <li>See full diff in <a href="https://github.com/actions/stale/compare/997185467fa4f803885201cee163a9f38240193d...b5d41d4e1d5dceea10e7104786b73624c18a190f">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore <dependency name> major version` will close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself) - `@dependabot ignore <dependency name> minor version` will close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself) - `@dependabot ignore <dependency name>` will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself) - `@dependabot unignore <dependency name>` will remove all of the ignore conditions of the specified dependency - `@dependabot unignore <dependency name> <ignore condition>` will remove the ignore condition of the specified dependency and ignore conditions </details> commit a6336c8 Author: Kevin Moore <kevmoo@users.noreply.github.com> Date: Mon Feb 23 11:09:23 2026 -0800 remove dynamic calls from Stream helpers (dart-lang#519) Mode CustomEventProviders `abstract final` Deleted a bunch of unused code in the streams.dart internal library commit 94d22fe Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue Feb 3 17:21:35 2026 -0800 Bump actions/checkout from 6.0.1 to 6.0.2 in the github-actions group (dart-lang#515) Bumps the github-actions group with 1 update: [actions/checkout](https://github.com/actions/checkout). Updates `actions/checkout` from 6.0.1 to 6.0.2 - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@8e8c483...de0fac2) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: 6.0.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github-actions ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 5edd2dd Author: Kevin Moore <kevmoo@users.noreply.github.com> Date: Tue Feb 3 17:18:04 2026 -0800 Fix use_null_aware_elements lint (dart-lang#518) commit d1958f8 Author: Sagar <halladakerisagar@gmail.com> Date: Wed Feb 4 06:29:06 2026 +0530 Support TypeScript indexed access types in interop_gen (dart-lang#509) * Support TypeScript indexed access types in interop_gen * Fix:prevent IndexedAccessType fallback from generating unsupported bindings * Remove unused indexed access fallback flag and simplify resolution logic * fixed analyzer issue * fix format issue * Improve IndexedAccessType handling and add composed type tests * interop_gen: refactor indexed access resolution, remove dead code, and Add test for symbol indexed access with primitive return type commit 2590763 Author: Harshita Yadav <seemayadavanuj123@gmail.com> Date: Mon Jan 26 23:40:27 2026 +0530 docs: clarify document.cookie nullability when migrating from dart:html (dart-lang#510) * docs: clarify document.cookie nullability when migrating from dart:html * docs: organize migration notes under specific APIs commit b9b1b38 Author: Sagar <halladakerisagar@gmail.com> Date: Sat Jan 17 02:31:52 2026 +0530 Interop gen additional literal types (dart-lang#503) * interop_gen: support additional literal type expressions via TypeChecker * fixed analyzer warning * simplify literal parsing and improve readability * Refactor literal handling and expand literal tests Use a shared helper to reduce duplication, remove unused TSTypeFlags, and add integration tests for false, null, and negative numeric literals. * Use typed literal helpers in _transformType and add tests Refactor literal handling to use scoped, typed helpers instead of a generic value-based helper. Clean up unused TSTypeFlags and add integration tests for false, null, and resolved-type literals. * fix CI issue * enable fallback literal coverage and update expectations commit 1dfcee7 Author: Harshita Yadav <seemayadavanuj123@gmail.com> Date: Fri Jan 16 03:21:37 2026 +0530 docs(web_generator): document minimum supported Node.js version (dart-lang#508) * docs(web_generator): document minimum supported Node.js version * docs(web_generator): simplify Node.js version wording
This PR improves interop_gen support for TypeScript literal types that aren’t represented as simple literals in the AST (for example, prefix unary expressions like -1).
When a literal can’t be handled directly, the generator now falls back to the TypeScript TypeChecker to resolve the type. If the resolved type is a literal, its value is used; otherwise, the code safely falls back to the underlying Dart type, preserving existing behavior.
The change also adds the minimal TypeScript bindings needed for this resolution and includes an integration test to cover the new case and prevent regressions.
Fixes:#465
Contribution guidelines: