feat(lint): add nursery rule noMisleadingReturnType#9799
Conversation
🦋 Changeset detectedLatest commit: 849fa8a 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 |
|
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 a new TypeScript lint rule 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: 3
🤖 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/src/lint/nursery/no_misleading_return_type.rs`:
- Around line 401-405: The bug is that build_inferred_description uses
annotation.flattened_union_variants() for union annotations instead of the
actual returned types; change the union branch to derive candidates from the
returns slice (e.g., collect flattened variants from each element of returns) so
the diagnostic reflects the inferred return types (modify the TypeData::Union(_)
arm to use returns.iter().flat_map(|r| r.flattened_union_variants()).collect()
or otherwise collect from returns, keeping the function
build_inferred_description, the returns parameter, and
annotation.flattened_union_variants() as reference points).
- Around line 108-111: The current early return suppresses diagnostics whenever
the annotated return type (effective_return_ty) includes undefined/void but none
of the collected explicit returns (returns from collect_block_returns) do;
instead, change the logic to look for bare returns and fall-through paths
explicitly: keep the diagnostic unless at least one collected return lacks an
expression (a bare `return`) or the function/block can fall through to an
implicit undefined (detectable by checking the block's final statement/exit
path), and only then suppress by returning None. Update the code that currently
references includes_undefined(&effective_return_ty) &&
!returns.iter().any(includes_undefined) to (1) inspect the collected returns for
any return node with no argument (bare return) and (2) check the block for a
possible fall-through/implicit return; if neither condition is true, do not
return None so the diagnostic is emitted.
- Around line 510-556: resolve_identifier_initializer_type and
identifier_refers_to_const_assertion currently match declarations by name across
the entire function body without regard to declaration order or lexical scope;
update both to perform scope- and order-aware binding resolution: start from the
usage node (id_expr / reference) and search for a matching binding only in
lexical scopes that enclose the usage (walk parent ancestors to find the nearest
enclosing block/function), and within each scope scan only preceding sibling
statements (and their nested declarations) — not later statements or
declarations inside nested inner blocks that do not lexically enclose the usage;
prefer using existing AST/symbol utilities if available, otherwise implement a
walker that checks ancestor scopes and prior siblings when looking for
JsVariableDeclarator/JsVariableStatement with the same identifier and a prior
source position before returning the initializer type in
resolve_identifier_initializer_type (and mirror the same ordering/scope rules in
identifier_refers_to_const_assertion).
🪄 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: f66d00b8-7d80-41b3-8585-8e4100f75453
⛔ Files ignored due to path filters (6)
crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/domain_selector.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_configuration/src/generated/linter_options_check.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/valid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (6)
.changeset/dark-new-old.mdcrates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rscrates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/invalid.tscrates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/valid.tscrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/no_misleading_return_type.rs
| if includes_undefined(&effective_return_ty) | ||
| && !returns.iter().any(includes_undefined) | ||
| { | ||
| return None; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs | head -150Repository: biomejs/biome
Length of output: 5798
🏁 Script executed:
sed -n '150,250p' crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rsRepository: biomejs/biome
Length of output: 3060
🏁 Script executed:
sed -n '250,400p' crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rsRepository: biomejs/biome
Length of output: 5224
🏁 Script executed:
sed -n '400,550p' crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rsRepository: biomejs/biome
Length of output: 4710
🏁 Script executed:
sed -n '550,650p' crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rsRepository: biomejs/biome
Length of output: 3504
🏁 Script executed:
sed -n '434,480p' crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rsRepository: biomejs/biome
Length of output: 1340
🏁 Script executed:
sed -n '470,495p' crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rsRepository: biomejs/biome
Length of output: 836
Account for bare return and fall-through paths explicitly, not via blanket exemption.
The check at lines 108–111 suppresses valid diagnostics for exhaustive functions like function f(b): string | undefined { if (b) return "a"; return "b"; }. Since collect_block_returns only records returns with explicit arguments, a bare return won't appear in the collected list. Rather than exempting all cases where the annotation includes undefined/void and returns don't, detect and handle bare returns and fall-through paths directly.
🤖 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 108 - 111, The current early return suppresses diagnostics whenever the
annotated return type (effective_return_ty) includes undefined/void but none of
the collected explicit returns (returns from collect_block_returns) do; instead,
change the logic to look for bare returns and fall-through paths explicitly:
keep the diagnostic unless at least one collected return lacks an expression (a
bare `return`) or the function/block can fall through to an implicit undefined
(detectable by checking the block's final statement/exit path), and only then
suppress by returning None. Update the code that currently references
includes_undefined(&effective_return_ty) &&
!returns.iter().any(includes_undefined) to (1) inspect the collected returns for
any return node with no argument (bare return) and (2) check the block for a
possible fall-through/implicit return; if neither condition is true, do not
return None so the diagnostic is emitted.
There was a problem hiding this comment.
intentional conservative bail, will refine in follow-up.
There was a problem hiding this comment.
This implies that you have some sort of plan, and that this PR doesn't implement all of it.
In these cases, the approach is the following:
- opening an issue to track the works
- add
issue_numberto the rule definition with the issue number you just opened
There was a problem hiding this comment.
@ematipico
Opened #9810 to track remaining work, and added issue_number in 4961a53.
There was a problem hiding this comment.
I took another look at this. Full CFA isn't an option since Typed and ControlFlowGraph can't be used as a query type together, but I think there's a reasonable middle ground - track bare return; statements, and check if the body's last statement is a return with an argument. If neither bare return nor fall-through is possible, we skip the bail.
Handles the case you flagged (function f(b): string | undefined { if (b) return "a"; return "b"; }), still bails for things like function f(flag): string | void { if (flag) return "yes"; } where the last statement isn't a return.
Not a complete fix, but should cover the common case. Let me know what you think. 🙏
There was a problem hiding this comment.
You can still grab the ControlFlowGraph service and it would be safe to do because all services are built before any rules that use services execute.
There was a problem hiding this comment.
Tried ctx.get_service::<JsControlFlowGraph>(), returns None. The CFG visitor emits via
match_query but never persists anything in the ServiceBag. Would it need a keyed store like EarlyReturnsModel to look up per-function CFGs?
There was a problem hiding this comment.
Ah, I thought it put something in the service bag. I'm fine if that's resolved in another PR.
dyc3
left a comment
There was a problem hiding this comment.
A decent first draft, but we can make some improvements.
- add doc comments to your helper functions, and comments inside where you think it's appropriate. the flow of the rule is a bit hard to follow.
- We need to be mindful of heap allocations. there's a lot of
Vecs andStrings being allocated.TokenTextcan sometimes be used to get an owned reference to a string. I've only marked a couple examples of this in my review because there is a lot of code here that does this.
Merging this PR will not alter performance
Comparing Footnotes
|
073d32b to
5eec5e6
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_analyze/src/lint/nursery/no_misleading_return_type.rs`:
- Around line 689-730: The match in is_wider_than currently ignores cases where
the inferred type is a Union or a Tuple, causing missed detections (e.g., string
vs "a" | "b" or [string, number] vs ["x",1] as const); add arms for (_,
TypeData::Union(inf_union)) and (_, TypeData::Tuple(inf_tuple)) that evaluate
whether every variant/element of the inferred union/tuple is narrower than the
annotated type by reusing/adding helper logic (e.g., call or add functions like
is_union_wider_when_inferred or is_tuple_wider to iterate variants/elements and
call is_wider_than/appropriate narrower checks with increased depth), and ensure
you handle literal-variant unions (Literal::String/Number/Boolean/BigInt/Object)
and const tuples by comparing each literal element to the annotated element
type.
🪄 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: 629428d9-62c5-428b-a229-d36379c9d07c
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/invalid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (1)
crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs
eb95ed7 to
c67a440
Compare
…leadingReturnType
…s in noMisleadingReturnType
c67a440 to
fee659f
Compare
e2e4e9d to
fc7df88
Compare
dyc3
left a comment
There was a problem hiding this comment.
I'm pretty happy with how this is shaping up, even though its quite complex. just a few more notes.
| let diag = match build_inferred_description(&state.returns) { | ||
| Some(desc) => diag.note(markup! { | ||
| "The inferred return type is narrower: "{desc}"." | ||
| }), | ||
| None => diag, | ||
| }; |
There was a problem hiding this comment.
Currently, the diagnostic does not provide anything actionable to the user. This (or another note) needs to tell the user how to resolve the error.
| if result.contains("...") || result.contains("__internal") || result.contains("typeof import(") { | ||
| return None; | ||
| } | ||
|
|
||
| if result.len() > 80 { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
lets document why these conditions exist
|
I noticed tuple element widening and ternary-inferred unions aren't detected yet. function f(): [string, number] { return ["hello", 42] as const; }
function g(b: boolean): string { return b ? "a" : "b"; }Should I add them here or keep for a follow-up? |
|
I would say add it to the tracking issue and do it in another pr. More code here is going to make this take longer to merge. |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
FYI, don't merge main into this branch anymore unless there are merge conflicts. It invalidates the CI and we'll have to manually approve the run again. |
I used Claude Code to assist with exploring the type infra and iterating on the implementation.
Summary
Added the rule noMisleadingReturnType. Resolves #9789.
The rule detects when a function's return type annotation is wider than what the implementation actually returns.
Biome now reports when the annotation (
: string) is wider than the inferred return type ("loading" | "idle"), helping callers rely on precise types.The rule bails conservatively on unresolved types and does not provide an autofix.
Test Plan
cargo test -p biome_js_analyze -- no_misleading_return_typepasses.just lpasses with zero warnings.Docs
Rule documentation is inline via rustdoc in
declare_lint_rule!, withInvalidandValidexamples.