fix(eslint-plugin): [consistent-indexed-object-style] fixed circular reference issue #2766
Conversation
|
Thanks for the PR, @lakshyabatman! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
| const value = sourceCode.getText(valueType.typeAnnotation); | ||
| const name = node.parent?.id?.name; | ||
| if (value.includes(name)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Hello! Thank you for the PR for my issue. I do not have experience in coding linters, but it seems to me that this simple includes string check could give a false positive for something like type Tree = Record<string, AnotherTree>, couldn't it?
There was a problem hiding this comment.
Yes, true. Thanks for letting me know. I've fixed that in the following commit.
| const value = sourceCode.getText(valueType.typeAnnotation); | ||
| const name = node.parent?.id?.name; | ||
| const valueArray = value.split('|').map(v => v.trim()); | ||
| if (valueArray.indexOf(name) != -1) { |
There was a problem hiding this comment.
doing operations and linting based on text is not correct and will always cause false positive or false negative.
here are a number of cases you'll false-positive
type T = { [k: string]: AT };
type T = { [k: string]: TA };
type T = { [k: string]: A.T };
type T = { [k: string]: A['T'] };
type T = { [k: string]: (A: string) => void };
// ...You need to use scope analysis here.
https://eslint.org/docs/developer-guide/scope-manager-interface
Look for references within the current node.
search this or the ESLint core codebase for context.getScope for examples
There was a problem hiding this comment.
Okay, I'll come back with a fix as soon as possible, thanks for the help.
There was a problem hiding this comment.
@bradzacher I did some changes but I feel the logic I implemented is too bloated, please review and let me know if I can refactor it, thankyou.
Codecov Report
@@ Coverage Diff @@
## master #2766 +/- ##
==========================================
- Coverage 92.80% 92.77% -0.03%
==========================================
Files 300 300
Lines 9852 9871 +19
Branches 2767 2778 +11
==========================================
+ Hits 9143 9158 +15
Misses 332 332
- Partials 377 381 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ae73b6c to
b760395
Compare
| const membersArray = memberTypes.types; | ||
| let flag = false; | ||
| membersArray.forEach((m: TSESTree.Node) => { | ||
| if ( | ||
| m.type == AST_NODE_TYPES.TSTypeReference && | ||
| m.typeName.type == AST_NODE_TYPES.Identifier && | ||
| m.typeName.name == name | ||
| ) { | ||
| flag = true; | ||
| } | ||
| }); | ||
| if (flag) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
You can use a for-of loop instead of .forEach and that allows you to return directly instead of using the flag:
| const membersArray = memberTypes.types; | |
| let flag = false; | |
| membersArray.forEach((m: TSESTree.Node) => { | |
| if ( | |
| m.type == AST_NODE_TYPES.TSTypeReference && | |
| m.typeName.type == AST_NODE_TYPES.Identifier && | |
| m.typeName.name == name | |
| ) { | |
| flag = true; | |
| } | |
| }); | |
| if (flag) { | |
| return; | |
| } | |
| const membersArray: TSESTree.Node[] = memberTypes.types; | |
| for (const m of membersArray) { | |
| if ( | |
| m.type == AST_NODE_TYPES.TSTypeReference && | |
| m.typeName.type == AST_NODE_TYPES.Identifier && | |
| m.typeName.name == name | |
| ) { | |
| return; | |
| } | |
| } |
There was a problem hiding this comment.
your code doesn't actually use the scope manager.
All you've done here is added a layer of indirection for getting the current AST node!
The scope manager is a powerful tool which lets you see the variables defined in a scope as well as the references to that variable.
const scope = context.getScope();
const variable = scope.set.get(name);
const references = variable.references;You can look at the references to the variable and determine if the references exist outside the type declaration
|
@bradzacher I'm sorry for the delay, but before proceeding with any changes I thought to share the code snippet to check if it's fine from your side. const scope = context.getScope();
const block = scope.block;
if (block.type == AST_NODE_TYPES.Program) {
if (block.body[0].type == AST_NODE_TYPES.TSTypeAliasDeclaration) {
const name = block.body[0].id.name;
const variables = scope.set.get(name);
const references = variables?.references ?? [];
if (references.length > 0) {
if (references[0].from.type == 'global') {
return;
}
}
}
}Though it would work only in case of the type declaration. Although in situations like, interface A {
key: A;
}The trigger should not work. |
|
You don't need the block checks. Something like this: const scope = context.getScope();
const variable = scope.set.get(typeName);
const references = variables?.references ?? [];
for (const ref of references) {
if (
ref.identifier.range[0] >= interfaceOrTypeNode.range[0] &&
ref.identifier.range[1] <= interfaceOrTypeNode.range[1]
) {
// is a self-reference as the reference identifier is inside the interface/type declaration
}
} |
The logic checks if the value includes the node name it'll not trigger the rule.
Fixes #2687