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.

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

Jump to bottom

feat(eslint-plugin): [explicit-member-accessibility] suggest adding explicit accessibility specifiers #5492

Merged
merged 12 commits into from Aug 24, 2022
Merged

feat(eslint-plugin): [explicit-member-accessibility] suggest adding explicit accessibility specifiers #5492

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
37bfd8f
feat(eslint-plugin): [explicit-member-accessibility] autofix missing …
jtbandes Aug 17, 2022
a955225
Convert auto-fix to suggestions
jtbandes Aug 18, 2022
b7a4d69
Merge branch 'main' into fix-explicit-member-accessibility
jtbandes Aug 18, 2022
0d57ef5
Merge remote-tracking branch 'upstream/main' into fix-explicit-member…
jtbandes Aug 18, 2022
5fd7ae3
remove trim()s
jtbandes Aug 18, 2022
d38da51
Update packages/eslint-plugin/src/rules/explicit-member-accessibility.ts
jtbandes Aug 18, 2022
65dd4ce
Merge remote-tracking branch 'origin/fix-explicit-member-accessibilit…
jtbandes Aug 18, 2022
4623489
merge message IDs
jtbandes Aug 18, 2022
b46d54e
!
jtbandes Aug 18, 2022
143c1aa
Merge branch 'main' into fix-explicit-member-accessibility
jtbandes Aug 22, 2022
6395b8f
Merge branch 'main' into fix-explicit-member-accessibility
jtbandes Aug 23, 2022
5471380
Merge branch 'main' into fix-explicit-member-accessibility
JoshuaKGoldberg Aug 24, 2022
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
159 changes: 105 additions & 54 deletions packages/eslint-plugin/src/rules/explicit-member-accessibility.ts
View file
Open in desktop
Expand Up @@ -25,7 +25,12 @@ interface Config {

type Options = [Config];

type MessageIds = 'unwantedPublicAccessibility' | 'missingAccessibility';
type MessageIds =
| 'unwantedPublicAccessibility'
| 'missingAccessibility'
| 'addPublicAccessibility'
| 'addPrivateAccessibility'
| 'addProtectedAccessibility';

const accessibilityLevel = {
oneOf: [
Expand All @@ -47,6 +52,7 @@ const accessibilityLevel = {
export default util.createRule<Options, MessageIds>({
name: 'explicit-member-accessibility',
meta: {
hasSuggestions: true,
type: 'problem',
docs: {
description:
Expand All @@ -58,6 +64,9 @@ export default util.createRule<Options, MessageIds>({
messages: {
missingAccessibility:
'Missing accessibility modifier on {{type}} {{name}}.',
addPublicAccessibility: "Add 'public' accessibility modifier",
addPrivateAccessibility: "Add 'private' accessibility modifier",
addProtectedAccessibility: "Add 'protected' accessibility modifier",
jtbandes marked this conversation as resolved.
Show resolved Hide resolved
unwantedPublicAccessibility:
'Public accessibility modifier on {{type}} {{name}}.',
},
Expand Down Expand Up @@ -103,26 +112,6 @@ export default util.createRule<Options, MessageIds>({
const propCheck = overrides.properties ?? baseCheck;
const paramPropCheck = overrides.parameterProperties ?? baseCheck;
const ignoredMethodNames = new Set(option.ignoredMethodNames ?? []);
/**
* Generates the report for rule violations
*/
function reportIssue(
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
messageId: MessageIds,
nodeType: string,
node: TSESTree.Node,
nodeName: string,
fix: TSESLint.ReportFixFunction | null = null,
): void {
context.report({
node,
messageId,
data: {
type: nodeType,
name: nodeName,
},
fix,
});
}

/**
* Checks if a method declaration has an accessibility modifier.
Expand Down Expand Up @@ -164,20 +153,25 @@ export default util.createRule<Options, MessageIds>({
check === 'no-public' &&
methodDefinition.accessibility === 'public'
) {
reportIssue(
'unwantedPublicAccessibility',
nodeType,
methodDefinition,
methodName,
getUnwantedPublicAccessibilityFixer(methodDefinition),
);
context.report({
node: methodDefinition,
messageId: 'unwantedPublicAccessibility',
data: {
type: nodeType,
name: methodName,
},
fix: getUnwantedPublicAccessibilityFixer(methodDefinition),
});
} else if (check === 'explicit' && !methodDefinition.accessibility) {
reportIssue(
'missingAccessibility',
nodeType,
methodDefinition,
methodName,
);
context.report({
node: methodDefinition,
messageId: 'missingAccessibility',
data: {
type: nodeType,
name: methodName,
},
suggest: getMissingAccessibilitySuggestions(methodDefinition),
});
}
}

Expand Down Expand Up @@ -223,6 +217,48 @@ export default util.createRule<Options, MessageIds>({
};
}

/**
* Creates a fixer that adds a "public" keyword with following spaces
*/
function getMissingAccessibilitySuggestions(
node:
| TSESTree.MethodDefinition
| TSESTree.PropertyDefinition
| TSESTree.TSAbstractMethodDefinition
| TSESTree.TSAbstractPropertyDefinition
| TSESTree.TSParameterProperty,
): TSESLint.ReportSuggestionArray<MessageIds> {
function fix(
accessibility: TSESTree.Accessibility,
fixer: TSESLint.RuleFixer,
): TSESLint.RuleFix | null {
if (node.decorators && node.decorators.length > 0) {
jtbandes marked this conversation as resolved.
Show resolved Hide resolved
const lastDecorator = node.decorators[node.decorators.length - 1];
const nextToken = sourceCode.getTokenAfter(lastDecorator);
if (!nextToken) {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

We generally expect most new code -and ideally all new rule code- to be covered by tests. Codecov is rightfully showing here that this isn't. One of two cases is true:

  • This case can be tested and should be
  • This case is impossible and the type system is wrong
    • Meaning: use a ! to show that the nextToken does always exist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expect it is probably impossible, because I can't think of any situation where there wouldn't be any tokens after the last decorator, but it feels wrong to simply ! the output of getTokenAfter just because I believe that (perhaps erroneously), so I was trying to code defensively. But that also means I can't think of any test cases to add test coverage here. What would you recommend doing?

Copy link
Member

Choose a reason for hiding this comment

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

I say ! it. If there are decorators they must be decorating something for this to be valid syntax.

In an ideal world, sourceCode.getTokenAfter would know that if it's given a node that must have an after, the returned token is defined. If we eventually get rich enough types the ! will be linted as unnecessary.

}
return fixer.insertTextBefore(nextToken, `${accessibility} `);
}
return fixer.insertTextBefore(node, `${accessibility} `);
}

return [
{
messageId: 'addPublicAccessibility',
fix: fixer => fix('public', fixer),
},
{
messageId: 'addPrivateAccessibility',
fix: fixer => fix('private', fixer),
},
{
messageId: 'addProtectedAccessibility',
fix: fixer => fix('protected', fixer),
},
];
}

/**
* Checks if property has an accessibility modifier.
* @param propertyDefinition The node representing a PropertyDefinition.
Expand All @@ -246,23 +282,28 @@ export default util.createRule<Options, MessageIds>({
propCheck === 'no-public' &&
propertyDefinition.accessibility === 'public'
) {
reportIssue(
'unwantedPublicAccessibility',
nodeType,
propertyDefinition,
propertyName,
getUnwantedPublicAccessibilityFixer(propertyDefinition),
);
context.report({
node: propertyDefinition,
messageId: 'unwantedPublicAccessibility',
data: {
type: nodeType,
name: propertyName,
},
fix: getUnwantedPublicAccessibilityFixer(propertyDefinition),
});
} else if (
propCheck === 'explicit' &&
!propertyDefinition.accessibility
) {
reportIssue(
'missingAccessibility',
nodeType,
propertyDefinition,
propertyName,
);
context.report({
node: propertyDefinition,
messageId: 'missingAccessibility',
data: {
type: nodeType,
name: propertyName,
},
suggest: getMissingAccessibilitySuggestions(propertyDefinition),
});
}
}

Expand Down Expand Up @@ -291,19 +332,29 @@ export default util.createRule<Options, MessageIds>({
switch (paramPropCheck) {
case 'explicit': {
if (!node.accessibility) {
reportIssue('missingAccessibility', nodeType, node, nodeName);
context.report({
node,
messageId: 'missingAccessibility',
data: {
type: nodeType,
name: nodeName,
},
suggest: getMissingAccessibilitySuggestions(node),
});
}
break;
}
case 'no-public': {
if (node.accessibility === 'public' && node.readonly) {
reportIssue(
'unwantedPublicAccessibility',
nodeType,
context.report({
node,
nodeName,
getUnwantedPublicAccessibilityFixer(node),
);
messageId: 'unwantedPublicAccessibility',
data: {
type: nodeType,
name: nodeName,
},
fix: getUnwantedPublicAccessibilityFixer(node),
});
}
break;
}
Expand Down