-
Notifications
You must be signed in to change notification settings - Fork 9
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
OliverJAsh
wants to merge
3
commits into
buildo:main
Choose a base branch
from
OliverJAsh:oja/no-redundant-pipe
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) | ||
), | ||
]; | ||
}, | ||
}, | ||
], | ||
}); | ||
}) | ||
); | ||
}, | ||
}; | ||
}, | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
output: ` | ||
import { pipe } from "fp-ts/function" | ||
value; | ||
`, | ||
}, | ||
], | ||
}, | ||
], | ||
}, | ||
], | ||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.