feat(type-info): add Pick and Omit type synthesis#9874
feat(type-info): add Pick and Omit type synthesis#9874ematipico merged 12 commits intobiomejs:mainfrom
Conversation
…ypes Skip synthesis when T resolves to Union/Intersection/Reference instead of producing an empty object. Also deduplicates the fallback path for unresolved K and T.
…action, add valid tests
🦋 Changeset detectedLatest commit: 438a40b 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 |
Merging this PR will not alter performance
Comparing Footnotes
|
WalkthroughAdds Pick/Omit-aware resolution to the type resolver: when a Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ematipico
left a comment
There was a problem hiding this comment.
The synthesis of Pick and Omit has definitely repercussions on some of our type-aware lint rules. This means:
- we should add more tests for the other lint rules (it seems to added tests for one already)
- we must have a changeset, notably focused towards what the lint rules can do now
1ec79bc to
38748cf
Compare
38748cf to
af11c8a
Compare
@ematipico Added test cases for the 8 type-aware rules in c96f270, and the changeset in 61ac64d. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_js_analyze/tests/specs/nursery/noUselessTypeConversion/invalid.ts (1)
58-67: Optional: add Omit parity for number/boolean too.You already cover
Pickfor number/boolean; adding matchingOmitvariants would make regressions even harder to sneak in. Not required, just a tidy completeness win.🤖 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/noUselessTypeConversion/invalid.ts` around lines 58 - 67, Add matching Omit test cases for the number and boolean cases to mirror the existing Pick variants: declare an omittedNum using Omit<{ n: number; s: string }, "s"> and call Number(omittedNum.n) and reference omittedNum.n (mirroring pickedNum), and declare an omittedFlag using Omit<{ flag: boolean; label: string }, "label"> and call Boolean(omittedFlag.flag) and use !!omittedFlag.flag (mirroring pickedBool); place these next to the existing omittedStr / pickedNum / pickedBool blocks so the number/boolean Omit parity is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@crates/biome_js_analyze/tests/specs/nursery/noUselessTypeConversion/invalid.ts`:
- Around line 58-67: Add matching Omit test cases for the number and boolean
cases to mirror the existing Pick variants: declare an omittedNum using Omit<{
n: number; s: string }, "s"> and call Number(omittedNum.n) and reference
omittedNum.n (mirroring pickedNum), and declare an omittedFlag using Omit<{
flag: boolean; label: string }, "label"> and call Boolean(omittedFlag.flag) and
use !!omittedFlag.flag (mirroring pickedBool); place these next to the existing
omittedStr / pickedNum / pickedBool blocks so the number/boolean Omit parity is
covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6456471d-6337-4d7a-9e3b-1613861cc18f
⛔ Files ignored due to path filters (8)
crates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noMisusedPromises/checksConditionalsInvalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noUnsafePlusOperands/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noUselessTypeConversion/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useArraySortCompare/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useExhaustiveSwitchCases/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/invalid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (10)
.changeset/smooth-ideas-write.mdcrates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/invalid.tscrates/biome_js_analyze/tests/specs/nursery/noMisusedPromises/checksConditionalsInvalid.tscrates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/invalid.tscrates/biome_js_analyze/tests/specs/nursery/noUnsafePlusOperands/invalid.tscrates/biome_js_analyze/tests/specs/nursery/noUselessTypeConversion/invalid.tscrates/biome_js_analyze/tests/specs/nursery/useArraySortCompare/invalid.tscrates/biome_js_analyze/tests/specs/nursery/useExhaustiveSwitchCases/invalid.tscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/invalid.tscrates/biome_js_type_info/src/resolver.rs
✅ Files skipped from review due to trivial changes (1)
- .changeset/smooth-ideas-write.md
ematipico
left a comment
There was a problem hiding this comment.
Thank you! Need to fix the merge conflicts
| let mut names = Vec::new(); | ||
| for ty in union.types() { | ||
| let resolved = resolver.resolve_and_get(ty)?; | ||
| let TypeData::Literal(lit) = resolved.to_data() else { | ||
| return None; | ||
| }; | ||
| let Literal::String(s) = lit.as_ref() else { | ||
| return None; | ||
| }; | ||
| names.push(s.as_ref().clone()); | ||
| } |
There was a problem hiding this comment.
I think this can be a written using types().filter_map().collect()
There was a problem hiding this comment.
Done in f215a04. Used .iter() before filter_map since types() returns a slice.
9321c99 to
f215a04
Compare
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/resolver.rs`:
- Around line 845-850: When synthesising Pick/Omit objects you're currently
hardcoding prototype: None in the TypeData::Object creation (inside the
resolver.register_and_resolve call); instead fetch and preserve the original
object's prototype value and use it when creating the new Object so prototype
metadata isn't lost. Locate where members is derived and the original
TypeData/Object instance for T, extract its prototype (e.g.
original_object.prototype or similar) and pass that value into the new Object's
prototype field rather than None; keep using members.into() for members and
ensure any Clone or ownership adjustments are handled.
- Around line 967-982: The current TypeData::Union arm uses filter_map which
silently drops non-string members and returns Some(vec) even if some union
members are invalid; change the logic in the TypeData::Union branch (the block
that calls resolver.resolve_and_get for each ty) to fail the whole operation
when any union member is not a Literal::String: iterate over union.types(), call
resolver.resolve_and_get(ty) for each, check that resolved.to_data() matches
TypeData::Literal and that lit is Literal::String, and if any check fails return
None; otherwise collect the string values into a Vec and return Some(vec).
Ensure you reference the same symbols (TypeData::Union,
resolver.resolve_and_get, TypeData::Literal, Literal::String) when applying the
fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9772a79-ea38-4b19-abfc-3b25adebfc83
📒 Files selected for processing (1)
crates/biome_js_type_info/src/resolver.rs
| if let Some(members) = members { | ||
| let resolved_id: ResolvedTypeId = resolver.register_and_resolve( | ||
| TypeData::Object(Box::new(Object { | ||
| prototype: None, | ||
| members: members.into(), | ||
| })), |
There was a problem hiding this comment.
Preserve the prototype when synthesising Pick/Omit objects
Line 848 hardcodes prototype: None. If T is an object with a prototype, that information is lost in the synthesised type.
Proposed fix
- if let Some(members) = members {
+ if let Some(members) = members {
+ let prototype = match &t_data {
+ TypeData::Object(object) => object.prototype.clone(),
+ _ => None,
+ };
let resolved_id: ResolvedTypeId = resolver.register_and_resolve(
TypeData::Object(Box::new(Object {
- prototype: None,
+ prototype,
members: members.into(),
})),
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_js_type_info/src/resolver.rs` around lines 845 - 850, When
synthesising Pick/Omit objects you're currently hardcoding prototype: None in
the TypeData::Object creation (inside the resolver.register_and_resolve call);
instead fetch and preserve the original object's prototype value and use it when
creating the new Object so prototype metadata isn't lost. Locate where members
is derived and the original TypeData/Object instance for T, extract its
prototype (e.g. original_object.prototype or similar) and pass that value into
the new Object's prototype field rather than None; keep using members.into() for
members and ensure any Clone or ownership adjustments are handled.
…-synthesis # Conflicts: # crates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/invalid.ts # crates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/invalid.ts.snap # crates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/valid.ts # crates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/valid.ts.snap
0939aa7 to
438a40b
Compare
@ematipico Resolved merge conflicts in 438a40b. |
I used Claude Code to assist with this implementation.
Summary
Addresses items from #9810: synthesizes
Pick<T, K>andOmit<T, K>following the existingRecord<K, V>pattern.Resolves T, extracts string literal keys from K, and filters members accordingly. Falls back to generic instantiation when T or K can't be resolved.
Test Plan
Snapshot tests.