Skip to content

Commit

Permalink
fix(eslint-plugin): [prefer-readonly] refine report locations (#8894)
Browse files Browse the repository at this point in the history
* [prefer-readonly] Refine report locations

* wip

* bring stuff to shared code

* lint
  • Loading branch information
kirkwaiblinger committed Jun 17, 2024
1 parent 1788214 commit 50ed604
Show file tree
Hide file tree
Showing 5 changed files with 369 additions and 146 deletions.
92 changes: 8 additions & 84 deletions packages/eslint-plugin/src/rules/explicit-member-accessibility.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ import {
nullThrows,
NullThrowsReasons,
} from '../util';
import {
getMemberHeadLoc,
getParameterPropertyHeadLoc,
} from '../util/getMemberHeadLoc';

type AccessibilityLevel =
| 'explicit' // require an accessor (including public)
Expand Down Expand Up @@ -166,7 +170,7 @@ export default createRule<Options, MessageIds>({
});
} else if (check === 'explicit' && !methodDefinition.accessibility) {
context.report({
loc: getMissingAccessibilityReportLoc(methodDefinition),
loc: getMemberHeadLoc(context.sourceCode, methodDefinition),
messageId: 'missingAccessibility',
data: {
type: nodeType,
Expand Down Expand Up @@ -221,87 +225,6 @@ export default createRule<Options, MessageIds>({
return { range: keywordRange, rangeToRemove };
}

/**
* For missing accessibility modifiers, we want to report any keywords
* out in front of the key, and the key itself, but not anything afterwards,
* i.e. parens, type annotations, method bodies, or `?`.
*/
function getMissingAccessibilityReportLoc(
node:
| TSESTree.MethodDefinition
| TSESTree.TSAbstractMethodDefinition
| TSESTree.PropertyDefinition
| TSESTree.TSAbstractPropertyDefinition,
): TSESTree.SourceLocation {
let start: TSESTree.Position;

if (node.decorators.length === 0) {
start = node.loc.start;
} else {
const lastDecorator = node.decorators[node.decorators.length - 1];
const nextToken = nullThrows(
context.sourceCode.getTokenAfter(lastDecorator),
NullThrowsReasons.MissingToken('token', 'last decorator'),
);
start = nextToken.loc.start;
}

let end: TSESTree.Position;

if (!node.computed) {
end = node.key.loc.end;
} else {
const closingBracket = nullThrows(
context.sourceCode.getTokenAfter(
node.key,
token => token.value === ']',
),
NullThrowsReasons.MissingToken(']', node.type),
);
end = closingBracket.loc.end;
}

return {
start: structuredClone(start),
end: structuredClone(end),
};
}

/**
* For missing accessibility modifiers, we want to report any keywords
* out in front of the key, and the key itself, but not anything afterwards,
* i.e. parens, type annotations, method bodies, or `?`.
*/
function getMissingAccessibilityReportLocForParameterProperty(
node: TSESTree.TSParameterProperty,
nodeName: string,
): TSESTree.SourceLocation {
// Parameter properties have a weirdly different AST structure
// than other class members.

let start: TSESTree.Position;

if (node.decorators.length === 0) {
start = structuredClone(node.loc.start);
} else {
const lastDecorator = node.decorators[node.decorators.length - 1];
const nextToken = nullThrows(
context.sourceCode.getTokenAfter(lastDecorator),
NullThrowsReasons.MissingToken('token', 'last decorator'),
);
start = structuredClone(nextToken.loc.start);
}

const end = context.sourceCode.getLocFromIndex(
node.parameter.range[0] + nodeName.length,
);

return {
start,
end,
};
}

/**
* Creates a fixer that adds an accessibility modifier keyword
*/
Expand Down Expand Up @@ -385,7 +308,7 @@ export default createRule<Options, MessageIds>({
!propertyDefinition.accessibility
) {
context.report({
loc: getMissingAccessibilityReportLoc(propertyDefinition),
loc: getMemberHeadLoc(context.sourceCode, propertyDefinition),
messageId: 'missingAccessibility',
data: {
type: nodeType,
Expand Down Expand Up @@ -422,7 +345,8 @@ export default createRule<Options, MessageIds>({
case 'explicit': {
if (!node.accessibility) {
context.report({
loc: getMissingAccessibilityReportLocForParameterProperty(
loc: getParameterPropertyHeadLoc(
context.sourceCode,
node,
nodeName,
),
Expand Down
37 changes: 27 additions & 10 deletions packages/eslint-plugin/src/rules/prefer-readonly.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ import {
nullThrows,
typeIsOrHasBaseType,
} from '../util';
import {
getMemberHeadLoc,
getParameterPropertyHeadLoc,
} from '../util/getMemberHeadLoc';

type MessageIds = 'preferReadonly';
type Options = [
Expand Down Expand Up @@ -160,15 +164,6 @@ export default createRule<Options, MessageIds>({
function getEsNodesFromViolatingNode(
violatingNode: ParameterOrPropertyDeclaration,
): { esNode: TSESTree.Node; nameNode: TSESTree.Node } {
if (
ts.isParameterPropertyDeclaration(violatingNode, violatingNode.parent)
) {
return {
esNode: services.tsNodeToESTreeNodeMap.get(violatingNode.name),
nameNode: services.tsNodeToESTreeNodeMap.get(violatingNode.name),
};
}

return {
esNode: services.tsNodeToESTreeNodeMap.get(violatingNode),
nameNode: services.tsNodeToESTreeNodeMap.get(violatingNode.name),
Expand Down Expand Up @@ -196,13 +191,35 @@ export default createRule<Options, MessageIds>({
for (const violatingNode of finalizedClassScope.finalizeUnmodifiedPrivateNonReadonlys()) {
const { esNode, nameNode } =
getEsNodesFromViolatingNode(violatingNode);

const reportNodeOrLoc:
| { node: TSESTree.Node }
| { loc: TSESTree.SourceLocation } = (() => {
switch (esNode.type) {
case AST_NODE_TYPES.MethodDefinition:
case AST_NODE_TYPES.PropertyDefinition:
case AST_NODE_TYPES.TSAbstractMethodDefinition:
return { loc: getMemberHeadLoc(context.sourceCode, esNode) };
case AST_NODE_TYPES.TSParameterProperty:
return {
loc: getParameterPropertyHeadLoc(
context.sourceCode,
esNode,
(nameNode as TSESTree.Identifier).name,
),
};
default:
return { node: esNode };
}
})();

context.report({
...reportNodeOrLoc,
data: {
name: context.sourceCode.getText(nameNode),
},
fix: fixer => fixer.insertTextBefore(nameNode, 'readonly '),
messageId: 'preferReadonly',
node: esNode,
});
}
},
Expand Down
108 changes: 108 additions & 0 deletions packages/eslint-plugin/src/util/getMemberHeadLoc.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
import type { TSESLint, TSESTree } from '@typescript-eslint/utils';
import {
nullThrows,
NullThrowsReasons,
} from '@typescript-eslint/utils/eslint-utils';

/**
* Generates report loc suitable for reporting on how a class member is
* declared, rather than how it's implemented.
*
* ```ts
* class A {
* abstract method(): void;
* ~~~~~~~~~~~~~~~
*
* concreteMethod(): void {
* ~~~~~~~~~~~~~~
* // code
* }
*
* abstract private property?: string;
* ~~~~~~~~~~~~~~~~~~~~~~~~~
*
* @decorator override concreteProperty = 'value';
* ~~~~~~~~~~~~~~~~~~~~~~~~~
* }
* ```
*/
export function getMemberHeadLoc(
sourceCode: Readonly<TSESLint.SourceCode>,
node:
| TSESTree.MethodDefinition
| TSESTree.TSAbstractMethodDefinition
| TSESTree.PropertyDefinition
| TSESTree.TSAbstractPropertyDefinition,
): TSESTree.SourceLocation {
let start: TSESTree.Position;

if (node.decorators.length === 0) {
start = node.loc.start;
} else {
const lastDecorator = node.decorators[node.decorators.length - 1];
const nextToken = nullThrows(
sourceCode.getTokenAfter(lastDecorator),
NullThrowsReasons.MissingToken('token', 'last decorator'),
);
start = nextToken.loc.start;
}

let end: TSESTree.Position;

if (!node.computed) {
end = node.key.loc.end;
} else {
const closingBracket = nullThrows(
sourceCode.getTokenAfter(node.key, token => token.value === ']'),
NullThrowsReasons.MissingToken(']', node.type),
);
end = closingBracket.loc.end;
}

return {
start: structuredClone(start),
end: structuredClone(end),
};
}

/**
* Generates report loc suitable for reporting on how a parameter property is
* declared.
*
* ```ts
* class A {
* constructor(private property: string = 'value') {
* ~~~~~~~~~~~~~~~~
* }
* ```
*/
export function getParameterPropertyHeadLoc(
sourceCode: Readonly<TSESLint.SourceCode>,
node: TSESTree.TSParameterProperty,
nodeName: string,
): TSESTree.SourceLocation {
// Parameter properties have a weirdly different AST structure
// than other class members.

let start: TSESTree.Position;

if (node.decorators.length === 0) {
start = structuredClone(node.loc.start);
} else {
const lastDecorator = node.decorators[node.decorators.length - 1];
const nextToken = nullThrows(
sourceCode.getTokenAfter(lastDecorator),
NullThrowsReasons.MissingToken('token', 'last decorator'),
);
start = structuredClone(nextToken.loc.start);
}

const end = sourceCode.getLocFromIndex(
node.parameter.range[0] + nodeName.length,
);

return {
start,
end,
};
}

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

Loading

0 comments on commit 50ed604

Please sign in to comment.