fix(lint): fix false positive in noMisleadingReturnType for nested object widening#10006
Conversation
🦋 Changeset detectedLatest commit: 952eef2 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 |
dbb2fd6 to
5b79e61
Compare
|
|
||
| while let Some((ann, inf)) = stack.pop() { | ||
| iterations += 1; | ||
| if iterations > 50 { |
There was a problem hiding this comment.
Same iteration guard as is_nonunion_wider. Prevents infinite loops on cyclic types like interface Node { parent: Node }. 50 is a generous ceiling - real code never nests anywhere near that deep.
There was a problem hiding this comment.
Don't use magic numbers though. Create a constant and document
There was a problem hiding this comment.
is_nonunion_wider already uses the same 50 for the same iteration guard. Extract both into one shared constant in this PR, or split it into a separate cleanup PR?
There was a problem hiding this comment.
@ematipico 952eef2 extracts both into a shared constant. Let me know if you'd rather have a separate PR for it.
Merging this PR will not alter performance
Comparing Footnotes
|
|
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:
WalkthroughRefactors Possibly related PRs
Suggested labels
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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/tidy-planes-stay.md:
- Line 5: Update the changeset description in .changeset/tidy-planes-stay.md to
include a link to the referenced issue (`#9810`); edit the single-line summary
("Fixed `noMisleadingReturnType` incorrectly flagging nested object literals
with widened properties.") or the body to append the issue URL (e.g. "See:
https://github.com/<org>/<repo>/issues/9810" or the appropriate repo link) so
the bug-fix changeset contains the issue reference per guidelines.
🪄 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: 20347756-606a-44d8-a0de-25b550290520
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/valid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
.changeset/tidy-planes-stay.mdcrates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rscrates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/valid.ts
5b79e61 to
65f01b5
Compare
65f01b5 to
e79b09d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs (1)
467-495: Optional: index-signature branch doesn't recurse into nested value types.Unlike the named-member branch below (which pushes mismatched pairs onto the stack), the index-signature path only accepts matches via
types_matchoris_base_type_of_literal. So an annotation likeRecord<string, { x: string }>against an inferred{ a: { x: "y" } }would bail out here rather than being recognised as pure literal widening. Not a regression introduced by this PR and arguably outside its scope — just flagging for a potential follow-up so the two branches stay symmetrical.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs` around lines 467 - 495, The index-signature branch (starting at annotated_index_signature and using index_signature_value_type) currently only accepts direct matches via types_match or is_base_type_of_literal and therefore doesn't recurse into nested value types; update this branch to mirror the named-member logic by, when a mismatch is found between index_signature_value_type and an inferred_type (and it isn't a simple base/literal match), push the pair (index_signature_value_type, inferred_type) onto the same comparison stack (or invoke the same recursion routine used by the named-member branch) so nested object literal widening (e.g., Record<string, { x: string }> vs { a: { x: "y" } }) is recognized; keep the existing index_signature_has_widening handling for base-of-literal cases and only return false if the pushed comparisons ultimately fail.
🤖 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/src/lint/nursery/no_misleading_return_type.rs`:
- Around line 467-495: The index-signature branch (starting at
annotated_index_signature and using index_signature_value_type) currently only
accepts direct matches via types_match or is_base_type_of_literal and therefore
doesn't recurse into nested value types; update this branch to mirror the
named-member logic by, when a mismatch is found between
index_signature_value_type and an inferred_type (and it isn't a simple
base/literal match), push the pair (index_signature_value_type, inferred_type)
onto the same comparison stack (or invoke the same recursion routine used by the
named-member branch) so nested object literal widening (e.g., Record<string, {
x: string }> vs { a: { x: "y" } }) is recognized; keep the existing
index_signature_has_widening handling for base-of-literal cases and only return
false if the pushed comparisons ultimately fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aee22f9e-80cb-49ac-8c81-f9330b5245c2
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/valid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
.changeset/tidy-planes-stay.mdcrates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rscrates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/valid.ts
✅ Files skipped from review due to trivial changes (2)
- .changeset/tidy-planes-stay.md
- crates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/valid.ts
e79b09d to
9425f15
Compare
99f712b to
38a6526
Compare
This PR contains the following updates: | Package | Update | Change | |---|---|---| | [@biomejs/biome](https://biomejs.dev) ([source](https://github.com/biomejs/biome/tree/HEAD/packages/@biomejs/biome)) | patch | `2.4.12` → `2.4.13` | --- ### Release Notes <details> <summary>biomejs/biome (@​biomejs/biome)</summary> ### [`v2.4.13`](https://github.com/biomejs/biome/blob/HEAD/packages/@​biomejs/biome/CHANGELOG.md#2413) [Compare Source](https://github.com/biomejs/biome/compare/@biomejs/biome@2.4.12...@biomejs/biome@2.4.13) ##### Patch Changes - [#​9969](biomejs/biome#9969) [`c5eb92b`](biomejs/biome@c5eb92b) Thanks [@​officialasishkumar](https://github.com/officialasishkumar)! - Added the nursery rule [`noUnnecessaryTemplateExpression`](https://biomejs.dev/linter/rules/no-unnecessary-template-expression/), which disallows template literals that only contain string literal expressions. These can be replaced with a simpler string literal. For example, the following code triggers the rule: ```js const a = `${"hello"}`; // can be 'hello' const b = `${"prefix"}_suffix`; // can be 'prefix_suffix' const c = `${"a"}${"b"}`; // can be 'ab' ``` - [#​10037](biomejs/biome#10037) [`f785e8c`](biomejs/biome@f785e8c) Thanks [@​minseong0324](https://github.com/minseong0324)! - Fixed [#​9810](biomejs/biome#9810): [`noMisleadingReturnType`](https://biomejs.dev/linter/rules/no-misleading-return-type/) no longer reports false positives on a getter with a matching setter in the same namespace. ```ts class Store { get status(): string { if (Math.random() > 0.5) return "loading"; return "idle"; } set status(v: string) {} } ``` - [#​10084](biomejs/biome#10084) [`5e2f90c`](biomejs/biome@5e2f90c) Thanks [@​jiwon79](https://github.com/jiwon79)! - Fixed [#​10034](biomejs/biome#10034): [`noUselessEscapeInRegex`](https://biomejs.dev/linter/rules/no-useless-escape-in-regex/) no longer flags escapes of `ClassSetReservedPunctuator` characters (`&`, `!`, `#`, `%`, `,`, `:`, `;`, `<`, `=`, `>`, `@`, `` ` ``, `~`) inside `v`-flag character classes as useless. These characters are reserved as individual code points in `v`-mode, so the escape is required. The following pattern is now considered valid: ```js /[a-z\&]/v; ``` - [#​10063](biomejs/biome#10063) [`c9ffa16`](biomejs/biome@c9ffa16) Thanks [@​Netail](https://github.com/Netail)! - Added extra rule sources from ESLint CSS. `biome migrate eslint` should do a bit better detecting rules in your eslint configurations. - [#​10035](biomejs/biome#10035) [`946b50e`](biomejs/biome@946b50e) Thanks [@​Netail](https://github.com/Netail)! - Fixed [#​10032](biomejs/biome#10032): [useIframeSandbox](https://biomejs.dev/linter/rules/use-iframe-sandbox/) now flags if there's no initializer value. - [#​9865](biomejs/biome#9865) [`68fb8d4`](biomejs/biome@68fb8d4) Thanks [@​dyc3](https://github.com/dyc3)! - Added the new nursery rule [`useDomNodeTextContent`](https://biomejs.dev/linter/rules/use-dom-node-text-content/), which prefers `textContent` over `innerText` for DOM node text access and destructuring. For example, the following snippet triggers the rule: ```js const foo = node.innerText; ``` - [#​10023](biomejs/biome#10023) [`bd1e74f`](biomejs/biome@bd1e74f) Thanks [@​ematipico](https://github.com/ematipico)! - Added a new nursery rule [`noReactNativeDeepImports`](https://biomejs.dev/linter/rules/no-react-native-deep-imports/) that disallows deep imports from the `react-native` package. Internal paths like `react-native/Libraries/...` are not part of the public API and may change between versions. For example, the following code triggers the rule: ```js import View from "react-native/Libraries/Components/View/View"; ``` - [#​9885](biomejs/biome#9885) [`3dce737`](biomejs/biome@3dce737) Thanks [@​dyc3](https://github.com/dyc3)! - Added a new nursery rule [`useDomQuerySelector`](https://biomejs.dev/linter/rules/use-dom-query-selector/) that prefers `querySelector()` and `querySelectorAll()` over older DOM query methods such as `getElementById()` and `getElementsByClassName()`. - [#​9995](biomejs/biome#9995) [`4da9caf`](biomejs/biome@4da9caf) Thanks [@​siketyan](https://github.com/siketyan)! - Fixed [#​9994](biomejs/biome#9994): Biome now parses nested CSS rules correctly when declarations follow them inside embedded snippets. - [#​10009](biomejs/biome#10009) [`b41cc5a`](biomejs/biome@b41cc5a) Thanks [@​Jayllyz](https://github.com/Jayllyz)! - Fixed [#​10004](biomejs/biome#10004): [`noComponentHookFactories`](https://biomejs.dev/linter/rules/no-component-hook-factories/) no longer reports false positives for object methods and class methods. - [#​9988](biomejs/biome#9988) [`eabf54a`](biomejs/biome@eabf54a) Thanks [@​Netail](https://github.com/Netail)! - Tweaked the diagnostics range for [useAltText](https://biomejs.dev/linter/rules/use-alt-text), [useButtonType](https://biomejs.dev/linter/rules/use-button-type), [useHtmlLang](https://biomejs.dev/linter/rules/use-html-lang), [useIframeTitle](https://biomejs.dev/linter/rules/use-iframe-title), [useValidAriaRole](https://biomejs.dev/linter/rules/use-valid-aria-role) & [useIfameSandbox](https://biomejs.dev/linter/rules/use-iframe-sandbox) to report on the opening tag instead of the full tag. - [#​10043](biomejs/biome#10043) [`fc65902`](biomejs/biome@fc65902) Thanks [@​mujpao](https://github.com/mujpao)! - Fixed [#​10003](biomejs/biome#10003): Biome no longer panics when parsing Svelte files containing `{#}`. - [#​9815](biomejs/biome#9815) [`5cc83b1`](biomejs/biome@5cc83b1) Thanks [@​dyc3](https://github.com/dyc3)! - Added the new nursery rule [`noLoopFunc`](https://biomejs.dev/linter/rules/no-loop-func/). When enabled, it warns when a function declared inside a loop captures outer variables that can change across iterations. - [#​9702](biomejs/biome#9702) [`ef470ba`](biomejs/biome@ef470ba) Thanks [@​ryan-m-walker](https://github.com/ryan-m-walker)! - Added the nursery rule [`useRegexpTest`](https://biomejs.dev/linter/rules/use-regexp-test/) that enforces `RegExp.prototype.test()` over `String.prototype.match()` and `RegExp.prototype.exec()` in boolean contexts. `test()` returns a boolean directly, avoiding unnecessary computation of match results. **Invalid** ```js if ("hello world".match(/hello/)) { } ``` **Valid** ```js if (/hello/.test("hello world")) { } ``` - [#​9743](biomejs/biome#9743) [`245307d`](biomejs/biome@245307d) Thanks [@​leetdavid](https://github.com/leetdavid)! - Fixed [#​2245](biomejs/biome#2245): Svelte `<script>` tag language detection when the `generics` attribute contains `>` characters (e.g., `<script lang="ts" generics="T extends Record<string, unknown>">`). Biome now correctly recognizes TypeScript in such script blocks. - [#​10046](biomejs/biome#10046) [`0707de7`](biomejs/biome@0707de7) Thanks [@​Conaclos](https://github.com/Conaclos)! - Fixed [#​10038](biomejs/biome#10038): [`organizeImports`](https://biomejs.dev/assist/actions/organize-imports/) now sorts imports in TypeScript modules and declaration files. ```diff declare module "mymodule" { - import type { B } from "b"; import type { A } from "a"; + import type { B } from "b"; } ``` - [#​10012](biomejs/biome#10012) [`94ccca9`](biomejs/biome@94ccca9) Thanks [@​ematipico](https://github.com/ematipico)! - Added the nursery rule [`noReactNativeLiteralColors`](https://biomejs.dev/linter/rules/no-react-native-literal-colors/), which disallows color literals inside React Native styles. The rule belongs to the `reactNative` domain. It reports properties whose name contains `color` and whose value is a string literal when they appear inside a `StyleSheet.create(...)` call or inside a JSX attribute whose name contains `style`. ```jsx // Invalid const Hello = () => <Text style={{ backgroundColor: "#FFFFFF" }}>hi</Text>; const styles = StyleSheet.create({ text: { color: "red" }, }); ``` ```jsx // Valid const red = "#f00"; const styles = StyleSheet.create({ text: { color: red }, }); ``` - [#​10005](biomejs/biome#10005) [`131019e`](biomejs/biome@131019e) Thanks [@​ematipico](https://github.com/ematipico)! - Added the nursery rule [`noReactNativeRawText`](https://biomejs.dev/linter/rules/no-react-native-raw-text/), which disallows raw text outside of `<Text>` components in React Native. The rule belongs to the new `reactNative` domain. ```jsx // Invalid <View>some text</View> <View>{'some text'}</View> ``` ```jsx // Valid <View> <Text>some text</Text> </View> ``` Additional components can be allowlisted through the `skip` option: ```json { "options": { "skip": ["Title"] } } ``` - [#​9911](biomejs/biome#9911) [`1603f78`](biomejs/biome@1603f78) Thanks [@​Netail](https://github.com/Netail)! - Added the nursery rule [`noJsxLeakedDollar`](https://biomejs.dev/linter/rules/no-jsx-leaked-dollar), which flags text nodes with a trailing `$` if the next sibling node is a JSX expression. This could be an unintentional mistake, resulting in a '$' being rendered as text in the output. **Invalid**: ```jsx function MyComponent({ user }) { return <div>Hello ${user.name}</div>; } ``` - [#​9999](biomejs/biome#9999) [`f42405f`](biomejs/biome@f42405f) Thanks [@​minseong0324](https://github.com/minseong0324)! - Fixed `noMisleadingReturnType` incorrectly flagging functions with reassigned `let` variables. - [#​10075](biomejs/biome#10075) [`295f97f`](biomejs/biome@295f97f) Thanks [@​ematipico](https://github.com/ematipico)! - Fixed [`#9983`](biomejs/biome#9983): Biome now parses functions declared inside Svelte `#snippet` blocks without throwing errors. - [#​10006](biomejs/biome#10006) [`cf4c1c9`](biomejs/biome@cf4c1c9) Thanks [@​minseong0324](https://github.com/minseong0324)! - Fixed [#​9810](biomejs/biome#9810): `noMisleadingReturnType` incorrectly flagging nested object literals with widened properties. - [#​10033](biomejs/biome#10033) [`11ddc05`](biomejs/biome@11ddc05) Thanks [@​ematipico](https://github.com/ematipico)! - Added the nursery rule [`useReactNativePlatformComponents`](https://biomejs.dev/linter/rules/use-react-native-platform-components/) that ensures platform-specific React Native components (e.g. `ProgressBarAndroid`, `ActivityIndicatorIOS`) are only imported in files with a matching platform suffix. It also reports when Android and iOS components are mixed in the same file. The following code triggers the rule when the file does not have an `.android.js` suffix: ```js // file.js import { ProgressBarAndroid } from "react-native"; ``` </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMzkuNiIsInVwZGF0ZWRJblZlciI6IjQzLjEzOS42IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Reviewed-on: https://git.oirnoir.dev/OIRNOIR/YouTube-Helper-Server/pulls/1
I used Claude Code to assist with this implementation.
Summary
Addresses items from #9810: Nested object widening
Test Plan
Snapshot tests.