1831354 - Prettier portion of ESLint can report incorrect paths

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, task)

Description

[Rob Lemley [:rjl]]

Attachment: sample.html

I noticed this when working with the prettier step of ESLint on comm-central code, but the issue is not limited to comm-central.

STR:

• Find a file a few paths deep that both ESLint and Prettier will check
• cd to that file's directory
• Modify the file, preferably in a way that both ESLint and Prettier will report an issue
• Run mach lint on the file without changing your shell working directory

EXPECTED:

• mach lint reports a single file with two errors, one from ESLint one from Prettier

ACTUAL:

• The output has two "files" with errors, the one from Prettier has an incorrect path.

See attachment.

Why this matters for comm-central:

comm-central has a mach commlint wrapper around mach lint in order to pick up different config files and such.

For ESLint, to ensure that comm/.eslintignore and comm/.prettierignore are used, before running ESLint, the working directory is changed to $topsrcdir/comm. This works well, helps keep paths to files consistent, and is transparent to the developer running mach lint.

However, because of this bug, Prettier now reports all paths incorrectly. The "comm/" portion of the path is dropped resulting in paths like /src/mozilla-central/mail/base/content/aboutDialog.js.

The bug is actually here.

if os.path.isabs(self.path):
    self.path = mozpath.normpath(self.path)
    self.relpath = mozpath.relpath(self.path, root)
else:
    self.relpath = mozpath.normpath(self.path)
    self.path = mozpath.join(root, self.path)

Context-wise, self.path is a path to a file with an "Issue", and this is post-init code for the Issue object that's supposed to make self.path absolute and self.relpath relative to $topsrcdir.

However, when the working directory is not $topsrcdir, the last mozpath.join line sets an invalid absolute path.

This potentially affects linters beyond ESLint and Prettier, though it's the first time I've encountered it.

There's two potential fixes I see: In run_prettier, make the paths output by Prettier absolute when building the Issue() objects, or change the mozpath.join line to mozpath.abspath.

The later may cause odd linter bugs elsewhere.

Comment 1

[Rob Lemley [:rjl]]

Changing the Issue() code as mentioned above is indeed problematic, though it's probably where the bug should be fixed.

Since this mostly affects the ESLint+Prettier results, I'm submitting a patch to convert the relative paths output by Prettier to absolute paths before they are converted to Issue objects as this is the least invasive method and fixes the immediate problem. I don't know of other linters being affected by this bug.

Comment 2

[Rob Lemley [:rjl]]

Attachment: Bug 1831354 - Calculate absolute paths of prettier results before returning. r=#linter-reviewers

Fixes an immediate problem with Prettier step of ESLint that returns relative
paths to files needing reformatting. When the absolute path is set by Issue(),
it's incorrect if the current working directory is not the Gecko source root.

Running ESLint and Prettier from the GRE root does not work for Thunderbird,
as the .eslintignore and .prettierignore files from comm/ are not used. The
solution that works best is to chdir("comm/") before running ESLint. Doing so
reports incorrect paths from Prettier.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10af8868042b
Calculate absolute paths of prettier results before returning. r=linter-reviewers,Standard8

Comment 4

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/10af8868042b