benjamn / recast

JavaScript syntax tree transformer, nondestructive pretty-printer, and automatic source map generator
MIT License
4.99k stars 350 forks source link

Return statements are always printed generically if anything in the return value has changed. #552

Open dbaum opened 5 years ago

dbaum commented 5 years ago

I understand that this is WAI - the code near the end if findChildReprints() indicates that this is a precautionary measure to avoid problems with ASI. I'd like the ability to disable this feature so that return statements use the normal rules for reprinting. The codebase I'm working with always has semicolons and the lack of reprinting causes problems with jscompiler type casts:

If I start with:

return /* @type {string} / (foo());

and change "foo" to "bar", the output is:

return ( /* @type {string} / bar(); );

This is no longer accepted by the jscompiler since jscompiler casts require parentheses around the value being cast.

Re-enabling return statement reprinting via an option should be relatively simple. I suspect the biggest problem is plumbing the option down into the code. Passing an options arg to getReprinter() seems reasonable, but I'm not sure if it would be better to explicitly pass the options through the various findReprint functions or capture the options in a local scope and move the findReprint functions into that scope. If you have a preferred way of doing this, let me know and I'll work on making the change.

kryops commented 3 years ago

I think I stumbled upon a similar problem with the same underlying cause.

I am using jscodeshift for a migration in a large React code base. We have a lot of components that look like this:

function Foobar() {
  return <Component
    lorem="ipsum"
  />
}

When changing anything in the React elements the component returns, parentheses are added and the JSX content is indented:

function Foobar() {
  return (
    <Component
      lorem="dolor sit amet"
    />
  );
}

Is there a way to change this behavior?


Reproduction:

const { parse, print } = require("recast");

const source = `function Foobar() {
  return <Component
    lorem="ipsum"
  />
}`

const ast = parse(source)

ast.program.body[0].body.body[0].argument.openingElement.attributes[0].value.value = "dolor sit amet"

console.log(print(ast).code);
gnprice commented 2 years ago

The codebase I'm working with always has semicolons

So, this unfortunately does not really protect you from running into the problems that that logic at the end of findChildReprints() is there to try to avoid.

The test cases that exercise that logic look like this. They start with code like:

function f() {
  return 3;
}

then parse it, and insert a comment:

      ast.program.body[0].body.body[0].argument.comments = [b.line("Foo")];

Then print the AST back, and check that it looks like this:

function f() {
  return (
    //Foo
    3
  );
}

If on the other hand you disable that logic that parenthesizes the return argument:

--- a/lib/patcher.ts
+++ b/lib/patcher.ts
@@ -496,12 +496,12 @@ function findChildReprints(newPath: any, oldPath: any, reprints: any) {
   // Return statements might end up running into ASI issues due to
   // comments inserted deep within the tree, so reprint them if anything
   // changed within them.
   if (
     ReturnStatement.check(newPath.getNode()) &&
     reprints.length > originalReprintCount
   ) {
-    return false;
+    // return false;
   }

   return true;

then the output will instead look like this:

function f() {
  return //Foo
  3;
}

where the body parses as two statements, equivalent to return; 3;, rather than one.

So the original source was perfectly normal, well-formatted code, with all its semicolons. The issue is that when you add a comment, it could force a newline to be inserted -- and that in turn could cause ASI to insert a semicolon where you don't intend one.

I think that makes an option not an appealing approach for this issue, because it's not the case that for some codebases (i.e. well-formatted, semicolon-using ones) it'd be safe to just always enable. Rather, whether such an option would be safe to enable would depend on the details of what the transform did. So probably the better approach is to work out more precisely when a reprint is actually needed here, and have this logic in findChildReprints start doing that.