ESLint rules that analyze `return` are broken.

[Darkle]

With the following code I get an error with the fp/no-nil plugin. It says that the map function is returning undefined.

videos.map(video -> 
  feed.item({
    title: video.title,
    guid: video.id,
    url: video.link, 
    date: video.pubDate,
  })
)

I'm not 100% certain this is a lightscript issue though, as I have only just started using the eslint-plugin-fp plugin.

This is what I have installed:

  "devDependencies": {
    "@babel/cli": "^7.1.0",
    "@babel/core": "^7.1.0",
    "@babel/plugin-external-helpers": "^7.0.0",
    "@babel/preset-env": "^7.1.0",
    "@lightscript/babel-preset": "^4.0.0-alpha.12",
    "@lightscript/eslint-plugin": "^4.0.0-alpha.12",
    "cross-env": "^5.2.0",
    "eslint": "^5.6.0",
    "eslint-plugin-fp": "^2.3.0",
    "npm-run-all": "^4.1.3",
    "rollup": "^0.66.2",
    "rollup-plugin-babel": "^4.0.3",
    "rollup-plugin-node-resolve": "^3.4.0",
    "rollup-plugin-notify": "^1.0.6",
    "rollup-plugin-replace": "^2.0.0"
  },
  "dependencies": {
    "@lightscript/runtime": "^0.1.1",
    "express": "^4.16.3",
    "folktale": "^2.3.0",
    "lowdb": "^1.0.0",
    "rss": "^1.2.2",
    "rss-parser": "^3.4.3",
    "timeproxy": "^1.2.1"
  }

My eslint config file:

{
  "parser": "@lightscript/eslint-plugin",
  "parserOptions": {
    "sourceType": "module"
  },
  "extends": [
    "eslint:recommended",
    "plugin:@lightscript/recommended"
  ],
  "globals": {
    "ISDEV": true
  },
  "env": {
    "node": true,
    "es6": true
  },
  "plugins": [
    "@lightscript/eslint-plugin",
    "fp"
  ],
  "rules": {
    "@lightscript/unnecessary-comma": 0,
    "@lightscript/unnecessary-semi": 0,
    "fp/no-arguments": "error",
    "fp/no-class": "error",
    "fp/no-delete": "error",
    "fp/no-events": "error",
    "fp/no-get-set": "error",
    "fp/no-let": "error",
    "fp/no-loops": "error",
    "fp/no-mutating-assign": "error",
    "fp/no-mutation": "error",
    "fp/no-nil": "error",
    "fp/no-proxy": "error",
    "fp/no-rest-parameters": "error",
    "fp/no-this": "error",
    "fp/no-throw": "error",
    "fp/no-valueof-field": "error"
  }
}

[wcjohnson]

OK, so the reason this is happening is that the linter is unaware of LightScript's implicit returns. So that rule thinks the function is returning nothing, which is a violation.

There are two paths forward to fixing this:

1) Apply the implicit returns transform as part of the custom parser running inside the linter.

2) Monkey-patch the fp/no-nil rule (and other such rules as we become aware of them)

Both paths have their ups and downs:

1) Upside: All rules that deal with return semantics in a purely static-analysis kind of way would likely be fixed in one fell swoop. Downside: This is the sort of approach we used in the previous "hacky" eslint patch, and it causes all kinds of problems, like for example, rules that care about token position, i.e. the actual return token and its position in the file (which is nonexistent) will all probably break as a result.

2) Upside: Won't accidentally break any other rules or create a bastardized AST that eslint can't handle. Downside: Massive tech debt -- we have to add a monkey patch for every rule in the ecosystem that cares about return semantics, and every time one of those rules gets updated we'd have to update our monkey patched version.

The question is which way to go. (1) is definitely more appealing to me as a sole maintainer here, but I know what kinds of problems it can bring from past experience mutilating the AST that's passed into eslint. Re (2), There actually aren't a lot of rules in the core eslint suite that need to be modified, and yours is the first instance of an external rule so far, so I wouldn't totally write (2) off yet.

[Darkle]

Hmm, the downsides of both of those options don't seem that great.

Perhaps you should have the same approach you did before and just say: well there are just some eslint rules/plugins that don't work with LightScript?

For this particular case I'm ok with forking fp/no-nil and trying to patch it for my own usage.

[wcjohnson]

Because of #97, I'm basically forced into pursuing option (1) here and hoping that not too many style rules are impacted. So that's the plan.

[wcjohnson]

This also breaks react/require-render-return

[wcjohnson]

array-callback-return

[wcjohnson]

So after a lot of work, I've found approach 1 is basically a non-starter. Therefore, addressing this via approach 2.

I've added a commit that fixes all the documented cases so far, and I'm leaving this issue open for ongoing contributions of new issues regarding implicit return as they crop up.

[wcjohnson]

no-unused-expressions

[Darkle]

I may have found another one relating to noop's. The following code causes the no-unreachable eslint rule to trigger.

noop() -> return

foo() ->
  console.log('foo')

https://eslint.org/docs/rules/no-unreachable

Oddly enough if you change the foo function to be a one-liner, it doesn't trigger the eslint warning.

Changing the noop function to noop = () -> return also fixes it.

[wcjohnson]

That belongs under #104

It's likely I'll have to disable no-unreachable for the time being, although there is some potential for monkey patching the entire eslint code path analyzer that's being pursued by another guy who's trying to extend the linter, see #104