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

Undeprecate no-use-before-declare #4789

Closed
ikokostya opened this issue Jul 9, 2019 · 6 comments
Closed

Undeprecate no-use-before-declare #4789

ikokostya opened this issue Jul 9, 2019 · 6 comments

Comments

@ikokostya
Copy link

Feature request

Is your feature request related to a problem? Please describe.

Rule no-use-before-declare was deprecated in the PR #4695, but as @karol-majewski noticed TypeScript compiler can't detect some errors.

For example, the following code:

const consumer = () => {
  console.log(variable);
};

consumer();

const variable = 'foo';

doesn't produce any error (TypeScript playground), but causes runtime error.

While deprecated no-use-before-declare rule detects this error:

variable 'variable' used before declaration (no-use-before-declare)
  1 | const consumer = () => {
> 2 |     console.log(variable);
    |                ^
  3 | };
  4 | 
  5 | consumer();

Describe the solution you'd like

Undeprecate no-use-before-declare.

Describe alternatives you've considered

File issue to TypeScript project as parallel process.

@JoshuaKGoldberg

This comment has been minimized.

@ikokostya
Copy link
Author

Because example from issue #2551 is not isolated and contains some project specific entities, I tried to test code from replies:

#2551 (comment)
#2551 (comment)

export function foo ({ param: p }: { param: any }) {
}

declare let user1: any;

const { email: user1Email, name: user1Name }: { email: string, name: string } = user1;

There are no errors with no-use-before-declare rule and latest version of tslint 5.18.0 for me.

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Jul 10, 2019

@adidahiya do you have more context on why the rule was deprecated? I'm not seeing anything linked by #3520.

Edit: for future reference, it's been mentioned as slow as far back as #1261.

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Jul 10, 2019

If we take the code from the OP and move consumer(); to after variable, the rule still gives a complaint. This is the false positive mentioned in the other discussions.

const consumer = () => {
  console.log(variable);
};

const variable = "foo";

consumer();

#4695 (comment)
False positive result is not a reason for removing rule. By this logic any buggy rule can be removed.

Well, no, there's a structural difference between a rule that has an implementation bug verses a rule that has a seemingly-unfixable structural bug. It would be quite a lot of work to make the rule understand that placing consumer(); before variable is a bug but variable before consumer(); is not. The rule would likely have to do something like converting the source file into a graph and checking for cycles in the identifiers and references. That's out of scope for the core ruleset.

If the rule is to be enabled, I think it would be a requirement to clearly mention its shortcomings in the docs per this thread. Thoughts?

@ikokostya
Copy link
Author

ikokostya commented Jul 10, 2019

If the rule is to be enabled, I think it would be a requirement to clearly mention its shortcomings in the docs per this thread. Thoughts?

I think it's good tradeoff. In original topic @OliverJAsh mentioned about it.

@adidahiya
Copy link
Contributor

This should be covered by the eslint rule: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-use-before-define.md

Closing as per deprecation timeline in #4534

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

No branches or pull requests

3 participants