Rule proposal: `prefer-immediate-return`

[remcohaszing]

In my opinion it’s never desirable to assign a value to a variable, only to be returned immediately. I believe this is often the leftover result of debugging.

In other words, if a value is declared, and returned immediately in the next statement, the two should be merged, removing the useless variable.

Fail

// A
function foo() {
  const result = bar();
  return result;
}

// B
function foo() {
  let result;
  return result;
}

Pass

function foo() {
  const result = bar();
  // Anything in between is allowed.
  console.log(result);
  return result;
}

function foo() {
  let result;
  result = bar();
  return result;
}

// Autofix result for A
function foo() {
  return bar();
}

// Autofix result for B
function foo() {
  return;
}

[fisker]

There is an existing rule sonarjs/prefer-immediate-return, but I'm not a fan, I disabled it recently.

[remcohaszing]

You’re right, it even has the same name as I came up with. I can’t believe I didn’t find this earlier. :sweat_smile:

I don’t know if eslint-plugin-unicorn wants to duplicate rules from other plugins, so I’ll just leave this issue open for now.

[fregante]

@fisker Why not a fan? It seems reasonable. When would that code be preferable?

[fisker]

I forgot where the code is, but this is similar to "anonymous default export", sometimes I want to give a name to the return value.

[slikts]

This rule conflicts with returning named functions, which is useful for debuggability:

const makeFoo = () => {
  const foo = () => {
    // something
  }
  return foo
}

[fregante]

I forgot where the code is, but this is similar to "anonymous default export"

You can use use return function foo() {} just like with export default function foo() {}

sometimes I want to give a name to the return value.

What's the purpose of this? The name of non-functions variables is lost after return (i.e. it's not available outside the function), so this would only be useful on those exact 2 lines:

const life = 42;
return life;

returning named functions, which is useful for debuggability

I think that's already covered (i.e. forbidden) by func-names, example:

export function a() {
	return function foo() {
		return 1
	}
}

✖  220:9   Unexpected named function foo.               func-names

If you want to name functions, you can disable func-names instead (which has been enabled for years in XO, for example)

---

I think this rule would conflict exclusively if you intend to use named arrow functions, which… seems picky since regular functions can be named and can be returned immediately.

[remcohaszing]

This rule would also play nice with no-return-await. Currently an intermediate variable could be used to circumvent this rule.

async function foo() {
  const value = await bar();
  return value;
}

Becomes (because of prefer-immediate-return):

async function foo() {
  return await bar();
}

Which then becomes (because of no-return-await):

async function foo() {
  return bar();
}

[slikts]

@fregante I'm using arrow functions for consistency and clarity and don't want to have to switch to named function expressions. I've made a request to support this use case at SonarSource/eslint-plugin-sonarjs#293.

Edit: it's actually more of a bug report than a feature request because a linter autofixing something that changes semantics is a way to introduce hard to spot bugs in someone's code, since it means the .name property for functions is different.

[remcohaszing]

There could be an option allowArrowFunctions.

[fregante]

I'm using arrow functions for consistency and clarity

As I said, that seems picky. Nothing clear about

const makeFoo = () => {
  const foo = () => {
    // something
  }
  return foo
}

over

const makeFoo = () => {
  return function foo() {
    // something
  }
}

Or even

function makeFoo() {
  return function foo() {
    // something
  }
}

If anything, it's the opposite: It's more compact and has less = => noise and foo duplication.

since it means the .name property for functions is different.

If you're using a minifier those names are being mangled to begin with.

[slikts]

Using both arrow functions and function expressions is inconsistent, because, if primarily using arrow functions, immediately returning a named function would be an exception. Function declarations also have undesirable behaviors like foo(); function foo() {} being possible. Arrow functions also communicate clearly that this is not used, and arrow function syntax is more terse. Spec compliant transpilers will support function name inference, although it's for debugging anyway.

[fregante]

You're arguing that arrow functions are better than regular functions, while ignoring the issue at hand.

immediately returning a named function would be an exception

Here you are returning a named function, that's why you posted a comment, because you want to name functions. Returning function name(){} seems like the obvious choice, that's the whole point of it.

Function declarations also have undesirable behaviors like foo(); function foo() {} being possible.

Completely not applicable here. return function foo (){} is not a declaration and as such it's not available before returning it.

Arrow functions also communicate clearly that this is not used

Arrow functions are perfectly capable of using this, it's just that they're pre-bound. If your intent is to not use this, then it doesn't matter either way.

arrow function syntax is more terse

No it's not in this case, I just showed you:

const foo = () => {
  // something
}
return foo

vs

return function foo () {
  // something
}

[slikts]

This is an example of the issues with hoisting, in that they cause confusion. const is also hoisted, just not initialized. You can do const foo = () => bar(); const bar = () => {}; foo() just fine. With function declarations, however, the function definition is also hoisted (not just the declaration), making them more of a special case and so making their hoisting behavior more confusing for no benefit.

Function declarations are also more loose compared to const because you can re-declare them, so it's an another source of mistakes.

The arrow function syntax is more terse for anonymous functions and/or when return is omitted.

By "not using this" I obviously mean that this can only be bound lexically in arrow functions and can't be rebound dynamically. Using arrow functions makes the intentions clear, whereas sometimes using functions expressions instead of arrow would require anyone reading the code to consider if the context isn't rebound.

In any case, the use case I mentioned (consistent use of arrow functions and returning named functions) is valid and your stylistic preferences of mixing types of function definitions are not really relevant.

[fregante]

I updated my comment 👆

You're arguing that arrow functions are better than regular functions, while ignoring the issue at hand.

[slikts]

I saw the updates before replying, and the last line addresses them: your personal stylistic preferences don't make my use case (consistently using arrow functions) invalid.

[fregante]

You're wrong and confused. You're the one arguing from a stylistic standpoint.

consistent use of arrow functions and returning named functions

arrow function syntax is more terse

These are stylistic preferences and you said them.

Function declarations are also more loose compared to const because you can re-declare them, so it's an another source of mistakes.

Invalid. return function foo() {} is not a declaration.

This is an example of the issues with hoisting, in that they cause confusion

Invalid. There's no hoisting in return function foo () {}, try it.

The arrow function syntax is more terse for anonymous functions

Invalid. You're here to ask for functions to be named so they're not anonymous.*

[slikts]

I'd suggest trying to practice charitable interpretation, since I'm referencing arrow functions and function declarations and expressions separately and in the correct contexts, so I know the difference. Declarations are relevant as part of the general reason to prefer consistent function definitions using only the arrow syntax, since declarations are one of the two alternatives (ignoring the Function constructor). That's a preference with valid reasons and you not seeing an issue with hoisting, etc. doesn't override those reasons, and function expressions are not relevant since that'd mean giving up consistent function definitions.

[AndreaPontrandolfo]

Agree with @fisker , it's one of the few sonarjs rules that i always leave off. It's one of those rules that seems good in theory but then do more harm then good in practice. Having that variable helps with readability, discovery/searchability and debugging.