Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow ExpressionStatement and SequenceExpression. #4

Open
dtinth opened this issue Mar 11, 2016 · 5 comments
Open

Disallow ExpressionStatement and SequenceExpression. #4

dtinth opened this issue Mar 11, 2016 · 5 comments

Comments

@dtinth
Copy link

dtinth commented Mar 11, 2016

Alternate title: Disallow function calls whose return values are discarded.

Based on this blog post, when you call a function and don’t use it’s return value, chances are high that it is being called for its side effect. e.g.

array.push(1)
alert('Hello world!')

Thinking it further, I found this to be an expansion of no-unused-expressions rule, but with every ExpressionStatement disallowed, even if it only contains a CallExpression.

Disallowing SequenceExpression may also make sense because there’s no point in evaluating a, b in (a, b, c) except for the side-effects caused by evaluating a and b.

I think this can also potentially fix #2.

As a result, every expression that gets evaluated must either go into a constant or returned. If we also ensure that there are no-unused-vars, we can get a pure-functional-language-like semantic.

@jhusain
Copy link
Owner

jhusain commented Mar 12, 2016

I agree this is a worthwhile addition to the rules.

@scottnonnenberg
Copy link
Contributor

I really like this idea for pushing code to functional style. Off the top of my head, we'd need a few key exceptions to make it reasonable:

  • console.log() or any log()-style method call
  • anything with side effects, like process.exit() or any library from fs
  • any kickoff of an async task which calls a provided or closed-upon callback (though we could force a return statement)

I might just have to implement it, start using it, then see what exceptions I'd want to put in place...

@dtinth
Copy link
Author

dtinth commented May 29, 2016

@scottnonnenberg

I agree that there are certain exceptions to this rule, but the whole point of pushing code to functional style is to eliminate side-effects. 😄

In my opinion, the only exception would be using invariant to make sure that the function is properly called, although it might be better enforced using babel-plugin-typecheck:

export const add = (a, b) => (
  invariant(typeof a === 'number', 'a must be a number'),
  invariant(typeof b === 'number', 'b must be a number'),
  a + b
)

But for a program to function, it needs side effects, right?

I’d put all the imperative, side-effecting code outside the core of the application, where this rule is disabled. While the core of my application (with main logic) would all be functional and without side-effects. This idea is called “Functional Core, Imperative Shell.”.

In fact, Haskell enforces this idea at a type level. All side effecting function has type IO<something>. For example, a function that returns an IO<String> takes some side-causes, produces some side-effects, and returns a String. Normally, IO functions can call normal functions, but not the other way around. This helps me make sure that all of my core logic is side-effects free.

For debugging via console, you can create a trace function. (This file must have the rule turned off).

/* eslint-disable no-impure-code */
export function trace (...args) {
  return (value) => {
    console.log(...args, value)
    return value
  }
}

Then you can add trace anywhere to log a value. The code below looks purely functional, but it produces some console.log as side-effects.

export const add = (a, b) => (
  trace('a =')(a) + trace('b =', b)
)

This breaks the purity rule, but when you’re debugging, anything goes. Haskell also contains the same trace function under Debug.Trace.

@scottnonnenberg
Copy link
Contributor

As long as the documentation recommends something like installing it for only part of your application, then it could be reasonable with no exceptions. It's not a standard use of eslint that I've seen, though I'm finding real benefits in having different configs for my src/, test/ and scripts/ directories.

Also, where's the PR? :0)

@dtinth
Copy link
Author

dtinth commented May 29, 2016

No one implemented it yet I guess…

So I just pushed it as a separate plugin in order to experiment more quickly:
purely-functional/eslint-plugin-pure.

I’m going to test this rule, along with other rules from this repo, with some of my personal projects and work projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants