Rule proposal: disallow unused return types #6442

KhafraDev · 2023-02-08T22:50:06Z

Before You File a Proposal Please Confirm You Have Done The Following...

I have searched for related issues and found none that match my proposal.
I have searched the current rule list and found no rules that match my proposal.
I have read the FAQ and my problem is not listed.

My proposal is suitable for this project

My proposal specifically checks TypeScript syntax, or it proposes a check that requires type information to be accurate.
My proposal is not a "formatting rule"; meaning it does not just enforce how code is formatted (whitespace, brace placement, etc).
I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Description

Paired with explicit-function-return-types, it's possible to include extraneous return types that are never used. This can happen when refactoring code meaning that some code branches might become impossible to reach.

For example you might have this function:

function random (param?: string): string | null {
  if (typeof param !== 'string') return null
  
  return 'has param'
}

where the return types are correct, but then it's refactored to

// note that param is no longer optional but the return type is the same union
function random (param: string): string | null {
  return 'has param'
}

and let's assume it's being used in a codebase before param is required:

const rand = random(param)

if (rand === null) {
  throw new Error('...')
}

since the return type is unchanged, the null check branch might not be removed during refactoring, and neither the linter or tsc will complain about it.

Fail Cases

function test (): string | null {
  return 'string'
}

class A {
  static test (): string | null {
    return 'string'
  }

  test (): string | null {
    return 'string'
  }
}

Pass Cases

function test (): string {
  return 'string'
}

class A {
  static test (): string | null {
    if (Math.random() < 0.5) return null

    return 'string'
  }

  test (): string {
    return 'string'
  }
}

Additional Info

It's totally possible I missed an issue that matched this proposal, but I couldn't find any.

JoshuaKGoldberg · 2023-02-08T22:54:51Z

Haha, I was wondering if this would come up soon. Explicit vs. inferred return types has been the rage on TypeScript Twitter for a week or so now.

Thanks for filing this issue - I'd been meaning to try to find or file one and hadn't gotten around to it yet. 😄

https://twitter.com/JoshuaKGoldberg/status/1622971899549720576:

FAQ: Why doesn't @tseslint have a lint rule to block explicit return types that don't add any info?

A: We'd love to, but we're blocked on microsoft/TypeScript#9879. There's no way for us to compare the type annotation to all returns in the function. One day maybe 🙏

JoshuaKGoldberg · 2024-08-01T19:36:06Z

Now that v8 is released, per #7936, we can mark this issue as unblocked! 🚀

ronami · 2024-10-25T08:21:15Z

This rule sounds interesting! Some questions/thoughts regarding this:

Should the rule handle async functions? Should the following example be reported? My understanding is that it should:

async function test(): Promise<string | null> {
  return 'hello';
}

If it should, should the following also be flagged? I think so:

function test(): Promise<string | null> {
  return Promise.resolve('hello');
}

Should arrays (potentially others) also be reported?

function test(): Array<string | null> {
  return ['hello', 'world'];
}

On a different topic, I think the rule may create some unwanted noise in some cases. Consider the following hypothetical example:

// used throughout the project
export type Plugin = { kind: 'one' } | { kind: 'two' };

function createSomePlugin(): Plugin {
  return {
    kind: 'one',
  };
}

The function above violates the rule; however, it uses a referenced union type rather than inlining the union type on its return type annotation. Fixing the example above is not as trivial as fixing the simpler cases.

In my opinion, cases like the one above shouldn't be flagged. I think the rule should only report a union that's declared as part of the function return type (inline), but not those on referenced types:

// should report 🚫
function test(): string | null {
  return 'hello'
}

// shouldn't report ✅
type R = string | null;

function test(): R {
  return 'hello'
}

// should report 🚫
type R = string;
type T = number;

function test(): R | T {
  return 'hello'
}

Does this make sense? I'll be happy to hear opinions on this.

kirkwaiblinger · 2024-10-27T17:57:42Z

My thoughts

Should the rule handle async functions? Should the following example be reported? My understanding is that it should:
async function test(): Promise<string | null> {
  return 'hello';
}

Absolutely! What we've done before, though, is said specifically async functions, not just Promise-returning functions, should get this special treatment. See #8674

If it should, should the following also be flagged? I think so:
function test(): Promise<string | null> {
  return Promise.resolve('hello');
}

Yep! checker.getAwaitedType() will be your friend here, refer to #8693

Should arrays (potentially others) also be reported?
function test(): Array<string | null> {
  return ['hello', 'world'];
}

I think so! Special treatment makes sense here if possible.

On a different topic, I think the rule may create some unwanted noise in some cases. Consider the following hypothetical example:
// used throughout the project
export type Plugin = { kind: 'one' } | { kind: 'two' };

function createSomePlugin(): Plugin {
  return {
    kind: 'one',
  };
}
The function above violates the rule; however, it uses a referenced union type rather than inlining the union type on its return type annotation. Fixing the example above is not as trivial as fixing the simpler cases.

In my opinion, cases like the one above shouldn't be flagged. I think the rule should only report a union that's declared as part of the function return type (inline), but not those on referenced types:
// should report 🚫
function test(): string | null {
  return 'hello'
}

// shouldn't report ✅
type R = string | null;

function test(): R {
  return 'hello'
}

// should report 🚫
type R = string;
type T = number;

function test(): R | T {
  return 'hello'
}
Does this make sense? I'll be happy to hear opinions on this.

Yeah, I generally agree that taking this heuristic-based approach makes sense. I think it makes sense in my head as well to inspect explicitly provided unions, and descend only into special generics, like Promise<T | U>, Array<T | U> (maybe also Set<T | U>, Map), at least as a first pass. Though, idk, I guess someone could make a case that we should collect all return statements, take their union, and if that's not equal to the return type (i.e. mutually assignable) we report 🤔. But, agreed that in my head it makes more sense to start with the heuristic-based approach.

The one thing that maybe we could consider is unpacking nullability.... For example, the following

type NullableString = string | null | undefined;
function foo(): NullableString {
    return "foo";
}

could be detected and fixed to the following?

function foo(): NonNullable<NullableString> {
    return "foo";
}

Another note is that I think we probably want to consider literals as satisfying their infinite type (which is sort of a corollary of the previous point)

// I think this is ok, even though the return type _could_ just be `"foo" | "bar"`.
function foo() : string {
    return Math.random() > 0.5 ? 'foo' : 'bar';
}

But,

// this is not ok
function foo(): "foo" | "bar" {
    return "foo";
}

kirkwaiblinger · 2024-10-31T18:57:33Z

Another syntax we could consider inspecting (if possible) is the fields within literal object types

function foo(): { a: string | number } {
   return { a: 'no numbers here' };
}

KhafraDev added enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Feb 8, 2023

JoshuaKGoldberg added blocked by external API Blocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API and removed triage Waiting for team members to take a look labels Feb 8, 2023

JoshuaKGoldberg mentioned this issue Oct 11, 2023

Proposal: Type Relationship API microsoft/TypeScript#9879

Open

JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed blocked by external API Blocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API labels Aug 1, 2024

This was referenced Dec 6, 2024

feat(eslint-plugin): add new rule no-extraneous-return-types #10464

Closed

feat(eslint-plugin): new rule no-unused-destructure-type-properties #10344

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rule proposal: disallow unused return types #6442

Rule proposal: disallow unused return types #6442

Rule proposal: disallow unused return types #6442

Rule proposal: disallow unused return types #6442

Comments

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

Description

Fail Cases

Pass Cases

Additional Info