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

fix(eslint-plugin): [class-literal-property-style] ignore property assigned in constructor #8412

Merged
merged 2 commits into from Mar 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
134 changes: 101 additions & 33 deletions packages/eslint-plugin/src/rules/class-literal-property-style.ts
@@ -1,7 +1,13 @@
import type { TSESLint, TSESTree } from '@typescript-eslint/utils';
import { AST_NODE_TYPES } from '@typescript-eslint/utils';

import { createRule, getStaticStringValue } from '../util';
import {
createRule,
getStaticStringValue,
isAssignee,
isFunction,
nullThrows,
} from '../util';

type Options = ['fields' | 'getters'];
type MessageIds =
Expand All @@ -15,6 +21,11 @@ interface NodeWithModifiers {
static: boolean;
}

interface PropertiesInfo {
properties: TSESTree.PropertyDefinition[];
excludeSet: Set<string>;
}

const printNodeModifiers = (
node: NodeWithModifiers,
final: 'get' | 'readonly',
Expand Down Expand Up @@ -65,10 +76,71 @@ export default createRule<Options, MessageIds>({
},
defaultOptions: ['fields'],
create(context, [style]) {
function getMethodName(node: TSESTree.MethodDefinition): string {
return (
getStaticStringValue(node.key) ?? context.sourceCode.getText(node.key)
const propertiesInfoStack: PropertiesInfo[] = [];

function getStringValue(node: TSESTree.Node): string {
return getStaticStringValue(node) ?? context.sourceCode.getText(node);
}

function enterClassBody(): void {
propertiesInfoStack.push({
properties: [],
excludeSet: new Set(),
});
}

function exitClassBody(): void {
const { properties, excludeSet } = nullThrows(
propertiesInfoStack.pop(),
'Stack should exist on class exit',
);

properties.forEach(node => {
const { value } = node;
if (!value || !isSupportedLiteral(value)) {
return;
}

const name = getStringValue(node.key);
if (excludeSet.has(name)) {
return;
}

context.report({
node: node.key,
messageId: 'preferGetterStyle',
suggest: [
{
messageId: 'preferGetterStyleSuggestion',
fix(fixer): TSESLint.RuleFix {
const name = context.sourceCode.getText(node.key);

let text = '';
text += printNodeModifiers(node, 'get');
text += node.computed ? `[${name}]` : name;
text += `() { return ${context.sourceCode.getText(value)}; }`;

return fixer.replaceText(node, text);
},
},
],
});
});
}

function excludeAssignedProperty(node: TSESTree.MemberExpression): void {
if (isAssignee(node)) {
const { excludeSet } =
propertiesInfoStack[propertiesInfoStack.length - 1];

const name =
getStaticStringValue(node.property) ??
context.sourceCode.getText(node.property);

if (name) {
excludeSet.add(name);
}
}
}

return {
Expand All @@ -94,14 +166,14 @@ export default createRule<Options, MessageIds>({
return;
}

const name = getMethodName(node);
const name = getStringValue(node.key);

if (node.parent.type === AST_NODE_TYPES.ClassBody) {
const hasDuplicateKeySetter = node.parent.body.some(element => {
return (
element.type === AST_NODE_TYPES.MethodDefinition &&
element.kind === 'set' &&
getMethodName(element) === name
getStringValue(element.key) === name
);
});
if (hasDuplicateKeySetter) {
Expand Down Expand Up @@ -132,37 +204,33 @@ export default createRule<Options, MessageIds>({
},
}),
...(style === 'getters' && {
ClassBody: enterClassBody,
'ClassBody:exit': exitClassBody,
'MethodDefinition[kind="constructor"] ThisExpression'(
node: TSESTree.ThisExpression,
): void {
if (node.parent.type === AST_NODE_TYPES.MemberExpression) {
let parent: TSESTree.Node | undefined = node.parent;

while (!isFunction(parent)) {
parent = parent.parent;
}

if (
parent.parent.type === AST_NODE_TYPES.MethodDefinition &&
parent.parent.kind === 'constructor'
) {
excludeAssignedProperty(node.parent);
}
}
},
PropertyDefinition(node): void {
if (!node.readonly || node.declare) {
return;
}

const { value } = node;

if (!value || !isSupportedLiteral(value)) {
return;
}

context.report({
node: node.key,
messageId: 'preferGetterStyle',
suggest: [
{
messageId: 'preferGetterStyleSuggestion',
fix(fixer): TSESLint.RuleFix {
const name = context.sourceCode.getText(node.key);

let text = '';

text += printNodeModifiers(node, 'get');
text += node.computed ? `[${name}]` : name;
text += `() { return ${context.sourceCode.getText(value)}; }`;

return fixer.replaceText(node, text);
},
},
],
});
const { properties } =
propertiesInfoStack[propertiesInfoStack.length - 1];
properties.push(node);
},
}),
};
Expand Down
56 changes: 1 addition & 55 deletions packages/eslint-plugin/src/rules/prefer-for-of.ts
@@ -1,7 +1,7 @@
import type { TSESLint, TSESTree } from '@typescript-eslint/utils';
import { AST_NODE_TYPES } from '@typescript-eslint/utils';

import { createRule } from '../util';
import { createRule, isAssignee } from '../util';

export default createRule({
name: 'prefer-for-of',
Expand Down Expand Up @@ -102,60 +102,6 @@ export default createRule({
);
}

function isAssignee(node: TSESTree.Node): boolean {
const parent = node.parent;
if (!parent) {
return false;
}

// a[i] = 1, a[i] += 1, etc.
if (
parent.type === AST_NODE_TYPES.AssignmentExpression &&
parent.left === node
) {
return true;
}

// delete a[i]
if (
parent.type === AST_NODE_TYPES.UnaryExpression &&
parent.operator === 'delete' &&
parent.argument === node
) {
return true;
}

// a[i]++, --a[i], etc.
if (
parent.type === AST_NODE_TYPES.UpdateExpression &&
parent.argument === node
) {
return true;
}

// [a[i]] = [0]
if (parent.type === AST_NODE_TYPES.ArrayPattern) {
return true;
}

// [...a[i]] = [0]
if (parent.type === AST_NODE_TYPES.RestElement) {
return true;
}

// ({ foo: a[i] }) = { foo: 0 }
if (
parent.type === AST_NODE_TYPES.Property &&
parent.value === node &&
parent.parent.type === AST_NODE_TYPES.ObjectExpression &&
isAssignee(parent.parent)
) {
return true;
}

return false;
}

function isIndexOnlyUsedWithArray(
body: TSESTree.Statement,
indexVar: TSESLint.Scope.Variable,
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/util/index.ts
Expand Up @@ -15,6 +15,7 @@ export * from './isUndefinedIdentifier';
export * from './misc';
export * from './objectIterators';
export * from './types';
export * from './isAssignee';

// this is done for convenience - saves migrating all of the old rules
export * from '@typescript-eslint/type-utils';
Expand Down
56 changes: 56 additions & 0 deletions packages/eslint-plugin/src/util/isAssignee.ts
@@ -0,0 +1,56 @@
import type { TSESTree } from '@typescript-eslint/utils';
import { AST_NODE_TYPES } from '@typescript-eslint/utils';

export function isAssignee(node: TSESTree.Node): boolean {
const parent = node.parent;
if (!parent) {
return false;

Check warning on line 7 in packages/eslint-plugin/src/util/isAssignee.ts

View check run for this annotation

Codecov / codecov/patch

packages/eslint-plugin/src/util/isAssignee.ts#L7

Added line #L7 was not covered by tests
}

// a[i] = 1, a[i] += 1, etc.
if (
parent.type === AST_NODE_TYPES.AssignmentExpression &&
parent.left === node
) {
return true;
}

// delete a[i]
if (
parent.type === AST_NODE_TYPES.UnaryExpression &&
parent.operator === 'delete' &&
parent.argument === node
) {
return true;
}

// a[i]++, --a[i], etc.
if (
parent.type === AST_NODE_TYPES.UpdateExpression &&
parent.argument === node
) {
return true;

Check warning on line 32 in packages/eslint-plugin/src/util/isAssignee.ts

View check run for this annotation

Codecov / codecov/patch

packages/eslint-plugin/src/util/isAssignee.ts#L32

Added line #L32 was not covered by tests
}

// [a[i]] = [0]
if (parent.type === AST_NODE_TYPES.ArrayPattern) {
return true;
}

// [...a[i]] = [0]
if (parent.type === AST_NODE_TYPES.RestElement) {
return true;
}

// ({ foo: a[i] }) = { foo: 0 }
if (
parent.type === AST_NODE_TYPES.Property &&
parent.value === node &&
parent.parent.type === AST_NODE_TYPES.ObjectExpression &&
isAssignee(parent.parent)
) {
return true;
}

return false;
}