Merged
Conversation
🦋 Changeset detectedLatest commit: 7d98550 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
A-Linter
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds multiple TypeScript test specs exercising Record-typed containers, optional chaining (including optional calls) and promise-related diagnostics across nursery linters (noFloatingPromises, noMisusedPromises, useArraySortCompare, useAwaitThenable). Resolver now synthesises Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@crates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/issue6606_invalid.ts`:
- Around line 3-20: Add a negative test covering promise-returning values read
through Record by declaring a type like HandlerRecord = Record<string, (() =>
Promise<void>) | undefined> and a function (e.g., callRecord or callHandlers)
that performs an optional-chained call on a record entry (for example:
handlers['x']?.()); this should be treated as a floating Promise and fail the
same as svc.client?.request(), so add that case to the same file alongside
AsyncApi/callAsyncApi and Service/callService.
In `@crates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/issue6606.ts`:
- Around line 48-50: Update the leading comment to accurately describe the code:
replace "Ternary with optional chaining — neither branch is a Promise" with
something like "Nullish coalescing with optional chaining — neither branch is a
Promise", referencing the config variable and its getValue method and the use of
the nullish coalescing operator (??) in the result assignment to match the
actual expression config?.getValue?.() ?? "fallback".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8c23e52-0377-4900-80c2-5788dc58416b
⛔ Files ignored due to path filters (2)
crates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/issue6606.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/issue6606_invalid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (2)
crates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/issue6606.tscrates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/issue6606_invalid.ts
crates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/issue6606_invalid.ts
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/issue6606.ts
Outdated
Show resolved
Hide resolved
ematipico
force-pushed
the
feat/record-floating-promises
branch
from
b46ff18 to
cce3adc
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
ematipico
changed the title
Recored<K, V>
ematipico
force-pushed
the
feat/record-floating-promises
branch
from
cce3adc to
37ef465
Compare
ematipico
force-pushed
the
feat/record-floating-promises
branch
from
37ef465 to
3a46637
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_js_type_info/src/flattening/expressions.rs (1)
377-383:⚠️ Potential issue | 🟠 MajorKeep the original
ResolverIdwhen walking callable objects.Lines 378-379 invent a fresh
ResolverIdfrom the current level. On thin inference that rewrites member refs to module0insidemember.deref_ty(), so callable interfaces/objects can resolve against the wrong module; please thread the originalResolvedTypeDataorResolverIdthrough this helper instead.Based on learnings,
crates/biome_js_type_info/**/*.rsshould "When usingResolvedTypeData, track theResolverIdto ensure subsequent resolver calls use the correct context".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_type_info/src/flattening/expressions.rs` around lines 377 - 383, The code creates a fresh ResolverId via ResolverId::from_level(resolver.level()) when building ResolvedTypeData before find_member, which loses the original resolution context and causes member.deref_ty() to resolve against module 0; instead keep or pass through the original ResolverId/ResolvedTypeData so subsequent resolves use the correct context: replace the ResolverId::from_level(...) construction in the TypeData::Interface/Object branch and use the ResolvedTypeData that already carries its ResolverId (or add a parameter to thread the ResolverId through the helper), then call find_member, ResolvedTypeMember::to_member, member.deref_ty(resolver) and resolver.resolve_and_get(...) with that preserved ResolverId/ResolvedTypeData so resolution happens in the original module context.
🧹 Nitpick comments (6)
crates/biome_js_analyze/tests/specs/nursery/useArraySortCompare/recordValid.ts (1)
1-6: Please rename this fixture to follow the usualvalid*pattern.The test itself looks spot on, but
recordValid.tsbreaks the normaltests/specs/naming convention.validRecord.tswould be easier to spot when skimming the suite. Tiny papercut, easy win.Based on learnings, create snapshot tests inside
tests/specs/and use files prefixed withvalidfor code that doesn't trigger the rule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/tests/specs/nursery/useArraySortCompare/recordValid.ts` around lines 1 - 6, Rename the fixture file recordValid.ts to validRecord.ts and update any test harness references to that filename so the suite follows the tests/specs/ naming convention; ensure the existing content (the const groups: Record<string, number[]> and the sort/toSorted comparator lines) stays unchanged and add or update snapshot tests under tests/specs/ using the new validRecord.ts filename so non-triggering examples use the valid* prefix.crates/biome_js_analyze/tests/specs/nursery/useArraySortCompare/recordInvalid.ts (1)
1-6: Please rename this fixture to follow the usualinvalid*pattern.Same tiny naming nit here:
recordInvalid.tsreads fine, butinvalidRecord.tsmatches the standard fixture convention and makes the invalid cases easier to scan.Based on learnings, create snapshot tests inside
tests/specs/and use files prefixed withinvalidfor code that triggers the rule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/tests/specs/nursery/useArraySortCompare/recordInvalid.ts` around lines 1 - 6, Rename the fixture file from recordInvalid.ts to invalidRecord.ts and update any test harness or import references that mention recordInvalid; ensure the file still contains the same invalid test code (the groups variable and the .sort/.toSorted calls) and create or update the corresponding snapshot test under the specs test suite so it follows the convention of using files prefixed with "invalid" for rule-triggering cases.crates/biome_js_analyze/tests/specs/nursery/useAwaitThenable/recordValid.ts (2)
11-14: Tighten the comment aboveAsyncHandlers.Line 11 says this is a “Record value typed as Promise”, but this case is actually a Record of functions returning a Promise. Tiny nit, but it saves the next maintainer a double-take.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/tests/specs/nursery/useAwaitThenable/recordValid.ts` around lines 11 - 14, Update the comment above the type alias AsyncHandlers to accurately describe that it is a Record of functions returning a Promise (not a Record value typed as Promise); locate the type alias AsyncHandlers and the asyncHandlers variable and replace the misleading comment with a concise description like "Record of functions returning Promise<void>" so future readers understand the shape is functions that return Promises rather than Promise-valued entries.
1-14: Rename this spec to follow thevalid*convention.Same story here:
recordValid.tsreads fine to humans, but the documented pattern fortests/specs/is prefixing valid cases. Something likevalidRecord.tswould match the house style. Based on learnings, tests undercrates/biome_analyze/tests/specs/**/*should use files prefixed withinvalidfor code that triggers the rule andvalidfor code that doesn't.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/tests/specs/nursery/useAwaitThenable/recordValid.ts` around lines 1 - 14, Rename the test file to follow the project's naming convention by changing recordValid.ts to validRecord.ts; update any test-suite references if present to the new name and preserve the file content (symbols to locate the tests: cache, fetchers, AsyncHandlers, asyncHandlers) so the spec remains a valid case that should not trigger diagnostics.crates/biome_js_analyze/tests/specs/nursery/useAwaitThenable/recordInvalid.ts (2)
7-9: Consider adding the optional-chaining negative case too.This file covers plain Record lookups, but the linked bug also hinges on optional calls through Record values. Adding a
Record<string, (() => number) | undefined>case withawait maybeGetters["count"]?.();would pin the false-negative branch as well.Possible addition
// Awaiting a Record function value that returns a non-Promise const getters: Record<string, () => number> = {}; await getters["count"](); + +// Awaiting an optional Record function value that still resolves to a non-thenable +const maybeGetters: Record<string, (() => number) | undefined> = {}; +await maybeGetters["count"]?.();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/tests/specs/nursery/useAwaitThenable/recordInvalid.ts` around lines 7 - 9, Add an optional-chaining negative case to mirror the existing Record test: declare a variable like maybeGetters as Record<string, (() => number) | undefined> and then await the optional call with await maybeGetters["count"]?.(); this reproduces the Record lookup that can be undefined and ensures the optional-chaining branch (maybeGetters and the ?.() call) is exercised alongside the existing getters and await getters["count"]().
1-9: Rename this spec to follow theinvalid*convention.This sits under
tests/specs/, butrecordInvalid.tsuses the suffix form rather than the documented invalid-case prefix. I’d rename it to something likeinvalidRecord.tsso the test tree stays predictable. Based on learnings, tests undercrates/biome_analyze/tests/specs/**/*should use files prefixed withinvalidfor code that triggers the rule andvalidfor code that doesn't.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/tests/specs/nursery/useAwaitThenable/recordInvalid.ts` around lines 1 - 9, Rename the spec file from recordInvalid.ts to follow the project convention by using the invalid* prefix (e.g., invalidRecord.ts); ensure the test file that contains the failing cases (awaiting a Record value and awaiting a Record function return) is renamed so it sits alongside other invalid-* tests and continues to export/run the same test content without code changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_type_info/src/flattening/expressions.rs`:
- Around line 397-431: The loop in TypeData::Union currently skips
GLOBAL_UNDEFINED_ID (in the union branch handling callable variants) which drops
the undefined arm of optional callables; instead of continuing when *variant ==
GLOBAL_UNDEFINED_ID.into(), record the undefined type into return_types so the
final union includes both the callable's return and undefined. Concretely, in
the loop around resolver.resolve_and_get / resolve_callee_to_function /
flattened_function_call, replace the continue on GLOBAL_UNDEFINED_ID with code
that creates a TypeData/TypeReference representing undefined (e.g., register or
reference the global undefined type via the resolver) and push that into
return_types, preserving the existing logic for callable variants and the
subsequent union assembly.
In `@crates/biome_js_type_info/src/resolver.rs`:
- Around line 759-772: The code builds a synthetic Object for a Record using
cloned qualifier.type_parameters (key_type/value_type) which can later be
blanked to Unknown; instead, resolve or register the qualifier type parameters
into proper TypeReference variants before constructing and registering the
synthetic object: for the two entries in qualifier.type_parameters used in the
Record branch, convert each into a resolved/import/qualifier TypeReference via
the resolver (e.g., using the same register/resolve path you use elsewhere) and
pass those TypeReference results into the TypeMember::IndexSignature and
TypeMember.ty when creating the Object, then call
resolver.register_and_resolve(TypeData::Object(...)) so the synthetic object
stores stable TypeReference variants rather than raw qualifier clones.
---
Outside diff comments:
In `@crates/biome_js_type_info/src/flattening/expressions.rs`:
- Around line 377-383: The code creates a fresh ResolverId via
ResolverId::from_level(resolver.level()) when building ResolvedTypeData before
find_member, which loses the original resolution context and causes
member.deref_ty() to resolve against module 0; instead keep or pass through the
original ResolverId/ResolvedTypeData so subsequent resolves use the correct
context: replace the ResolverId::from_level(...) construction in the
TypeData::Interface/Object branch and use the ResolvedTypeData that already
carries its ResolverId (or add a parameter to thread the ResolverId through the
helper), then call find_member, ResolvedTypeMember::to_member,
member.deref_ty(resolver) and resolver.resolve_and_get(...) with that preserved
ResolverId/ResolvedTypeData so resolution happens in the original module
context.
---
Nitpick comments:
In
`@crates/biome_js_analyze/tests/specs/nursery/useArraySortCompare/recordInvalid.ts`:
- Around line 1-6: Rename the fixture file from recordInvalid.ts to
invalidRecord.ts and update any test harness or import references that mention
recordInvalid; ensure the file still contains the same invalid test code (the
groups variable and the .sort/.toSorted calls) and create or update the
corresponding snapshot test under the specs test suite so it follows the
convention of using files prefixed with "invalid" for rule-triggering cases.
In
`@crates/biome_js_analyze/tests/specs/nursery/useArraySortCompare/recordValid.ts`:
- Around line 1-6: Rename the fixture file recordValid.ts to validRecord.ts and
update any test harness references to that filename so the suite follows the
tests/specs/ naming convention; ensure the existing content (the const groups:
Record<string, number[]> and the sort/toSorted comparator lines) stays unchanged
and add or update snapshot tests under tests/specs/ using the new validRecord.ts
filename so non-triggering examples use the valid* prefix.
In
`@crates/biome_js_analyze/tests/specs/nursery/useAwaitThenable/recordInvalid.ts`:
- Around line 7-9: Add an optional-chaining negative case to mirror the existing
Record test: declare a variable like maybeGetters as Record<string, (() =>
number) | undefined> and then await the optional call with await
maybeGetters["count"]?.(); this reproduces the Record lookup that can be
undefined and ensures the optional-chaining branch (maybeGetters and the ?.()
call) is exercised alongside the existing getters and await getters["count"]().
- Around line 1-9: Rename the spec file from recordInvalid.ts to follow the
project convention by using the invalid* prefix (e.g., invalidRecord.ts); ensure
the test file that contains the failing cases (awaiting a Record value and
awaiting a Record function return) is renamed so it sits alongside other
invalid-* tests and continues to export/run the same test content without code
changes.
In `@crates/biome_js_analyze/tests/specs/nursery/useAwaitThenable/recordValid.ts`:
- Around line 11-14: Update the comment above the type alias AsyncHandlers to
accurately describe that it is a Record of functions returning a Promise (not a
Record value typed as Promise); locate the type alias AsyncHandlers and the
asyncHandlers variable and replace the misleading comment with a concise
description like "Record of functions returning Promise<void>" so future readers
understand the shape is functions that return Promises rather than
Promise-valued entries.
- Around line 1-14: Rename the test file to follow the project's naming
convention by changing recordValid.ts to validRecord.ts; update any test-suite
references if present to the new name and preserve the file content (symbols to
locate the tests: cache, fetchers, AsyncHandlers, asyncHandlers) so the spec
remains a valid case that should not trigger diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c8561a2-d2d1-4ab1-a49b-7ce7145f63de
⛔ Files ignored due to path filters (8)
crates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/issue6606.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/issue6606_invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noMisusedPromises/recordInvalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noMisusedPromises/recordValid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useArraySortCompare/recordInvalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useArraySortCompare/recordValid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useAwaitThenable/recordInvalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useAwaitThenable/recordValid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (12)
.changeset/resolve-record-type.mdcrates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/issue6606.tscrates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/issue6606_invalid.tscrates/biome_js_analyze/tests/specs/nursery/noMisusedPromises/recordInvalid.tscrates/biome_js_analyze/tests/specs/nursery/noMisusedPromises/recordValid.tscrates/biome_js_analyze/tests/specs/nursery/useArraySortCompare/recordInvalid.tscrates/biome_js_analyze/tests/specs/nursery/useArraySortCompare/recordValid.tscrates/biome_js_analyze/tests/specs/nursery/useAwaitThenable/recordInvalid.tscrates/biome_js_analyze/tests/specs/nursery/useAwaitThenable/recordValid.tscrates/biome_js_type_info/src/flattening/expressions.rscrates/biome_js_type_info/src/resolver.rscrates/biome_js_type_info/src/type_data.rs
✅ Files skipped from review due to trivial changes (1)
- .changeset/resolve-record-type.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/issue6606.ts
- crates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/issue6606_invalid.ts
dyc3
approved these changes
Mar 10, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_type_info/src/flattening/expressions.rs`:
- Around line 405-411: The code unconditionally treats a null/undefined callee
as short-circuiting to undefined (in the match on resolved.as_raw_data() for
TypeData::Undefined | TypeData::Null), but this should only happen for optional
calls; update the type layer to carry optional-call info (e.g., add a boolean
flag on TypeofCallExpression or on the flattened_call struct when creating it
from JsCallExpression using JsCallExpression::optional_chain_token()), then
change this match to only push TypeData::Undefined into return_types when that
optional flag is true; otherwise, do not short-circuit and let the non-optional
case produce a type error or be handled elsewhere.
In `@crates/biome_js_type_info/src/resolver.rs`:
- Around line 787-794: The generic instantiation currently registers
TypeInstance with raw qualifier.type_parameters, which can lose
resolved/module-context type arguments; change the call in
resolver.register_and_resolve(TypeData::instance_of(TypeInstance { ... })) to
use the already-resolved type argument list instead of qualifier.type_parameters
— i.e., pass Self::merge_parameters(parameters, &<resolved-args>) where
<resolved-args> is the resolved type-argument vector you computed earlier (the
same resolved args used to form resolved_id) so TypeInstance uses the resolved
parameters rather than the raw qualifier.type_parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3efec958-d52e-4179-bf41-5fd21e5adfa7
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/nursery/useAwaitThenable/recordInvalid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (4)
crates/biome_js_analyze/src/lint/nursery/use_await_thenable.rscrates/biome_js_analyze/tests/specs/nursery/useAwaitThenable/recordInvalid.tscrates/biome_js_type_info/src/flattening/expressions.rscrates/biome_js_type_info/src/resolver.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_js_analyze/tests/specs/nursery/useAwaitThenable/recordInvalid.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b779adb5-9fa5-4577-b806-c1a295a3afa9
📒 Files selected for processing (1)
crates/biome_js_type_info/src/resolver.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #6606
The inference now infers
Record<K, V>. I let the AI agent drive this one, the code seems to be reasonably correct.Test Plan
Added new tests
Tests generated via AI
Docs