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

Incorrectly triggers no-undef and no-redeclare in some cases #2477

Closed
3 tasks done
otakustay opened this issue Sep 3, 2020 · 15 comments
Closed
3 tasks done

Incorrectly triggers no-undef and no-redeclare in some cases #2477

otakustay opened this issue Sep 3, 2020 · 15 comments
Assignees
Labels
bug Something isn't working has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@otakustay
Copy link

otakustay commented Sep 3, 2020

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have read the FAQ and my problem is not listed.

Repro

https://github.com/otakustay/typescript-eslint-issue

Expected Result

No errors

Actual Result

as-const-undef.ts
  4:6  error  'const' is not defined  no-undef

generic-jsx-no-undef.tsx
  12:19  error  'T' is not defined  no-undef

type-assertion-no-undef.ts
  6:43  error  'config' is not defined  no-undef

type-var-redef.tsx
  5:7  error  'Value' is already defined  @typescript-eslint/no-redeclare

✖ 3 problems (3 errors, 0 warnings)

Additional Info

Versions

package version
@typescript-eslint/parser 4.0.1
TypeScript 4.0.2
ESLint 7.8.1
node 14.7.0
@otakustay otakustay added package: parser Issues related to @typescript-eslint/parser triage Waiting for maintainers to take a look labels Sep 3, 2020
@bradzacher bradzacher added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin and removed package: parser Issues related to @typescript-eslint/parser triage Waiting for maintainers to take a look labels Sep 3, 2020
@tadhgmister
Copy link
Contributor

#2462 covers the type-assertion-no-undef case.

@wKovacs64
Copy link

wKovacs64 commented Sep 4, 2020

Should 4.x be finding types from third-party namespaces? Things like React and JSX seem to trigger no-undef now:

import * as React from 'react';

function Hello(): JSX.Element { // 'JSX' is not defined. eslint(no-undef)
  return <h1>Hello</h1>;
}

export default Hello;

↓ ↓ ↓

C:\dev\test\no-undef-react-repro\src\hello.tsx
  3:19  error  'JSX' is not defined  no-undef

image

I read the release notes, and it mentions parserOptions.lib but that doesn't cover this, to my knowledge. I also saw plugin:@typescript-eslint/recommended disables no-undef but I'm unclear if that's now required or if this is a bug.

@bradzacher
Copy link
Member

The recommended set turns of no-undef because it's covered by TS, so double reporting is somewhat useless.

Some people do prefer to have the rule on so they can get the errors via a lint run. Each to their own.

parserOptions.lib will include types that are defined by typescript itself.
The JSX namespace is actually a 3rd party namespace, defined in @types/react.

You can define any additional globals you need for your project: https://eslint.org/docs/user-guide/configuring#specifying-globals

@wKovacs64
Copy link

The JSX namespace is actually a 3rd party namespace, defined in @types/react.

Right, understood. What do we need to do in order to get no-undef to be aware of those? Or can't we?

You can define any additional globals you need for your project: https://eslint.org/docs/user-guide/configuring#specifying-globals

Sure, but does that apply to React/JSX? We didn't have to do that in v3 - is that because v3 was "broken" and v4 is fixed, as is somewhat mentioned in the release notes?

@bradzacher
Copy link
Member

Pre v4 the scope analyser (this piece of infra which backs rules likeno-undef, no-shadow and no-unused-vars) essentially barely worked.

The implementation was a hastily patched version of the core eslint scope analyser in an attempt to teach it about some common use cases for types, but it was a bad approach to solve the problem because typescript's types scope is super complex. There were many bugs, and it sucked.

For v4 I forked the core eslint scope analyser and more or less rewrote it from the ground up so that it can analyse and understand the dual scope (values and types) nature of TS.

So now you need to define globals for global types (as you would for global variables) so that rules can correctly understand your project.

@wKovacs64
Copy link

Thanks for the clarification and all your work on this.

@Knagis
Copy link

Knagis commented Sep 4, 2020

What about the no-redeclare case?

type foo = string;
const foo = "123";

this triggers @typescript-eslint/no-redeclare. Unfortunately the only option is for declaration merging, that does not cover declaring type and value with the same name (basically simulating classes).

@Knagis
Copy link

Knagis commented Sep 4, 2020

Btw, after fixing a lot of instances of the no-redeclare issues in our large codebase it seems that the current behavior is beneficial. Some 80%-90% of the situations where type is named the same as function/variable are not correct and requires rename of the type. Only the remaining cases are ones where it is actually intended to simulate class/enum-like variable/type combo.

@bradzacher
Copy link
Member

bradzacher commented Sep 4, 2020

I think most codebases will not want to allow type/variable redeclaration like that. For those cases just using a disable comment is probably better to indicate that it's entirely intention they're named the same.

But I think adding an option to allow it (defaulting to false) is probably a good idea for codebases that use the pattern a lot. TS will catch any actual issues that might arise so no point in completely blocking it.

@otakustay
Copy link
Author

Yes I admit that disallow type/variable redeclaration is reasonable, we can note it in rule's document maybe

bradzacher added a commit that referenced this issue Sep 6, 2020
Fixes #2455
And part of #2477

JSX is a first-class citizen of TS, so we should really support it as well.
I was going to just rely upon `eslint-plugin-react`'s patch lint rules (`react/jsx-uses-react` and `react/jsx-uses-vars`), but that leaves gaps in our tooling.
For example #2455, `consistent-type-imports` makes assumptions and can create invalid fixes for react without this change.
We could add options to that lint rule for the factory, but that is kind-of a sub-par experience and future rule authors will likely run into similar problems.

- Adds full scope analysis support for JSX.
- Adds two new `parserOption`:
    - `jsxPragma` - the name to use for constructing JSX elements. Defaults to `"React"`. Will be auto detected from the tsconfig.
    - `jsxFragmentName` - the name that unnamed JSX fragments use. Defaults to `null` (i.e. assumes `React.Fragment`). Will be auto detected from the tsconfig.
bradzacher added a commit that referenced this issue Sep 6, 2020
bradzacher added a commit that referenced this issue Sep 6, 2020
Fixes #2455
And part of #2477

JSX is a first-class citizen of TS, so we should really support it as well.
I was going to just rely upon `eslint-plugin-react`'s patch lint rules (`react/jsx-uses-react` and `react/jsx-uses-vars`), but that leaves gaps in our tooling.
For example #2455, `consistent-type-imports` makes assumptions and can create invalid fixes for react without this change.
We could add options to that lint rule for the factory, but that is kind-of a sub-par experience and future rule authors will likely run into similar problems.

- Adds full scope analysis support for JSX.
- Adds two new `parserOption`:
    - `jsxPragma` - the name to use for constructing JSX elements. Defaults to `"React"`. Will be auto detected from the tsconfig.
    - `jsxFragmentName` - the name that unnamed JSX fragments use. Defaults to `null` (i.e. assumes `React.Fragment`). Will be auto detected from the tsconfig.
bradzacher added a commit that referenced this issue Sep 6, 2020
@bradzacher
Copy link
Member

bradzacher commented Sep 6, 2020

as-const-undef.ts
  4:6  error  'const' is not defined  no-undef
generic-jsx-no-undef.tsx
  12:19  error  'T' is not defined  no-undef
type-assertion-no-undef.ts
  6:43  error  'config' is not defined  no-undef
type-var-redef.tsx
  5:7  error  'Value' is already defined  @typescript-eslint/no-redeclare

bradzacher added a commit that referenced this issue Sep 6, 2020
bradzacher added a commit that referenced this issue Sep 6, 2020
@bradzacher bradzacher added the has pr there is a PR raised to close this label Sep 6, 2020
@bradzacher bradzacher self-assigned this Sep 6, 2020
bradzacher added a commit that referenced this issue Sep 6, 2020
bradzacher added a commit that referenced this issue Sep 6, 2020
@tadhgmister
Copy link
Contributor

@bradzacher regarding const and type overlapping, Don't worry about an option but I just wanted to show my use case:

I have a class that represents a react component but in order to be used as one it has to be wrapped in a React.forwardRef so I end up exporting the valid react component and an alias to the instance type with the same name so ref works the same as it would for other react components. (link)

/**
 * react component to implement Popout_Cls hook class.
 * @see Popout_Cls for full details
 */
export const Popout = Popout_Cls.createComponent();
/** type is an alias to Popout_Cls so that importing react component also imports ref type */
export type Popout = Popout_Cls;
export default Popout;

This way the behaviour of importing a class imports both the value used in JSX <Popout> and the type of the instance, this has the exact same behaviour.

So at least for me an option to allow type and const overlap, but only when they are declared directly sequentially would be a nice option, but don't worry about implementing it just for me. 😛

@lll000111
Copy link

Using the latest release (eslint, typescript-eslint), I still get "no-redeclare" errors for all instances of function (header) overloading.

@lll000111
Copy link

lll000111 commented Sep 10, 2020

I also get 'NodeJS' is not defined from no-undef, but that is defined - in @types/node. The other undefined messages, e.g. from "as const" are gone but this one remains.

@lll000111
Copy link

@bradzacher The no-redeclare issue has not been solved fully. Function overload declarations (multiple headers-only besides the actual function) still trigger it.

Using

$ yarn list --depth 0|grep eslint
├─ @eslint/eslintrc@0.1.3
├─ @typescript-eslint/eslint-plugin@4.1.1
├─ @typescript-eslint/experimental-utils@4.1.1
├─ @typescript-eslint/parser@4.1.1
├─ @typescript-eslint/scope-manager@4.1.1
├─ @typescript-eslint/types@4.1.1
├─ @typescript-eslint/typescript-estree@4.1.1
├─ @typescript-eslint/visitor-keys@4.1.1
├─ eslint-config-prettier@6.11.0
├─ eslint-plugin-es@3.0.1
├─ eslint-plugin-jsdoc@30.5.1
├─ eslint-plugin-node@11.1.0
├─ eslint-plugin-prettier@3.1.4
├─ eslint-scope@5.1.1
├─ eslint-utils@2.1.0
├─ eslint-visitor-keys@1.3.0
├─ eslint@7.9.0

Tigge added a commit to Tigge/eslint-config-typescript-shareable that referenced this issue Oct 1, 2020
Use @typescript-eslint versions of the no-shadow and no-redeclare rules.

Remove the no-undef rule since that is covered by typescript and not in
the recommended set of rules. See also
<typescript-eslint/typescript-eslint#2477 (comment)>.
Tigge added a commit to Tigge/eslint-config-typescript-shareable that referenced this issue Oct 1, 2020
Use @typescript-eslint versions of the no-shadow and no-redeclare rules.

Remove the no-undef rule since that is covered by typescript and not in
the recommended set of rules. See also
<typescript-eslint/typescript-eslint#2477 (comment)>.
anius pushed a commit to AxisCommunications/eslint-config-typescript-shareable that referenced this issue Oct 2, 2020
Use @typescript-eslint versions of the no-shadow and no-redeclare rules.

Remove the no-undef rule since that is covered by typescript and not in
the recommended set of rules. See also
<typescript-eslint/typescript-eslint#2477 (comment)>.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

6 participants