Skip to content

[no-shadow] declaring a global causes no-shadow to error #2654

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.

Sign up for GitHub

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

Jump to bottom
Closed
3 tasks done
salemhilal opened this issue Oct 8, 2020 · 7 comments · Fixed by #10710
Closed
3 tasks done

[no-shadow] declaring a global causes no-shadow to error #2654

salemhilal opened this issue Oct 8, 2020 · 7 comments · Fixed by #10710
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request good first issue Good for newcomers locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@salemhilal
Copy link

Hey, I think I found what's either a bug or a confusing way that globals and ambient types are handled (potentially similarly to the no-undef issue mentioned in the FAQs).

I'm currently migrating a large codebase to use TypeScript. We have a global has() function that we use to determine what environment we're in. It's handled by our build system, and is set as a read-only global in our existing ESLint config.

When I try and declare an ambient type for this global, I've noticed that @typescript-eslint/no-shadow thinks that I'm shadowing it. Removing globals: { has: false } from the config fixes this problem. Is this expected behavior?

Thanks in advance, and sorry if I'm overlooking something.

  • 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

I set up a fresh project by running the following in an empty directory:

yarn init -y
yarn add -D eslint typescript @typescript-eslint/parser @typescript-eslint/eslint-plugin

I then added these two files

// .eslintrc.js
module.exports = {
  root: true,
  parser: '@typescript-eslint/parser',
  plugins: [
    '@typescript-eslint',
  ],
  extends: [
    'eslint:recommended',
    'plugin:@typescript-eslint/recommended',
  ],
  globals: {
    has: false,
  },
  rules: {
    "no-shadow": "off",
    "@typescript-eslint/no-shadow": ["error", { builtinGlobals: true }],
  }
};
// index.d.ts
declare const has: (environment: "dev" | "prod" | "test") => boolean;

Expected Result

I'd expect yarn eslint index.d.ts to not error.

Actual Result

It logged the following error:

 1:15  error  'has' is already declared in the upper scope  @typescript-eslint/no-shadow

Versions

package version
@typescript-eslint/eslint-plugin 4.4.0
@typescript-eslint/parser 4.4.0
TypeScript 4.0.3
ESLint 7.10.0
node 12.15.0
@salemhilal salemhilal added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Oct 8, 2020
@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party external This issue is with another package, not typescript-eslint itself and removed triage Waiting for team members to take a look labels Oct 8, 2020
@bradzacher
Copy link
Member

bradzacher commented Oct 8, 2020 •
edited
Loading

Are you saying that because it's a declare it shouldn't error?
Or are you saying that because it's a .d.ts file it shouldn't error?

Could you please clarify?

@bradzacher bradzacher removed the external This issue is with another package, not typescript-eslint itself label Oct 8, 2020
@salemhilal
Copy link
Author

salemhilal commented Oct 8, 2020 •
edited
Loading

@bradzacher perhaps both, although I think the declare statement in particular is the problem here.
Edit: to clarify, I'd expect declare statements in nond.ts files to also not have this error. I think I'd only expect this error to show up when either a type shadows a type, or a variable shadows a variable.

@bradzacher
Copy link
Member

I think I'd only expect this error to show up when either a type shadows a type, or a variable shadows a variable.

Because of limitations, a global you've defined in your config doesn't have the context about if it's a type or a value - so it's treated as both.

This is done on purpose so users can define global types that work with no-undef, if they want to use that rule. (to be clear - I'm firmly against using that rule with TS because TS does it better with less config... but 🤷 people do what they like)

Also side note that declare const has isn't a "type", it's a "value". It's just that TS doesn't emit a declaration for it.

This is what's happening: rule reports here because

You've got a global thing defined in your config.
The scope analyser starts to analyse your file, so it creates the "global" scope to house everything in the file.
Within this scope it defines all the global variables configured by you explicitly, and implicitly by your configured environments.

Because you've configured parserOptions.sourceType = 'module' the scope analyser then creates an implicit child scope called the "module" scope (this is done because of the JS spec and how it works).

The analyser then proceeds to analyse your file.
With declare const has, you're declaring a local value variable in your file, that gets put in the "module" scope.

Because the "module" scope is a child of the "global" scope, the local variable has shadows the global variable has.
Hence the rule reports.

Our extension rule probably could be modified to stop declared things from triggering the rule at all within a .d.ts file.

@bradzacher bradzacher added enhancement New feature or request good first issue Good for newcomers and removed awaiting response Issues waiting for a reply from the OP or another party labels Oct 8, 2020
@salemhilal
Copy link
Author

Thanks for the quick response! til declare const has is declaring a value, not a type. If I have some spare time, I can look into what it would take to have declarations in .d.ts files get ignored by this rule.

@InExtremaRes

This comment has been minimized.

Sign in to view
@bradzacher

This comment has been minimized.

Sign in to view
@arminyahya
Copy link
Contributor

Hi everyone.
So we want an option for @typescript-eslint/no-shadow to ignore d.ts files?

@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Feb 24, 2022
@Josh-Cena Josh-Cena changed the title [@typescript-eslint/no-shadow] setting a global causes no-shadow to error on ambient types [no-shadow] declaring a global causes no-shadow to error Jun 5, 2024
@sopa301 sopa301 mentioned this issue Jan 24, 2025
Merged
3 tasks
@github-actions github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Feb 11, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request good first issue Good for newcomers locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
5 participants
@salemhilal @InExtremaRes @JoshuaKGoldberg @bradzacher @arminyahya