Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

return-undefined false positive #2812

Closed
cartant opened this issue May 23, 2017 · 5 comments · Fixed by #3298
Closed

return-undefined false positive #2812

cartant opened this issue May 23, 2017 · 5 comments · Fixed by #3298

Comments

@cartant
Copy link

cartant commented May 23, 2017

Bug Report

  • TSLint version: 5.3.0
  • TypeScript version: 2.3.3
  • Running TSLint via: CLI

TypeScript code being linted

export function t(p: (a: any) => any): void {}
t((a) => { return undefined; });

with tslint.json configuration:

{
  "rules": {
    "return-undefined": { "severity": "error" }
  }
}

Actual behavior

ERROR: ...: `void` function should use `return;`, not `return undefined;`.

Expected behavior

No error, as the function parameter is typed as returning any - not void.

Prior to updating to the latest version of TSLint, the configuration did not see an error effected. No error is effected if 5.2.0 is used.

cartant added a commit to cartant/firebase-nightlight that referenced this issue May 23, 2017
@adidahiya
Copy link
Contributor

adidahiya commented May 23, 2017

looks like a direct result of #2731. notice the test baseline that got changed: https://github.com/palantir/tslint/pull/2731/files#diff-bd90f0718fe7a8725e9db572ef333339R36

@cartant can you provide a bit more context around the code your'e trying to lint here? should it really be typed any?

... either way, this behavior is kind of surprising, so we should probably revert that part of the rule change @andy-hanson

@cartant
Copy link
Author

cartant commented May 23, 2017

@adidahiya Sure. The typings aren't mine and any is reasonably appropriate for the usage.

It's a transactionUpdate parameter in the Firebase JavaScript client:

transaction (
  transactionUpdate : (a : any ) => any ,
  onComplete ? : (
    a : Error | null ,
    b : boolean ,
    c : firebase.database.DataSnapshot | null
  ) => any ,
  applyLocally ? : boolean
) : firebase.Promise < any > ;

I need to implement that interface in a mock.

@cartant
Copy link
Author

cartant commented May 23, 2017

@adidahiya And the code that fails the linting is the test for the mock:

return mockRef
    .transaction((value) => {

        return undefined;
    })
    .then(({ committed, snapshot }) => {

Firebase transactions return undefined if nothing is updated.

@cartant
Copy link
Author

cartant commented May 29, 2017

Seeing as though this is a breaking change, is it going to be considered a bug? Or should I implement the trivial workaround that'll be required if this is indeed how you want the rule to work?

@adidahiya
Copy link
Contributor

I consider it a bug. Will label it as such.

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

Successfully merging a pull request may close this issue.

2 participants