Fix missing comments in assignment pattern by yamafaktory · Pull Request #704 · prettier/prettier

yamafaktory · 2017-02-14T22:13:21Z

This PR should fix #699.

yamafaktory · 2017-02-14T22:16:09Z

tests/comments/__snapshots__/jsfmt.spec.js.snap

 "
 `;

-exports[`test dangling_array.js 1`] = `


Oops.. will fix that!

vjeux · 2017-02-14T22:35:12Z

src/comments.js

    } else {
      if (handleIfStatementComments(enclosingNode, followingNode, comment)) {
        // We're good
+      } else if (handleAssignmentPatternComments(enclosingNode, comment)) {


can you put

if ( handleIfStatement... || handleAssignment... || )

like we do above?

vjeux · 2017-02-14T22:36:13Z

src/comments.js

  return false;
 }

+function handleAssignmentPatternComments(enclosingNode, comment) {


I don't think that this is the right fix. Can you figure out where the comment is attached without this logic and find out why this node didn't print its comment?

I did investigate a bit. For the following case, this line is involved https://github.com/jlongster/prettier/blob/master/src/comments.js#L192. Then addCommentHelper enforcescomment.printed = false which strips the comment. If set to true then this is working again:

const { c = 1 /* comment */ } = d;

I don't think you want to alter such a primitive function, that's why I used this handleAssignmentPatternComments function.

But, funny thing, switching to comment.printed = true doesn't break any tests 😮! Then, I just need to figure out how to print the comment for:

const {a /* comment */ = 1 } = b

comment.printed shouldn't have any affect on the output; it's used to do a sanity check afterwards to make sure we printed all the comments. I don't know why it would affect the output.

Good to know, thanks! But this definitely has an impact... Your thoughts on the handleAssignmentPatternComments function, is that a wrong approach?

It's not the wrong approach if we have to do it, but I'd also like to know why we have to. If like you say it triggers that branch and attaches it as a trailing comment to the identifier, it should be printed.

Search the code for comment.printed; it's only ever set the true and false. Nothing ever branched on it, so can you look into why something is changed and why the comment isn't printed in the first place?

Yeah, I saw it. I'm going to investigate, thanks!

@jlongster + @vjeux I think I've identified what's causing the issue. For example, with something like:

const {a /* comment */ = 1 } = b

The comment is stripped and we get the error message for non-printed comments. This is coming from https://github.com/jlongster/prettier/blob/master/src/comments.js#L487 where the right-hand side of the boolean (types.getFieldValue(value, "comments")) is always null or undefined. As a consequence, on line 490, printed is returned and the comment is sadly omitted...

Oh, and I forgot to mention that in this case there is a trailing comment (I can trace it here https://github.com/jlongster/prettier/blob/master/src/comments.js#L193).

jlongster · 2017-02-16T15:16:19Z

I discovered the problem. The problem is that the { a = 1 } code is compiled into an ObjectProperty with shorthand: true, which indicates that we should only print the value node. For normal shorthand objects like { a } it wouldn't matter if we printed the key or value, but we print the value because the destructuring assignment above exists on the value prop as a AssignmentPattern. Here's the important parts of the AST:

Node {
  type: 'ObjectProperty',
  start: 7,
  end: 26,
  loc:
   SourceLocation {
     start: Position { line: 1, column: 7 },
     end: Position { line: 1, column: 26 } },
  method: false,
  shorthand: true,
  computed: false,
  key:
   Node {
     type: 'Identifier',
     start: 7,
     end: 8,
     loc: SourceLocation { start: [Object], end: [Object], identifierName: 'a' },
     name: 'a',
     leadingComments: null,
     trailingComments: [ [Object] ],
     comments: [ [Object] ] },
  value:
   Node {
     type: 'AssignmentPattern',
     start: 7,
     end: 26,
     loc: SourceLocation { start: [Object], end: [Object] },
     left:
      Node {
        type: 'Identifier',
        start: 7,
        end: 8,
        loc: [Object],
        name: 'a',
        __clone: [Function: __clone] },
     right:

The problem is that the comment is attached to the identifier in the key node, as you can see in the comments array. But the identifier in the AssignmentPattern is what's actually printed.

Not sure what the best solution is yet, but that's the problem.

yamafaktory · 2017-02-19T15:56:18Z

@jlongster I tried to implement a different logic, i.e. solving the root issue by handling these comments attached to the key node. Not sure if this is a better way to do it, and by the way, this currently only solves the non-printed comment warning (still need to print those comments in the end).

vjeux · 2017-02-20T02:36:32Z

@@ -179,7 +179,8 @@ function attach(comments, ast, text) {
         addDanglingComment(ast, comment);
       }
     } else {
-      if (handleIfStatementComments(enclosingNode, followingNode, comment)) {
+      if (handleIfStatementComments(enclosingNode, followingNode, comment) ||
+          handleObjectProperty(enclosingNode, precedingNode, comment)) {
         // We're good
       } else if (precedingNode && followingNode) {
         // Otherwise, text exists both before and after the comment on
@@ -395,6 +396,17 @@ function handleConditionalExpressionComments(
   return false;
 }

+function handleObjectProperty(enclosingNode, precedingNode, comment) {
+  if (enclosingNode.type === "ObjectProperty" &&
+      enclosingNode.shorthand &&
+      enclosingNode.key === precedingNode &&
+      enclosingNode.value.type === "AssignmentPattern") {
+    addTrailingComment(enclosingNode.value.left, comment);
+    return true;
+  }
+  return false;
+}
+

What do you think about relocating the comment the same way we move a bunch of others?

const {/* missing */ a /* missing comment */ = 1} = b

outputs

const { /* missing */ a /* missing comment */ = 1 } = b;

yamafaktory · 2017-02-20T06:01:23Z

This looks good to me!

yamafaktory · 2017-02-20T08:02:20Z

@vjeux Actually you've implemented something close to what I initially did 😛 fc998c9. And we need to check for type === "Property" when using Flow. But sadly this is breaking other tests, I have to investigate.

vjeux · 2017-02-20T15:21:45Z

src/comments.js


    if (
      locStart(child) - locStart(comment) <= 0 &&
-        locEnd(comment) - locEnd(child) <= 0


Can you revert all the changes you've triggered by running prettier on that file? :)

vjeux · 2017-02-20T15:22:29Z

src/printer.js

  return path.needsParens() ? concat(["(", lines, ")"]) : lines;
 }

+function shouldPrintComma(options, level) {


I think you messed up your rebase :x

Oops, let me clean that!

…99-fix-missing-comments-assignment-pattern

…ction.

vjeux · 2017-02-20T15:39:14Z

Yay, thanks!

yamafaktory · 2017-02-20T15:39:18Z

@vjeux Now this should be cleaned up and all the tests should pass!

yamafaktory · 2017-02-20T15:39:34Z

@vjeux Wow, you're fast! Thanks 👍

vjeux · 2017-02-20T15:41:57Z

I'm pretty excited about this PR :)

yamafaktory added 3 commits February 14, 2017 23:10

Add handleAssignmentPatternComments helper function.

fc998c9

Add new tests 🎉.

9637bfb

Remove unecessary arg in function.

c7c8a34

yamafaktory commented Feb 14, 2017

View reviewed changes

Reorder it.

44b0302

vjeux reviewed Feb 14, 2017

View reviewed changes

Experimental fix for key comments in the printComments function.

8e5e713

Pull master and switch to @vjeux implementation.

c241212

vjeux reviewed Feb 20, 2017

View reviewed changes

yamafaktory added 3 commits February 20, 2017 16:33

Fix bad rebase.

0c6f214

Merge branch 'master' of https://github.com/jlongster/prettier into 6…

33af950

…99-fix-missing-comments-assignment-pattern

Add additional checking for enclosingNode in handleObjectProperty fun…

229b6b4

…ction.

vjeux merged commit d613f92 into prettier:master Feb 20, 2017

yamafaktory deleted the 699-fix-missing-comments-assignment-pattern branch February 25, 2017 18:30

lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 21, 2019

lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019

Uh oh!

Conversation

yamafaktory commented Feb 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamafaktory Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlongster commented Feb 16, 2017

Uh oh!

yamafaktory commented Feb 19, 2017

Uh oh!

vjeux commented Feb 20, 2017

Uh oh!

yamafaktory commented Feb 20, 2017

Uh oh!

yamafaktory commented Feb 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vjeux commented Feb 20, 2017

Uh oh!

yamafaktory commented Feb 20, 2017

Uh oh!

yamafaktory commented Feb 20, 2017

Uh oh!

vjeux commented Feb 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yamafaktory Feb 15, 2017 •

edited

Loading

yamafaktory commented Feb 20, 2017 •

edited

Loading