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

Add no-redundant-pipe #225

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
113 changes: 113 additions & 0 deletions src/rules/no-redundant-pipe.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
import * as NonEmptyArray from "fp-ts/NonEmptyArray";
import { AST_NODE_TYPES } from "@typescript-eslint/experimental-utils";
import { flow, pipe } from "fp-ts/function";
import * as O from "fp-ts/Option";
import {
contextUtils,
createRule,
createSequenceExpressionFromCallExpressionWithExpressionArgs,
getCallExpressionWithExpressionArgs,
prettyPrint,
CallExpressionWithExpressionArgs,
} from "../utils";

const errorMessages = {
redundantPipeWithSingleArg:
"pipe can be removed because it takes only one argument",
redundantPipeWithSingleArgInsidePipe:
"pipe can be removed because it is used as the first argument inside another pipe",
};

export default createRule({
name: "no-redundant-pipe",
meta: {
type: "suggestion",
fixable: "code",
hasSuggestions: true,
schema: [],
docs: {
description: "Remove redundant uses of pipe",
recommended: "warn",
},
messages: {
...errorMessages,
removePipe: "remove pipe",
},
},
defaultOptions: [],
create(context) {
const { isPipeExpression } = contextUtils(context);

const getPipeCallExpressionWithExpressionArgs = flow(
O.fromPredicate(isPipeExpression),
/**
* We ignore pipe calls which contain a spread argument because these are never invalid.
*/
O.chain(getCallExpressionWithExpressionArgs)
);

type RedundantPipeCallAndMessage = {
redundantPipeCall: CallExpressionWithExpressionArgs;
errorMessageId: keyof typeof errorMessages;
};

const getRedundantPipeCall = (
pipeCall: CallExpressionWithExpressionArgs
): O.Option<RedundantPipeCallAndMessage> => {
const firstArg = pipe(pipeCall.args, NonEmptyArray.head);

if (pipeCall.args.length === 1) {
const result: RedundantPipeCallAndMessage = {
redundantPipeCall: pipeCall,
errorMessageId: "redundantPipeWithSingleArg",
};
return O.some(result);
} else if (firstArg.type === AST_NODE_TYPES.CallExpression) {
return pipe(
getPipeCallExpressionWithExpressionArgs(firstArg),
O.map(
(redundantPipeCall): RedundantPipeCallAndMessage => ({
redundantPipeCall,
errorMessageId: "redundantPipeWithSingleArgInsidePipe",
})
)
);
} else {
return O.none;
}
};

return {
CallExpression(node) {
pipe(
node,
getPipeCallExpressionWithExpressionArgs,
O.chain(getRedundantPipeCall),
O.map(({ redundantPipeCall, errorMessageId }) => {
context.report({
node: redundantPipeCall.node,
messageId: errorMessageId,
suggest: [
{
messageId: "removePipe",
fix(fixer) {
const sequenceExpression =
createSequenceExpressionFromCallExpressionWithExpressionArgs(
redundantPipeCall
);
return [
fixer.replaceText(
redundantPipeCall.node,
prettyPrint(sequenceExpression)
),
];
},
},
],
});
})
);
},
};
},
});
15 changes: 14 additions & 1 deletion src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,18 @@ export const contextUtils = <
);
}

function isPipeExpression(node: TSESTree.CallExpression): boolean {
return pipe(
node,
calleeIdentifier,
option.exists(
(callee) =>
callee.name === "pipe" &&
isIdentifierImportedFrom(callee, /fp-ts\//, context)
)
);
}

function isPipeOrFlowExpression(node: TSESTree.CallExpression): boolean {
return pipe(
node,
Expand Down Expand Up @@ -464,6 +476,7 @@ export const contextUtils = <
return {
addNamedImportIfNeeded,
removeImportDeclaration,
isPipeExpression,
isFlowExpression,
isPipeOrFlowExpression,
isIdentifierImportedFrom,
Expand All @@ -488,7 +501,7 @@ const getArgumentExpression = (

const checkIsArgumentExpression = O.getRefinement(getArgumentExpression);

type CallExpressionWithExpressionArgs = {
export type CallExpressionWithExpressionArgs = {
node: TSESTree.CallExpression;
args: NonEmptyArray.NonEmptyArray<TSESTree.Expression>;
};
Expand Down
169 changes: 169 additions & 0 deletions tests/rules/no-redundant-pipe.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
import rule from "../../src/rules/no-redundant-pipe";
import { ESLintUtils } from "@typescript-eslint/experimental-utils";

const ruleTester = new ESLintUtils.RuleTester({
parser: "@typescript-eslint/parser",
parserOptions: {
sourceType: "module",
},
});

ruleTester.run("no-redundant-pipe", rule, {
valid: [
`import { pipe } from "fp-ts/function"
pipe(...x);
`,
`import { pipe } from "fp-ts/function"
pipe(value, fn);
`,
`import { pipe } from "fp-ts/function"
pipe(value, pipe(value2, fn));
`,
],
invalid: [
{
code: `
import { pipe } from "fp-ts/function"
pipe(value);
`,
errors: [
{
messageId: "redundantPipeWithSingleArg",
suggestions: [
{
messageId: "removePipe",
output: `
import { pipe } from "fp-ts/function"
value;
`,
},
],
},
],
},
{
code: `
import { pipe } from "fp-ts/function"
pipe(pipe(pipe(value, fn1), fn2), fn3);
`,
errors: [
{
messageId: "redundantPipeWithSingleArgInsidePipe",
suggestions: [
{
messageId: "removePipe",
output: `
import { pipe } from "fp-ts/function"
pipe(pipe(value, fn1), fn2, fn3);
`,
},
],
},
{
messageId: "redundantPipeWithSingleArgInsidePipe",
suggestions: [
{
messageId: "removePipe",
output: `
import { pipe } from "fp-ts/function"
pipe(pipe(value, fn1, fn2), fn3);
`,
},
],
},
],
},
{
code: `
import { pipe } from "fp-ts/function"
pipe(pipe(value, fn1), fn2, fn3);
`,
errors: [
{
messageId: "redundantPipeWithSingleArgInsidePipe",
suggestions: [
{
messageId: "removePipe",
output: `
import { pipe } from "fp-ts/function"
pipe(value, fn1, fn2, fn3);
`,
},
],
},
],
},
{
code: `
import { pipe } from "fp-ts/function"
pipe(
// foo
pipe(value, fn1),
fn2
);`,
errors: [
{
messageId: "redundantPipeWithSingleArgInsidePipe",
suggestions: [
{
messageId: "removePipe",
output: `
import { pipe } from "fp-ts/function"
pipe(
// foo
value, fn1,
fn2
);`,
},
],
},
],
},
{
code: `
import { pipe } from "fp-ts/function"
pipe(
value,
);
`,
errors: [
{
messageId: "redundantPipeWithSingleArg",
suggestions: [
{
messageId: "removePipe",
output: `
import { pipe } from "fp-ts/function"
value;
`,
},
],
},
],
},
{
code: `
import { pipe } from "fp-ts/function"
pipe(
// foo
value,
);
`,
errors: [
{
messageId: "redundantPipeWithSingleArg",
suggestions: [
{
messageId: "removePipe",
// TODO: ideally we would preserve the comment here but I'm not sure how
Copy link
Member

@gabro gabro Dec 14, 2021

Choose a reason for hiding this comment

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

If I'm reading this correctly, this is due to the fact that we're synthesizing a new AST node. It's hard to preserve trivias (comments, whitespaces, etc) when manipulating the AST because it's a lossy representation of the source code.
This is why I tend to lean towards reading AST but writing tokens (aka characters). It's harder to work with, but it's less invasive for the final user.

Do you think this fix could be implemented without generating a new AST node? Accidentally removing comments is a show-stopper in my opinion, I think a fix that can produce invalid syntax in some edge cases is still preferable to a silent removal of a comment

Copy link
Contributor Author

@OliverJAsh OliverJAsh Dec 14, 2021

Choose a reason for hiding this comment

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

Thanks for bringing this up! I agree it's not good if we accidentally remove a comment. To be clear, this is only an issue for comments inside the pipe—comments outside are preserved.

Do you think this fix could be implemented without generating a new AST node?

Yes, I think it would be the same as the fix we used to have for no-redundant-flow before b76044a which was to fix #222. Maybe there is another way to fix #222?

That said, in my opinion it's extremely error prone to manipulate code as text rather than as an AST—although this unfortunately seems to be the status quo for ESLint rules, as far as I can tell. As far as I understand, there is no information about comments in TSESTree's representation of the AST. However, there is some information about comments in TypeScript's AST. I wonder if we could implement the fix using TypeScript's AST instead, e.g. by mapping the TSESTree node to a TS node, constructing a new node and then using TypeScript's AST printer. Do you have any experience doing this inside of ESLint rules?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have direct experience with Typescript AST, no, but if it supports not blowing away the comments maybe it's worth a try.

Otherwise we can think of a more ad-hoc fix for the specific use case

Copy link
Contributor Author

@OliverJAsh OliverJAsh Dec 14, 2021

Choose a reason for hiding this comment

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

I think the ad-hoc fix would be to revert b76044a and then find another way to workaround #222, but I'm not sure how else we can workaround this? If the trailing comma is there, we need to adjust the fix to remove that as well.

The TSESTree AST doesn't seem to have any information about whether a Node has a trailing comma, but maybe we can read this from the TS AST instead.

output: `
import { pipe } from "fp-ts/function"
value;
`,
},
],
},
],
},
],
});