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

Wrong cursorOffset with multi-line comment #14811

Closed
hyrious opened this issue May 10, 2023 · 10 comments · Fixed by #14812
Closed

Wrong cursorOffset with multi-line comment #14811

hyrious opened this issue May 10, 2023 · 10 comments · Fixed by #14812
Assignees
Labels
area:ranges Issues about formatting/ignoring/etc segments of files lang:javascript Issues affecting JS locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. type:bug Issues identifying ugly output, or a defect in the program

Comments

@hyrious
Copy link

hyrious commented May 10, 2023

Environments:

  • Prettier Version: 2.8.8
  • Running Prettier via: Node.js API
  • Runtime: Node.js 18.16.0
  • Operating System: macOS
  • Prettier plugins (if any):

Steps to reproduce:

Save this script and run it:

import prettier from "prettier";

const code = `
// All messages are represented in JSON.
// So, the prettierd.py controls a subprocess which spawns "node {this_file}".
import {  } from "fs"
`; //   ^ cursor here

console.log(
  prettier.formatWithCursor(code, {
    cursorOffset: 129,
    parser: "babel",
  })
); // => { cursorOffset: 65 }

Expected behavior:

Expect the returned cursorOffset to be around 129, right in the {}.

Actual behavior:

It returns 65, which is in the comments.

Frankly, I don't even find a minimal case to reproduce this issue (the code above is all I could make, changing any line of the comment may make the result correct).

@kachkaev
Copy link
Member

Please use the issue template. Prettier Playground supports ranges if this is what you are trying to show:

Screenshot 2023-05-10 at 08 43 51

@kachkaev kachkaev added the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label May 10, 2023
@hyrious
Copy link
Author

hyrious commented May 10, 2023

@kachkaev Ok, I've updated the format.

The playground does not help here since it cannot set the cursor position.

@github-actions github-actions bot removed the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label May 10, 2023
@fisker fisker added type:bug Issues identifying ugly output, or a defect in the program lang:javascript Issues affecting JS area:ranges Issues about formatting/ignoring/etc segments of files labels May 10, 2023
@fisker fisker self-assigned this May 10, 2023
@fisker fisker mentioned this issue May 10, 2023
4 tasks
@fisker
Copy link
Member

fisker commented May 10, 2023

@hyrious

The cursor tracking does not always return the expected position, I feel lots of problems in this part.
#14812 may fix your case.

Do you have more test cases?

@fisker
Copy link
Member

fisker commented May 10, 2023

the code above is all I could make, changing any line of the comment may make the result correct

Your right about this, this is strange. I can't change the code either.

My script to test
import * as mainPrettier from "./index.js";
import stablePrettier from "./node_modules/prettier/index.js";

const code = `
// All messages are represented in JSON.
// So, the prettierd.py controls a subprocess which spawns "node {this_file}".
import {<|>  } from "fs"
`;

const cursorOffset = code.indexOf("<|>");
const codeToFormat = code.replace("<|>", "");

console.table([
  {
    prettier: mainPrettier.version,
    originalCursorOffset: cursorOffset,
    cursorOffset: (
      await mainPrettier.formatWithCursor(codeToFormat, {
        cursorOffset,
        parser: "babel",
      })
    ).cursorOffset,
  },
  {
    prettier: stablePrettier.version,
    originalCursorOffset: cursorOffset,
    cursorOffset: (
      await stablePrettier.formatWithCursor(codeToFormat, {
        cursorOffset,
        parser: "babel",
      })
    ).cursorOffset,
  },
]);

@fisker
Copy link
Member

fisker commented May 10, 2023

Smaller reproduction

// All m are represented in JSON.
// So, the prettierd.py controls a subprocess which spawns "node {this_file}".
import {<|>  } from "fs"

Change the first m letter in first line to a, generates different result.

@fisker
Copy link
Member

fisker commented May 10, 2023

import { diffArrays } from "diff";

const code = `
// All m are represented in JSON.
// So, the prettierd.py controls a subprocess which spawns "node {this_file}".
import {} from "fs";
`.trim();

function diff(string) {
  return diffArrays([...'import {  } from "fs";'], [...string]);
}

console.log([diff(code), diff(code.replace("m", "a"))]);

The results are so different, maybe bug in diff?

[
  [
    { count: 1, added: undefined, removed: true, value: [Array] },
    { count: 7, added: true, removed: undefined, value: [Array] },
    { count: 1, value: [Array] },
    { count: 7, added: true, removed: undefined, value: [Array] },
    { count: 1, value: [Array] },
    { count: 22, added: true, removed: undefined, value: [Array] },
    { count: 1, value: [Array] },
    { count: 7, added: true, removed: undefined, value: [Array] },
    { count: 1, value: [Array] },
    { count: 1, added: true, removed: undefined, value: [Array] },
    { count: 1, value: [Array] },
    { count: 8, added: true, removed: undefined, value: [Array] },
    { count: 1, value: [Array] },
    { count: 1, added: undefined, removed: true, value: [Array] },
    { count: 8, added: true, removed: undefined, value: [Array] },
    { count: 1, value: [Array] },
    { count: 1, added: true, removed: undefined, value: [Array] },
    { count: 1, value: [Array] },
    { count: 40, added: true, removed: undefined, value: [Array] },
    { count: 1, value: [Array] },
    { count: 9, added: true, removed: undefined, value: [Array] },
    { count: 1, value: [Array] },
    { count: 3, added: true, removed: undefined, value: [Array] },
    { count: 10, value: [Array] }
  ],
  [
    { count: 25, added: true, removed: undefined, value: [Array] },
    { count: 1, value: [Array] },
    { count: 88, added: true, removed: undefined, value: [Array] },
    { count: 7, value: [Array] },
    { count: 2, added: undefined, removed: true, value: [Array] },
    { count: 12, value: [Array] }
  ]
]

@fisker
Copy link
Member

fisker commented May 10, 2023

Raised an issue kpdecker/jsdiff#400

@fisker
Copy link
Member

fisker commented May 10, 2023

Shortest string I can find

import { diffArrays } from "diff";

const code = `
// m represent o, 1 represent 1 1 {}
import {} from "fs";
`.trim();

function diff(string) {
  return diffArrays([...'import {  } from "fs";'], [...string]);
}

console.log([
  diff(code),
  diff(code.replace("o", "a")),
  diff(code.replace("m", "a")),
]);

@so1ve
Copy link
Contributor

so1ve commented May 11, 2023

jsdiff is almost... dead? I think the project has been abandoned by its author and this bug may not be fixed in a while.


Just curious. Never mind.

@fisker
Copy link
Member

fisker commented May 11, 2023

I know that. My PR has been there for over a year kpdecker/jsdiff#351

Anyway, #14812 will fix your issue, so not that important.

@github-actions github-actions bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Nov 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area:ranges Issues about formatting/ignoring/etc segments of files lang:javascript Issues affecting JS locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. type:bug Issues identifying ugly output, or a defect in the program
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants