Integration with Putout code transformer

coderaiser commented 3 years ago

You can integrate easily with putout code transformer, and asyncify can be used as a plugin, similar to async-await-codemod.

A little change is required so code written in the way that jscodeshift suggests.

jedwards1211 commented 3 years ago

There was some techincal reason I called recast directly in this weird way, because normally I would use jscodeshift in the normal way, but I forget what wasn't working. I'll look into it. How come it wouldn't work as-is with putout though? If running with stock jscodeshift, a transform can process a file in any way it wants, it just has to return a string...

jedwards1211 commented 3 years ago

Oh now i remember. When I wrote this there were too many bugs in the version of recast that jscodeshift uses, and it lags behind the latest version, so I used a later version directly in this package. I don't want users running with an old version of jscodeshift to think my transform is buggy. This is the kind of nasty situation that results from neither package using semver (they are both 0.x.x)

coderaiser commented 3 years ago

How come it wouldn't work as-is with putout though?

Putout uses dependency injection provided by jscodeshift to provide parsed AST and to determine that code was changed.

jedwards1211 commented 3 years ago

But if the transform parses the code on its own it returns a string just like any other jscodeshift transform, can putout not use that returned string? If not, then putout is not technically 100% supporting all jscodeshift transforms.

I would need a system to detect the version of jscodeshift that's injected and error out if it's too old so that users don't think codemod glitches are due to this package itself.

jedwards1211 commented 3 years ago

Also transforms can completely sidestep jscodeshift's parser dependency injection by doing

exports.parser = { parse: (code) => ... }

Which I guess would be another way I could make it use the version of recast I want...

coderaiser commented 3 years ago

But if the transform parses the code on its own it returns a string just like any other jscodeshift transform, can putout not use that returned string?

Putout injects parsed AST to avoid duplicate parsing because it slow.

If not, then putout is not technically 100% supporting all jscodeshift transforms.

There is no such target, but the cases written in jscodeshift documentation is supported. What I want to suggest you is using putout to make transformation, it's much simpler then using jscodeshift API, or raw babel API. There is simplified declarative transform syntax described in @putout/compare and in @putout/engine-runner. So you can do more transformation with less code:

module.exports.replace = () => ({
    '__a(__args).then(__b)': '__b(await __a(__args))'
})

It will transform:

fetch('/api.json').then(parse);

to:

parse(await fetch('/api.json'));

jedwards1211 commented 3 years ago

I've been working on my own tool called astx for similar use cases, which allows mixing structural search and replace with arbitrary code to manipulate AST nodes, for maximum flexibility. I still need to finish publishing the runner.

When it comes to transforming promises to async/await though, asyncify does vastly more in-depth transformations than is possible with only structural search and replace.

I understand duplicate parsing would be slow, but if I do it the way you suggest, how can I guarantee that the code is parsed with a recent-enough version of recast (and if I don't use recast, how can I guarantee that unchanged portions of the AST don't get reformatted, if the user doesn't have any code formatter setup)? Converting promises to async/await is going to be a one-time operation for most codebases. So it makes more sense to guarantee correctness than to guarantee speed.

coderaiser commented 3 years ago

I've been working on my own tool called astx for similar use cases, which allows mixing structural search and replace with arbitrary code to manipulate AST nodes, for maximum flexibility. I still need to finish publishing the runner.

It looks very good :). Very laconic syntax, the only thing is tests count very small. Putout has more then 2500 tests, and a lot of built-in declarative plug-ins.

When it comes to transforming promises to async/await though, asyncify does vastly more in-depth transformations than is possible with only structural search and replace.

You absolutely right, and putout can handle it as well using declarative search and imperative transform.

how can I guarantee that the code is parsed with a recent-enough version of recast

I think mention supported version of jscodeshift in documentation will be enough.

and if I don't use recast, how can I guarantee that unchanged portions of the AST don't get reformatted, if the user doesn't have any code formatter setup)?

Every JavaScript project must use formatted setup, and most folks are using (even) type check :).

Converting promises to async/await is going to be a one-time operation for most codebases

What if new developer, that just joined team didn’t know all the rules related to promises in project. He writes code, and then his IDE helps him to fix it with autofix using putout integration with eslint. I think it's very useful :).

jedwards1211 commented 3 years ago

I think mention supported version of jscodeshift in documentation will be enough.

I guess but that raises the likelihood that I have to deal with issues raised by someone who didn't read that...

Every JavaScript project must use formatted setup, and most folks are using (even) type check :).

We may think it's foolish not to, but no JS project has to use a formatter. It's nice to be able to not change the format of code that a codemod doesn't touch, so that it doesn't cause problems for people who aren't using formatters. Using recast enables me to do this (and that's why stock jscodeshift uses recast)

tests count very small

I don't know if you saw that most of my tests are organized as fixture files, test coverage isn't as high as yours but here's the coverage

-------------------|---------|----------|---------|---------|-------------------
File               | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-------------------|---------|----------|---------|---------|-------------------
All files          |    89.7 |    78.82 |    88.1 |   89.72 |                   
 src               |   85.17 |    75.84 |   78.72 |   84.43 |                   
  Astx.ts          |      50 |    46.43 |      50 |      50 | ...48,169,203-236 
  find.ts          |      97 |    83.02 |     100 |   96.59 | 115,157,233       
  replace.ts       |   92.86 |    82.35 |   91.67 |   93.24 | 25-30,102-103     
 ...compileMatcher |   95.06 |     85.4 |   95.08 |   95.55 |                   
  ...eanLiteral.ts |       0 |        0 |       0 |       0 | 8-14              
  Capture.ts       |   95.24 |    92.86 |      80 |   95.24 | 18                
  ...Implements.ts |     100 |    66.67 |     100 |     100 | 9-10              
  ClassProperty.ts |   83.33 |       70 |     100 |      80 | 22                
  ...nStatement.ts |      80 |       50 |     100 |      75 | 15                
  ...nTypeParam.ts |   66.67 |    64.29 |     100 |      70 | 11-15,30          
  ...rayMatcher.ts |   94.29 |    88.37 |     100 |    93.1 | 105-106,132-133   
  ...odeMatcher.ts |     100 |     96.3 |     100 |     100 | 104               
  ...Annotation.ts |     100 |    83.33 |     100 |     100 | 9                 
  Identifier.ts    |     100 |      100 |     100 |     100 |                   
  ...tSpecifier.ts |     100 |       75 |     100 |     100 | 9-10              
  JSXAttribute.ts  |     100 |    66.67 |     100 |     100 | 9,15              
  ...nContainer.ts |      80 |       50 |     100 |      75 | 15                
  JSXIdentifier.ts |     100 |      100 |     100 |     100 |                   
  Literal.ts       |   92.31 |    90.91 |     100 |     100 | 23                
  ...ricLiteral.ts |     100 |      100 |     100 |     100 |                   
  ...Expression.ts |   96.15 |       92 |     100 |   98.63 | 85                
  ...ctProperty.ts |     100 |      100 |     100 |     100 |                   
  ...peProperty.ts |     100 |    83.33 |     100 |     100 | 9,22              
  RegExpLiteral.ts |     100 |      100 |     100 |     100 |                   
  StringLiteral.ts |     100 |      100 |     100 |     100 |                   
  ...eArguments.ts |     100 |    66.67 |     100 |     100 | 9-10              
  ...ySignature.ts |     100 |       80 |     100 |     100 | 9,20              
  ...eParameter.ts |     100 |     87.5 |     100 |     100 | 9                 
  ...eReference.ts |     100 |    83.33 |     100 |     100 | 9                 
  TypeParameter.ts |     100 |    83.33 |     100 |     100 | 9                 
  ...Declarator.ts |     100 |      100 |     100 |     100 |                   
  indentDebug.ts   |     100 |       50 |     100 |     100 | 6                 
  index.ts         |     100 |    90.63 |     100 |     100 | 125-128           
  sortFlags.ts     |     100 |      100 |     100 |     100 |                   
 src/util          |   78.41 |    51.67 |   88.89 |   79.52 |                   
  areASTsEqual.ts  |   82.35 |    72.22 |     100 |   92.86 | 10                
  getFieldNames.ts |     100 |      100 |     100 |     100 |                   
  ...dOrReplace.ts |     100 |      100 |     100 |     100 |                   
  template.ts      |   69.81 |    29.41 |   83.33 |   69.23 | 15-19,47-70,73    
-------------------|---------|----------|---------|---------|-------------------

Does putout have a placeholder syntaxes for nodes that aren't expressions like ObjectProperty etc? Does it support reshuffling properties like the following?

exports.find = `{ $_before, foo: 1, $_after }`
exports.replace = `{ $_before, foo: 1, rest: { $_after } }`

What if new developer, that just joined team didn’t know all the rules related to promises in project.

Sure, I'm just saying, I really don't think people will be running asyncify often enough that we should prioritize speed over correctness. We're not going to agree on those priorities it seems

jedwards1211 commented 3 years ago

Now that i think about it though, we could do a compromise solution: I could add another entry module that parses using the jscodeshift api, so you can call that one

jedwards1211 commented 3 years ago

The eslint folks were interested in working on an async-await autofix at one point too, but asyncify is invasive enough that I think it should be used with caution, rather than an always-autofix manner

jedwards1211 commented 3 years ago

Plus, it can still mess up comments on any version of recast. So dangerous to use automatically all the time

coderaiser commented 3 years ago

Does putout have a placeholder syntaxes for nodes that aren't expressions like ObjectProperty etc? Does it support reshuffling properties like the following?

exports.find = `{ $_before, foo: 1, $_after }`
exports.replace = `{ $_before, foo: 1, rest: { $_after } }`

It will be something like this:

That's right, putout has IDE to write plugins fast :).

Plus, it can still mess up comments on any version of recast. So dangerous to use automatically all the time

Putout preserves comments, also testing of plugins is very simple.

We may think it's foolish not to, but no JS project has to use a formatter. It's nice to be able to not change the format of code that a codemod doesn't touch, so that it doesn't cause problems for people who aren't using formatters

Look, recast always make your code a mess, I have a lot fixtures, where code after recast looks really weird. And that's OK, because ESLint can make everything shine :). Here is one of examples of raw recast transform:

before -> after

This is after calling path.remove() on unused variables.

The eslint folks were interested in working on an async-await autofix at one point too, but asyncify is invasive enough that I think it should be used with caution, rather than an always-autofix manner

That's right, the more cases we will receive, the better quality of a rule we can make. Putout much better than jscodeshift in a lot ways: it has progress-bar, config, integration with ESLint and ability to work with plugins in a simple way: develop in IDE, test using @putout/test, and publish as plugin to putout with a name: putout-plugin-asyncify. Everything to make developing of hight quality code mods simple and pleasant :).

jedwards1211 commented 3 years ago

Look, recast always make your code a mess, I have a lot fixtures, where code after recast looks really weird. And that's OK, because ESLint can make everything shine :).

Yeah its output can be goofy sometimes. But the main codebase I made asyncify for, sequelize, doesn't use a formatter, and I had to make sure my PRs didn't have irrelevant changes in the diffs. Recast was the only thing I know of that allowed me to do this. So I'm not going to reformat users' entire files by default. I have different requirements than you, it's great that you're showing me what putout has to offer but I don't want you to tell me what you think I need.

That's cool that you made a putout IDE with AST Explorer. If putout can do enough things that astx can do I might switch to it. Can it match a series of statements, or just single statements and expressions? Can replace functions access the captured nodes and paths, or can they only be used in replacement strings?

jedwards1211 commented 3 years ago

One of the most involved refactorings I did was

exports.astx = ({ astx, statements, j }) => {
  astx.findStatements`
    const code = $code
    const root = j(code)
    const result = addImports(
      root,
      $imports
    )
    expect(result).to.deep.equal($expectedReturn)
    expect(root.toSource()).to.equal($expectedCode)
  `.replace(({ captures: { $imports } }) => {
    if ($imports.quasi)
      return statements`
        testCase({
          code: $code,
          add: ${j.stringLiteral($imports.quasi.quasis[0].value.raw)},
          expectedCode: $expectedCode,
          expectedReturn: $expectedReturn,
        })
      `
  })
}

(It sounds like you have a convenient syntax for capturing quasis inside template string literals which is great, but I can't quite tell from the readme how that works)

Which made a bunch of changes like

-    it(`adds missing non-default requires without alias`, function() {
-      const code = `const {bar} = require('baz')`
-      const root = j(code)
-      const result = addImports(
-        root,
-        statement`const {foo: qux} = require('baz')`
-      )
-      expect(result).to.deep.equal({ qux: 'qux' })
-      expect(root.toSource()).to.equal(`const {
-  bar,
-  foo: qux
-} = require('baz')`)
+    it(`adds missing non-default requires without alias`, function () {
+      testCase({
+        code: `const {bar} = require('baz')`,
+        add: "const {foo: qux} = require('baz')",
+        expectedCode: `const {
+    bar,
+    foo: qux
+  } = require('baz')`,
+        expectedReturn: { qux: 'qux' },
+      })
     })

jedwards1211 commented 3 years ago

It's also worth noting that asyncify really wasn't intended to work like a linting rule, but rather as an aid to human refactoring. For one thing, it sometimes has to rename variables, and it can't pick a semantically good renaming like a human can; it has to munge them. (IIRC async-await-codemod doesn't rename variables, meaning it can break behavior) Here was a real example from sequelize tests:

It was okay to leave user0 in these tests. But in many cases it will be best for a dev to check through the output and choose better variable names.

Also, sometimes it's a matter of personal preference whether to use async/await or a promise chain, especially inside Promise.all, and no automated transform that always runs on a codebase would be able to satisfy everyone. For example, if I were refactoring by hand, I probably would have left this .catch call as-is rather than converting to the clunky (async () => { ... })(). (Note that asyncify did leave the delay(100).then() chain as-is because it's short. But it decided the .catch chain was too long, so it's hard to make a perfect rule)

jedwards1211 commented 3 years ago

Okay I added an entrypoint that you can use. Or you can use the index.js entrypoint and pass the noRecastWorkaround: true option:

https://github.com/codemodsquad/asyncify/blob/master/README.md#disabling-recast-workaround

jedwards1211 commented 3 years ago

I'd still discourage users from using this in a routine, eslint autofix kind of way, for reasons described above. But you and some eslint contributors seem to think that would be worthwhile, I just want to make sure it's clear that there are risks to it.

coderaiser commented 3 years ago

One of the most involved refactorings

For putout it will look this way:

const {operator, template} = require('putout');
const {compare, getTemplateValues} = operator;

const patterns = [
    'const code = __a',
    'const root = j(code)',
    'const result = addImports(root, __b)',
    'expect(result).to.deep.equal(__c)',
    'expect(root.toSource()).to.equal(__d)',
];

const buildTestCase = template(`
    testCase({
        code: CODE,
        add: ADD,
        expectedReturn: EXPECTED_RETURN,
        expectedCode: EXPECTED_CODE,
    })
`);

const getValue = (body, position) => {
    const currentNode = body[position];
    return getTemplateValues(currentNode.expression || currentNode, patterns[position]);
};

module.exports.match = () => ({
    'it(__a, __b)': ({__b}, path) => {
        const {body} = __b.body;

        for (let i = 0; i < patterns.length; i++) {
            const pattern = patterns[i];
            const currentNode = body[i];

            if (!compare(currentNode, pattern))
                return false;
        }

        return true;
    }
});

const CODE_POSITION = 0;
const ADD_POSITION = 2;
const EXPECTED_RETURN_POSITION = 3;
const EXPECTED_CODE_POSITION = 4;

module.exports.replace = () => ({
    'it(__a, __b)': ({__b}, path) => {
        const {body} = __b.body;
        const {__a: CODE} = getValue(body, CODE_POSITION);
        const {__b: ADD} = getValue(body, ADD_POSITION);
        const {__c: EXPECTED_CODE} = getValue(body, EXPECTED_RETURN_POSITION);
        const {__d: EXPECTED_RETURN} = getValue(body, EXPECTED_CODE_POSITION);

        const ast = buildTestCase({
            CODE,
            ADD,
            EXPECTED_CODE,
            EXPECTED_RETURN,
        });

        __b.body.body = [
            ast,
        ];

        return path;
    }
});

Little bit more code, but most of the time I'm working with short patterns, 5 lines of code with absolutely same name and meaning it's a rare case, anyways, it is possible :).

or one thing, it sometimes has to rename variables, and it can't pick a semantically good renaming like a human can; it has to munge them.

That's what I'm telling about, you got a lot expertise in converting chaining to async, and case with auto-renaming can be handling during coding review.

jedwards1211 commented 3 years ago

I see, yeah putout seems to have a great API for doing a bunch of codemods en masse. My main goal with astx is to do project-specific refactoring as easily as possible (in my example there were dozens of places where those statements got repeated in my tests that I wanted to refactor), though I've tried to make the API usable for writing general purpose codemods as well.

To that end, the astx cli prints a diff of what changes it will make and asks if you want to apply the changes.

Ideally, some day I'll figure out how to make a VSCode extension that provides a frontend UI for astx that's similar to the builtin find/replace UIs.

coderaiser commented 3 years ago

I see that astx very powerful, would be amazing to use it in putout, the couple downsides is:

how to install it, looks like it is not published to npm
I think that using jscodeshift makes things slower (need benchmarks, but raw babel must be much faster)
compatibility with current format of templates, I'm using names like __a, anyways long identifier names is also good :) I need a function compare which I could reuse over codebase:

compare('const a = `hello`', 'const __a = `__b`');
compare(node, 'const __a = `__b`');

It seems to me that if we combine efforts, we can get a universal tool for working with AST that will significantly outperform existing solutions.

What I'm thinking most of the time is improving and simplifying ecosystem of plugins. Current situation with codemods looks like a mess, you need to download some file, to run it with external tools without knowing the progress, would be much better to use npm for this purpose and consolidate the result on a permanent basis, so there will never be a problem of broken backward compatibility like I made for file manager for the web in @putout/plugin-cloudcmd.

Here is similar project for Go but the way: ruleguard.

Current implementation of compare looks this way. So it's like deep equal but more for AST nodes. What can you suggest to make this part more extensible. How do you compare nodes in astx?

coderaiser commented 3 years ago

Here is simpler solution :)

const from = `
    it(__a, function() {
        const code = __b;
        const root = j(code);
        const result = addImports(root, __c);
        expect(result).to.deep.equal(__d);
        expect(root.toSource()).to.equal(__e);
    });
`;

const to = `
    it(__a, function() {
        testCase({
        code: __b,
        add: __c,
        expectedReturn: __d,
        expectedCode: __e,
    });
})
`;

module.exports.replace = () => ({
    [from]: to,
})

jedwards1211 commented 3 years ago

Ah yes. It's kind of like how it's difficult to match just an object property, syntactically you have to write the object around it. Though I've been thinking of making a special $ObjectProperty(Key, Value) syntax for this. There was a time recently when I needed to generate import statements from a bunch of key-value pairs.

jedwards1211 commented 3 years ago

By the way, since this package mostly uses the babel API instead of ast-types, I could try to make an entry point that doesn't use recast at all if you're interested. It might have less comment issues, because Recast uses the comments field instead of leadingComments etc, so the babel APIs don't move the recast comments like they should

jedwards1211 commented 3 years ago

Yeah I've thought about making a babel version of astx as well. And yeah I still need to publish it, need to finalize and document some aspects of the CLI and transform file format.

It might be hard for us to combine efforts because I can be opinionated :P but we are after a lot of the same things so we should at least learn about each other's approaches.

After some false starts with my compare implementation I refactored it into a compileMatcher function that takes the query AST and preprocesses it into a matcher function. For example in this step it checks if an identifier is a capture variable or not, if it is it creates a capture/backreference match function for that AST node, otherwise it just creates a function that matches only identifiers with the same name. That makes running the matcher very efficient, and solved several other technical problems I was having as well.

jedwards1211 commented 3 years ago

Your implementation of compare looks similar to what I started with, walking the properties of the template nodes (called query in my code, whatever) and comparing them to the source code AST path to be matched. I ran into some kind of difficulties with that approach after awhile, I forget what, but they made me decide to refactor it into a precompilation step that generates a CompiledMatcher object:

export interface CompiledMatcher {
  predicate?: false
  captureAs?: string
  arrayCaptureAs?: string
  match: (path: ASTPath<any>, matchSoFar: MatchResult) => MatchResult
  nodeType?: keyof typeof t.namedTypes | (keyof typeof t.namedTypes)[]
}

Passing the matchSoFar allows backreferencing, for instance [$a, $a] would match ['foo', 'foo'] but not ['foo', 'bar'].

Being able to have the arrayCaptureAs field on this object proved especially useful for capturing array slices into a given variable for various different nodes that have array properties.

For example, with the query (template):

import { $_a, foo } from 'foo'

The ImportSpecifier node $_a gets compiled into a matcher with arrayCaptureAs: $_a, and the compiled array matcher for ImportDeclaration.specifiers sees that and is able to capture all specifiers before foo into $_a.

(Note: there's no backreferencing of array slice captures like $_a yet)

The same happens with a type parameters, for example:

class A<$_a, foo> { }

In this case the $_a TypeParameter gets compiled into a matcher with arrayCaptureAs: $_a, and the compiled array matcher for TypeParameterDeclaration.params is able to capture all params before foo into $_a.

In astx, GenericNodeMatcher.ts is what compiles a matcher for an arbitrary node type that compares each of its properties one by one. But I have case-by-case logic for certain node types, for instance the function to compile an ImportSpecifier looks to see if the identifier is a capture variable, and if so return a capturing matcher, otherwise it falls back to GenericNodeMatcher.ts. There's also GenericArrayMatcher.ts where the logic for matching array properties and capturing array slices goes.

jedwards1211 commented 3 years ago

@coderaiser Okay I just finally added some basic documentation for the CLI and transform file format and published astx, if you want to try it out

jedwards1211 commented 3 years ago

I made nice find output for the CLI:

coderaiser commented 3 years ago

Here is what I came up with https://github.com/coderaiser/putout/commit/5846c23952d345d91af8cadec4a6d24351169ed4#diff-ef2420e6e56a8db4df7bf8c4fe146d4bf4d834cfa0531848d7e4c1ea09d83dedR33-R66 Fresh lint and test of 1727 files sped up 1m42s -> 1m30s.

export interface CompiledMatcher {
  predicate?: false
  captureAs?: string
  arrayCaptureAs?: string
  match: (path: ASTPath<any>, matchSoFar: MatchResult) => MatchResult
  nodeType?: keyof typeof t.namedTypes | (keyof typeof t.namedTypes)[]
}

This looks little bit similar to RegExp, is it similar inside?

jedwards1211 commented 3 years ago

No idea really, I've never looked at regex implementations much. I do wonder how my approach to array matching with variable-length placeholders compares to how various regex implementations work

jedwards1211 commented 3 years ago

@coderaiser I just ran into that issue with recast setting loc to null that you reported. It's definitely horrifying to see how it modifies fields output by the parser, I can definitely see why you're advocating for requiring users to use a code formatter if they want to preserve formatting.

I still think being able to reprint a modified AST and preserve formatting of untouched nodes is a worthwhile goal, but the way recast does it seems like a mess...

Btw, I noticed another tool similar to our stuff called semgrep. A friend of mine who's searching for tools like this for C++ let me know about it. Interestingly it supports a broad variety of languages, and is probably extremely fast since the parsers are built with C. There's also a cross-language tool called comby, but AFAICT it's not actually AST-based and I don't like its placeholder syntax at all.

So it's interesting, this is a burgeoning space and it seems like collective consciousness is developing about the need for structural search and replace. But I think there's still a need for tools dedicated to JS. For example semgrep doesn't support Flow syntax right now, and I doubt it will ever support all the different optional syntaxes people can configure in Babel, like the pipeline proposals (I made babel-parse-wild-code for this purpose). I think semgrep will always be a jack of all trades, master of none.

I've been rewriting my astx replacement engine to be really smart about transmuting captured nodes to fit in the replacement context. For example:

// before
const foo = require('foo')
const bar = require('bar')
const {glom, qlx} = require('foo')
const {bar: barr, default: qux} = require('bar')

// -f 'const $1 = require("$2")' -r 'import $1 from "$2"'

// after
import foo from 'foo'
import bar from 'bar'
import {glom, qlx} from 'foo'
import qux, {bar as barr} from 'bar'

This is the kind of JS-specific attention to detail I think cross-language tools will probably never find time to implement.

I'm also planning to change my array matching syntax to use $$ instead of $_, for example [foo, $$rest] would match an array with foo as the first element and then capture the rest of the elements into $$rest. This should be more obvious, especially to anyone familiar with how in jQuery $ selects one element and $$ selects multiple. With this change, and initial $_ would be the new escape sequence for literal $.

After I finish this I'll probably either convert it to pure Babel or figure out a way to make pluggable jscodeshift/Babel backends.

coderaiser commented 3 years ago

@coderaiser I just ran into that issue with recast setting loc to null that you reported. It's definitely horrifying to see how it modifies fields output by the parser, I can definitely see why you're advocating for requiring users to use a code formatter if they want to preserve formatting.

recast is amazing for transforming as minimal as possible, but maintainers usually do not merge PR's so I had to make a fork with my umerged PR's @putout/recast.

Btw, I noticed another tool similar to our stuff called semgrep

Looks good, I'll test it and tell what it can find:

=== setting up agent configuration
| using 652 semgrep rules configured on the web UI
| using 477 code asset inventory rules
| using default path ignore rules of common test and dependency directories
| found 2644 files in the paths to be scanned
| skipping 474 files based on path ignore rules
=== looking for current issues in 2170 files

What I see right now: it doesn't transform, only search without progress bar, who knows when it's done 🤷‍♂️.

It's finished! And detected:

Which is made for a reason, so it's not very useful findings.

By the way, have you seen my PR? Also during work on https://github.com/coderaiser/putout/issues/54 I came up to convert-new-promise-to-async rule in @putout/plugin-promises. This is the simplest variant of converting Promise to async-await, but it designed to be absolutely safe (as it's possible).

There's also a cross-language tool called comby, but AFAICT it's not actually AST-based and I don't like its placeholder syntax at all.

comby much more powerful when it go's to modifications of code, but yes the syntax is strange. I think that point that it's not compatible with JavaScript is disadvantage, of course it's easy choice and now everything can be supported, just use :[args] and be happy with a lot of languages. Anyways ability to parse so many languages with one parser is mind blowing 🤯 , and sounds very interesting (and has video presentation), but yes it can be jack of all trades. It has no built-in rules, so I have no idea what I can use it for.

This is the kind of JS-specific attention to detail I think cross-language tools will probably never find time to implement.

I agree, I think a lot JS-specific features will be missed in such tools. About imports, I'm using __imports in @putout/compare and have two plugins to convert to ESM and CommonJS:

jedwards1211 commented 3 years ago

My point isn't just about creating the ability to convert requires to imports but that the transmutation opens up new possibilities and ease of transformation for unanticipated use cases. For example say for whatever reason you wanted to refactor

const {default: foo, bar: baz} = loadStuff()

To

import foo, {bar as baz} from 'stuff'

Or vice versa. Niche transformations like this are trivial to do with the new replacement engine compared to previous approaches. const $1 = loadStuff() to import $1 from 'stuff'.

Cases like this may seem kind of unlikely, but I believe this will prove surprisingly useful for unanticipated use cases, because refactoring use cases can vary wildly.

So my goal is to make a system so powerful and flexible that I'm able to accomplish transformations I can't even anticipate at the moment with it. I think a lot of linting rules for obvious mistakes like $cond ? $x : $x tend to be simple patterns, but spur-of-the-moment refactoring is a bigger challenge. (Which I'm guessing is why semgrep doesn't support capturing arrays of nodes and interpolating them in replacements, and that's kind of difficult with putout right now too).

Another thing I see a need for is using one pattern to capture where an identifier gets bound to scope, and then doing a find/replace on patterns that refer to that same identifier binding, rather than any identifier with the same name. Because doing scope lookups with recast/babel APIs is still tedious. I haven't decided what the API for finding scoped bindings with patterns would look like yet though.

Semgrep is weird to me because I can't tell if the pattern language has a precise grammar for how capture variables and ... fit into to target language, or if it's doing some kind of string preprocessing instead. (And how does it distinguish between the ... match operator and matching and array or object spread, for example.) It seems poorly specified.

coderaiser commented 3 years ago

So my goal is to make a system so powerful and flexible that I'm able to accomplish transformations I can't even anticipate at the moment with it. I think a lot of linting rules for obvious mistakes like $cond ? $x : $x tend to be simple patterns, but spur-of-the-moment refactoring is a bigger challenge. (Which I'm guessing is why semgrep doesn't support capturing arrays of nodes and interpolating them in replacements, and that's kind of difficult with putout right now too).

This is a good goal :) and putout has different set of goals:

do a practical transformation, that will be used in a day-to-day basis;
work fast even when there is a lot rules that should be run;
easily writable and testable, stable and expandable rules;

So I’m working on practical things that improves JavaScript code, not abstract things that I can't imagine. If I trap in case when I have to do the task of capturing arrays of nodes and interpolating them in replacements more often then never I’ll implement such things. All putout evolution based on needs, not on thoughts. Anyways, different projects set different goals, and there is always things that we can learn on every approach :).

Another thing I see a need for is using one pattern to capture where an identifier gets bound to scope, and then doing a find/replace on patterns that refer to that same identifier binding, rather than any identifier with the same name. Because doing scope lookups with recast/babel APIs is still tedious. I haven't decided what the API for finding scoped bindings with patterns would look like yet though

I’m OK with Babel API for work with scopes, but I understand what are you talking about. I’m also have no idea how it can be expressed in declarative way (especially with keeping in mind compatibility with JavaScript syntax), but I with pleasure take a look at what you will came up with :).

Semgrep is weird to me because I can't tell if the pattern language has a precise grammar for how capture variables and ... fit into to target language, or if it's doing some kind of string preprocessing instead. (And how does it distinguish between the ... match operator and matching and array or object spread, for example.) It seems poorly specified.

That’s true, but video presentation really interesting, this guy made a lot of work building universal AST for so many languages.

What I understand well, that if you doing linter for a language then you should definitely program on it every day, in other case you just want trap on a lot issues that every programmer trap and that can be fixed. Same go’s for frameworks, to build a good quality tool it should be used a lot.

codemodsquad / asyncify

Integration with Putout code transformer #30