Skip to content

fix(eslint-plugin): [no-shadow] ignore ordering of type declarations #10593

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

Merged
merged 5 commits into from
Jan 13, 2025
Merged

fix(eslint-plugin): [no-shadow] ignore ordering of type declarations #10593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f9f48f6
initial implementation
ronami Dec 24, 2024
0eb600b
add relevant tests
ronami Dec 26, 2024
0a63dc2
add docs
ronami Jan 2, 2025
b164971
snapshots
ronami Jan 2, 2025
7dd6b06
match original implementation
ronami Jan 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions packages/eslint-plugin/docs/rules/no-shadow.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,52 @@ It adds support for TypeScript's `this` parameters and global augmentation, and
This rule adds the following options:

```ts
type AdditionalHoistOptionEntries = 'types' | 'functions-and-types';

type HoistOptionEntries =
| BaseNoShadowHoistOptionEntries
| AdditionalHoistOptionEntries;

interface Options extends BaseNoShadowOptions {
hoist?: HoistOptionEntries;
ignoreTypeValueShadow?: boolean;
ignoreFunctionTypeParameterNameValueShadow?: boolean;
}

const defaultOptions: Options = {
...baseNoShadowDefaultOptions,
hoist: 'functions-and-types',
ignoreTypeValueShadow: true,
ignoreFunctionTypeParameterNameValueShadow: true,
};
```

### hoist: `types`

Examples of incorrect code for the `{ "hoist": "types" }` option:

```ts option='{ "hoist": "types" }' showPlaygroundButton
type Bar<Foo> = 1;
type Foo = 1;
```

### hoist: `functions-and-types`

Examples of incorrect code for the `{ "hoist": "functions-and-types" }` option:

```ts option='{ "hoist": "functions-and-types" }' showPlaygroundButton
// types
type Bar<Foo> = 1;
type Foo = 1;

// functions
if (true) {
let b = 6;
}

function b() {}
```

### `ignoreTypeValueShadow`

{/* insert option description */}
Expand Down
46 changes: 34 additions & 12 deletions packages/eslint-plugin/src/rules/no-shadow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
{
allow?: string[];
builtinGlobals?: boolean;
hoist?: 'all' | 'functions' | 'never';
hoist?: 'all' | 'functions' | 'functions-and-types' | 'never' | 'types';
ignoreFunctionTypeParameterNameValueShadow?: boolean;
ignoreOnInitialization?: boolean;
ignoreTypeValueShadow?: boolean;
Expand All @@ -28,6 +28,13 @@
AST_NODE_TYPES.TSConstructorType,
]);

const functionsHoistedNodes = new Set([AST_NODE_TYPES.FunctionDeclaration]);

const typesHoistedNodes = new Set([
AST_NODE_TYPES.TSInterfaceDeclaration,
AST_NODE_TYPES.TSTypeAliasDeclaration,
]);

export default createRule<Options, MessageIds>({
name: 'no-shadow',
meta: {
Expand Down Expand Up @@ -63,7 +70,7 @@
type: 'string',
description:
'Whether to report shadowing before outer functions or variables are defined.',
enum: ['all', 'functions', 'never'],
enum: ['all', 'functions', 'functions-and-types', 'never', 'types'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've expanded the existing hoist configuration to include types and functions-and-types. I don't think this is optional, as permutations need to be manually set ...

Yeah, I'm thinking this exposes a general learning: don't use enum option types for "when do I X?" questions. Eventually folks will want to do something that could be better expressed as an object. I.e. here:

{ optionName: { functions: true, types: true, variables: true } }

...but, to your point, I think it would be even more disruptive to extend the base rule option to a wholly different format in that way. For now I think the enum is fine. If this rule gets even more complicated over time then we can always switch to the object later.

tl;dr: agreed as-is 👍

},
ignoreFunctionTypeParameterNameValueShadow: {
type: 'boolean',
Expand All @@ -88,7 +95,7 @@
{
allow: [],
builtinGlobals: false,
hoist: 'functions',
hoist: 'functions-and-types',
ignoreFunctionTypeParameterNameValueShadow: true,
ignoreOnInitialization: false,
ignoreTypeValueShadow: true,
Expand Down Expand Up @@ -513,15 +520,30 @@
const inner = getNameRange(variable);
const outer = getNameRange(scopeVar);

return !!(
inner &&
outer &&
inner[1] < outer[0] &&
// Excepts FunctionDeclaration if is {"hoist":"function"}.
(options.hoist !== 'functions' ||
!outerDef ||
outerDef.node.type !== AST_NODE_TYPES.FunctionDeclaration)
);
if (!inner || !outer || inner[1] >= outer[0]) {
return false;
}

if (!outerDef) {
return true;

Check warning on line 528 in packages/eslint-plugin/src/rules/no-shadow.ts

View check run for this annotation

Codecov / codecov/patch

packages/eslint-plugin/src/rules/no-shadow.ts#L528 

Added line #L528 was not covered by tests
}
Comment on lines +527 to +529
Copy link
Member Author

@ronami ronami Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went over this again and made one small change. I think I've introduced an unnecessary change to the original implementation (returning false instead of true if outerDef is false, see 7dd6b06).

I don't think this code is reachable (all tests seem to pass regardless of this change), and I couldn't reproduce this in a test. Still, just to be sure, I've changed this to match the original implementation.

Copy link
Member Author

@ronami ronami Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that it's on its own if statement, it fails codecov (even though this wasn't covered previously but was in a single long binary operation). I'm a bit unsure what the best way to tackle this is. I'd normally use nullThrows or a type assertion, but I'm not entirely sure this really is an unreachable line.

This part of the code seems to originate from the original eslint rule and is there from the very beginning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'd just ignore Codecov here. Its reporter isn't perfect.


if (options.hoist === 'functions') {
return !functionsHoistedNodes.has(outerDef.node.type);
}

if (options.hoist === 'types') {
return !typesHoistedNodes.has(outerDef.node.type);
}

if (options.hoist === 'functions-and-types') {
return (
!functionsHoistedNodes.has(outerDef.node.type) &&
!typesHoistedNodes.has(outerDef.node.type)
);
}

return true;
}

/**
Expand Down
29 changes: 28 additions & 1 deletion packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-shadow.shot
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading