Skip to content

fix(eslint-plugin): [explicit-member-accessibility] refine report locations #8869

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 7 commits into from
May 28, 2024
Merged

fix(eslint-plugin): [explicit-member-accessibility] refine report locations #8869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9d3c8af
[explicit-member-accessibility] Refine report locations.
kirkwaiblinger Apr 6, 2024
82d5a2e
fix snapshot
kirkwaiblinger Apr 9, 2024
ada6422
fix types
kirkwaiblinger Apr 9, 2024
89717bd
computed names
kirkwaiblinger Apr 10, 2024
f2de27a
simplify slightly
kirkwaiblinger Apr 10, 2024
40d4a8e
Merge branch 'main' into explicit-member-accessibility-report-loc
kirkwaiblinger May 3, 2024
602de2a
missing output
kirkwaiblinger May 3, 2024
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
181 changes: 141 additions & 40 deletions packages/eslint-plugin/src/rules/explicit-member-accessibility.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,9 @@ export default createRule<Options, MessageIds>({
* @param methodDefinition The node representing a MethodDefinition.
*/
function checkMethodAccessibilityModifier(
methodDefinition: TSESTree.MethodDefinition,
methodDefinition:
| TSESTree.MethodDefinition
| TSESTree.TSAbstractMethodDefinition,
): void {
if (methodDefinition.key.type === AST_NODE_TYPES.PrivateIdentifier) {
return;
Expand Down Expand Up @@ -152,18 +154,19 @@ export default createRule<Options, MessageIds>({
check === 'no-public' &&
methodDefinition.accessibility === 'public'
) {
const publicKeyword = findPublicKeyword(methodDefinition);
context.report({
node: methodDefinition,
loc: rangeToLoc(context.sourceCode, publicKeyword.range),
messageId: 'unwantedPublicAccessibility',
data: {
type: nodeType,
name: methodName,
},
fix: getUnwantedPublicAccessibilityFixer(methodDefinition),
fix: fixer => fixer.removeRange(publicKeyword.rangeToRemove),
});
} else if (check === 'explicit' && !methodDefinition.accessibility) {
context.report({
node: methodDefinition,
loc: getMissingAccessibilityReportLoc(methodDefinition),
messageId: 'missingAccessibility',
data: {
type: nodeType,
Expand All @@ -175,49 +178,132 @@ export default createRule<Options, MessageIds>({
}

/**
* Creates a fixer that removes a "public" keyword with following spaces
* Returns an object containing a range that corresponds to the "public"
* keyword for a node, and the range that would need to be removed to
* remove the "public" keyword (including associated whitespace).
*/
function getUnwantedPublicAccessibilityFixer(
function findPublicKeyword(
node:
| TSESTree.MethodDefinition
| TSESTree.PropertyDefinition
| TSESTree.TSAbstractMethodDefinition
| TSESTree.TSAbstractPropertyDefinition
| TSESTree.TSParameterProperty,
): TSESLint.ReportFixFunction {
return function (fixer: TSESLint.RuleFixer): TSESLint.RuleFix {
const tokens = context.sourceCode.getTokens(node);
let rangeToRemove!: TSESLint.AST.Range;
for (let i = 0; i < tokens.length; i++) {
const token = tokens[i];
if (
token.type === AST_TOKEN_TYPES.Keyword &&
token.value === 'public'
) {
const commensAfterPublicKeyword =
context.sourceCode.getCommentsAfter(token);
if (commensAfterPublicKeyword.length) {
// public /* Hi there! */ static foo()
// ^^^^^^^
rangeToRemove = [
token.range[0],
commensAfterPublicKeyword[0].range[0],
];
break;
} else {
// public static foo()
// ^^^^^^^
rangeToRemove = [token.range[0], tokens[i + 1].range[0]];
break;
}
): { range: TSESLint.AST.Range; rangeToRemove: TSESLint.AST.Range } {
const tokens = context.sourceCode.getTokens(node);
let rangeToRemove!: TSESLint.AST.Range;
let keywordRange!: TSESLint.AST.Range;
for (let i = 0; i < tokens.length; i++) {
const token = tokens[i];
if (
token.type === AST_TOKEN_TYPES.Keyword &&
token.value === 'public'
) {
keywordRange = structuredClone(token.range);
const commensAfterPublicKeyword =
context.sourceCode.getCommentsAfter(token);
if (commensAfterPublicKeyword.length) {
// public /* Hi there! */ static foo()
// ^^^^^^^
rangeToRemove = [
token.range[0],
commensAfterPublicKeyword[0].range[0],
];
break;
} else {
// public static foo()
// ^^^^^^^
rangeToRemove = [token.range[0], tokens[i + 1].range[0]];
break;
}
}
return fixer.removeRange(rangeToRemove);
}
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 a "public" keyword with following spaces
* Creates a fixer that adds an accessibility modifier keyword
*/
function getMissingAccessibilitySuggestions(
node:
Expand Down Expand Up @@ -284,21 +370,22 @@ export default createRule<Options, MessageIds>({
propCheck === 'no-public' &&
propertyDefinition.accessibility === 'public'
) {
const publicKeywordRange = findPublicKeyword(propertyDefinition);
context.report({
node: propertyDefinition,
loc: rangeToLoc(context.sourceCode, publicKeywordRange.range),
messageId: 'unwantedPublicAccessibility',
data: {
type: nodeType,
name: propertyName,
},
fix: getUnwantedPublicAccessibilityFixer(propertyDefinition),
fix: fixer => fixer.removeRange(publicKeywordRange.rangeToRemove),
});
} else if (
propCheck === 'explicit' &&
!propertyDefinition.accessibility
) {
context.report({
node: propertyDefinition,
loc: getMissingAccessibilityReportLoc(propertyDefinition),
messageId: 'missingAccessibility',
data: {
type: nodeType,
Expand Down Expand Up @@ -335,7 +422,10 @@ export default createRule<Options, MessageIds>({
case 'explicit': {
if (!node.accessibility) {
context.report({
node,
loc: getMissingAccessibilityReportLocForParameterProperty(
node,
nodeName,
),
messageId: 'missingAccessibility',
data: {
type: nodeType,
Expand All @@ -348,14 +438,15 @@ export default createRule<Options, MessageIds>({
}
case 'no-public': {
if (node.accessibility === 'public' && node.readonly) {
const publicKeyword = findPublicKeyword(node);
context.report({
node,
loc: rangeToLoc(context.sourceCode, publicKeyword.range),
messageId: 'unwantedPublicAccessibility',
data: {
type: nodeType,
name: nodeName,
},
fix: getUnwantedPublicAccessibilityFixer(node),
fix: fixer => fixer.removeRange(publicKeyword.rangeToRemove),
});
}
break;
Expand All @@ -372,3 +463,13 @@ export default createRule<Options, MessageIds>({
};
},
});

function rangeToLoc(
sourceCode: TSESLint.SourceCode,
range: TSESLint.AST.Range,
): TSESTree.SourceLocation {
return {
start: sourceCode.getLocFromIndex(range[0]),
end: sourceCode.getLocFromIndex(range[1]),
};
}
Loading
Loading