Skip to content

feat(lint): add nursery rule noMisleadingReturnType#9799

Merged
dyc3 merged 27 commits intobiomejs:mainfrom
minseong0324:feat/no-misleading-return-type
Apr 6, 2026
Merged

feat(lint): add nursery rule noMisleadingReturnType#9799
dyc3 merged 27 commits intobiomejs:mainfrom
minseong0324:feat/no-misleading-return-type

Conversation

@minseong0324
Copy link
Copy Markdown
Contributor

@minseong0324 minseong0324 commented Apr 5, 2026

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.

function getStatus(b: boolean): string {
  if (b) return "loading";
  return "idle";
}

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

  • 17 invalid cases produce diagnostics.
  • 33 valid cases produce zero diagnostics.
  • cargo test -p biome_js_analyze -- no_misleading_return_type passes.
  • just l passes with zero warnings.

Docs

Rule documentation is inline via rustdoc in declare_lint_rule!, with Invalid and Valid examples.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 5, 2026

🦋 Changeset detected

Latest commit: 849fa8a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages
Name Type
@biomejs/biome Patch
@biomejs/cli-win32-x64 Patch
@biomejs/cli-win32-arm64 Patch
@biomejs/cli-darwin-x64 Patch
@biomejs/cli-darwin-arm64 Patch
@biomejs/cli-linux-x64 Patch
@biomejs/cli-linux-arm64 Patch
@biomejs/cli-linux-x64-musl Patch
@biomejs/cli-linux-arm64-musl Patch
@biomejs/wasm-web Patch
@biomejs/wasm-bundler Patch
@biomejs/wasm-nodejs Patch
@biomejs/backend-jsonrpc Patch

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 github-actions Bot added A-Project Area: project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis labels Apr 5, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 5, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds a new TypeScript lint rule noMisleadingReturnType that detects when a function’s declared return type is wider than its actual returned values. The rule skips generators and overload implementations, unwraps async Promise annotations, collects inferred return-site types (respecting as const and nested scopes), applies heuristics to avoid false positives, performs depth-limited structural comparisons, and emits diagnostics with an optional inferred-literal note. Also adds valid/invalid TypeScript test fixtures, registers a new no_misleading_return_type rule options module, and introduces an empty NoMisleadingReturnTypeOptions struct.

Suggested reviewers

  • ematipico
  • dyc3
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: adding a new nursery linter rule named noMisleadingReturnType.
Description check ✅ Passed The description explains the rule's purpose, provides a concrete example, and documents the test coverage and implementation approach.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1d09f0f and 05390cd.

⛔ Files ignored due to path filters (6)
  • crates/biome_configuration/src/analyzer/linter/rules.rs is excluded by !**/rules.rs and included by **
  • crates/biome_configuration/src/generated/domain_selector.rs is excluded by !**/generated/**, !**/generated/** and included by **
  • crates/biome_configuration/src/generated/linter_options_check.rs is excluded by !**/generated/**, !**/generated/** and included by **
  • crates/biome_diagnostics_categories/src/categories.rs is excluded by !**/categories.rs and included by **
  • crates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/invalid.ts.snap is excluded by !**/*.snap and included by **
  • crates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/valid.ts.snap is excluded by !**/*.snap and included by **
📒 Files selected for processing (6)
  • .changeset/dark-new-old.md
  • crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs
  • crates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/invalid.ts
  • crates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/valid.ts
  • crates/biome_rule_options/src/lib.rs
  • crates/biome_rule_options/src/no_misleading_return_type.rs

Comment on lines +108 to +111
if includes_undefined(&effective_return_ty)
&& !returns.iter().any(includes_undefined)
{
return None;
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cat -n crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs | head -150

Repository: biomejs/biome

Length of output: 5798


🏁 Script executed:

sed -n '150,250p' crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs

Repository: biomejs/biome

Length of output: 3060


🏁 Script executed:

sed -n '250,400p' crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs

Repository: biomejs/biome

Length of output: 5224


🏁 Script executed:

sed -n '400,550p' crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs

Repository: biomejs/biome

Length of output: 4710


🏁 Script executed:

sed -n '550,650p' crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs

Repository: biomejs/biome

Length of output: 3504


🏁 Script executed:

sed -n '434,480p' crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs

Repository: biomejs/biome

Length of output: 1340


🏁 Script executed:

sed -n '470,495p' crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs

Repository: 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.

✅ Addressed in commits 1695115 to 14e3a65

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intentional conservative bail, will refine in follow-up.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@minseong0324

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_number to the rule definition with the issue number you just opened

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ematipico
Opened #9810 to track remaining work, and added issue_number in 4961a53.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🙏

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I thought it put something in the service bag. I'm fine if that's resolved in another PR.

Comment thread crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs Outdated
Copy link
Copy Markdown
Contributor

@dyc3 dyc3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 and Strings being allocated. TokenText can 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.

Comment thread crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs Outdated
Comment thread crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs Outdated
Comment thread crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs Outdated
Comment thread crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs Outdated
Comment thread crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs Outdated
Comment thread crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs Outdated
Comment thread crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs Outdated
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 5, 2026

Merging this PR will not alter performance

✅ 58 untouched benchmarks
⏩ 196 skipped benchmarks1


Comparing minseong0324:feat/no-misleading-return-type (849fa8a) with main (a2f3f7e)

Open in CodSpeed

Footnotes

  1. 196 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@minseong0324 minseong0324 force-pushed the feat/no-misleading-return-type branch from 073d32b to 5eec5e6 Compare April 5, 2026 15:53
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 073d32b and 5eec5e6.

⛔ Files ignored due to path filters (1)
  • crates/biome_js_analyze/tests/specs/nursery/noMisleadingReturnType/invalid.ts.snap is excluded by !**/*.snap and included by **
📒 Files selected for processing (1)
  • crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs

Comment thread crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs Outdated
Comment thread crates/biome_js_analyze/src/lint/nursery/no_misleading_return_type.rs Outdated
@minseong0324 minseong0324 force-pushed the feat/no-misleading-return-type branch 4 times, most recently from eb95ed7 to c67a440 Compare April 5, 2026 16:29
@minseong0324 minseong0324 force-pushed the feat/no-misleading-return-type branch from e2e4e9d to fc7df88 Compare April 6, 2026 16:17
Copy link
Copy Markdown
Contributor

@dyc3 dyc3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty happy with how this is shaping up, even though its quite complex. just a few more notes.

Comment on lines +160 to +165
let diag = match build_inferred_description(&state.returns) {
Some(desc) => diag.note(markup! {
"The inferred return type is narrower: "{desc}"."
}),
None => diag,
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b79277a.

Comment on lines +438 to +444
if result.contains("...") || result.contains("__internal") || result.contains("typeof import(") {
return None;
}

if result.len() > 80 {
return None;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets document why these conditions exist

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 4b52422.

@minseong0324
Copy link
Copy Markdown
Contributor Author

@dyc3

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?

@dyc3
Copy link
Copy Markdown
Contributor

dyc3 commented Apr 6, 2026

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.

@minseong0324 minseong0324 requested a review from dyc3 April 6, 2026 17:28
@minseong0324

This comment was marked as duplicate.

@dyc3
Copy link
Copy Markdown
Contributor

dyc3 commented Apr 6, 2026

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Project Area: project L-JavaScript Language: JavaScript and super languages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants