Open Bug 1803229 Opened 2 years ago Updated 1 year ago

Lando's code formatting can fix more than just formatting and more than just touched files

Categories

(Conduit :: Lando, defect)

Product:
Conduit
Component:
Lando
Type:
defect

Tracking

(Not tracked)

People

(Reporter: standard8, Unassigned)

References

Details

In bug 1801092, I have been attempting to land a patch that enables a new rule for ESLint. This is not formatting, it replaces one API for another. This broke tests because the tests were expecting the original API.

I did not get any warning (from neither Lando nor Phabricator review bots) that these items would be replaced.

The problem appeared because we are trying to roll out a new rule which raises warnings where there are existing failures. The rule is able to be auto-fixed, however we are not automatically fixing all instances straight away because of the test issues mentioned above. The plan is to fix them gradually - however we want to get the rule out early to help prevent new instances occurring.

In this case, because Lando was running with --fix the warnings were fixed automatically. Code review did not show these up because they were warnings in files not affected by the patch.

For formatting via ESLint we mainly use prettier. Though there is one additional rule that handles formatting as well.

One option here may be to change how Lando runs things, so that it runs only the formatting parts of ESLint. We might be able to do this with a separate config file.

Another option would be for Phab/Lando to make it clear that more than just formatting will change.

See Also: → 1801092

(In reply to Mark Banner (:standard8))

In this case, because Lando was running with --fix the warnings were fixed automatically. Code review did not show these up because they were warnings in files not affected by the patch.

This looks like a bug: Lando runs mach lint --fix --outgoing which should only touch files touched by commits not on the remote repository. The commits from Bug 1803229 (7323c74 and 9df9c27) don't directly touch any of the files modified by Lando (9153de9). I wonder if explicitly specifying the revset to mach lint would be better?

One option here may be to change how Lando runs things, so that it runs only the formatting parts of ESLint. We might be able to do this with a separate config file.

That sounds like the right approach here however it appears some mach plumbing is required first.
mach format sounds like what we want, but that just calls mach lint --fix 🙁

(In reply to :glob ✱ from comment #2)

(In reply to Mark Banner (:standard8))

In this case, because Lando was running with --fix the warnings were fixed automatically. Code review did not show these up because they were warnings in files not affected by the patch.

This looks like a bug: Lando runs mach lint --fix --outgoing which should only touch files touched by commits not on the remote repository. The commits from Bug 1803229 (7323c74 and 9df9c27) don't directly touch any of the files modified by Lando (9153de9). I wonder if explicitly specifying the revset to mach lint would be better?

That would probably help in some cases, but not in cases where we're touching the support-files of a linter, which causes a full re-lint.

One option here may be to change how Lando runs things, so that it runs only the formatting parts of ESLint. We might be able to do this with a separate config file.

That sounds like the right approach here however it appears some mach plumbing is required first.
mach format sounds like what we want, but that just calls mach lint --fix 🙁

We might be able to add a --format-only argument to mach lint which would then instruct ESLint to load a different configuration file. I'm not totally sure, but I think that would be possible.

(In reply to Mark Banner (:standard8) from comment #3)

That would probably help in some cases, but not in cases where we're touching the support-files of a linter, which causes a full re-lint.

That feels like a problem that mach lint should address if it doesn't already - ie. if mach lint --fix --outgoing (or similar) detects that one of the files touched might require a full repo run, it should internally expand the list of files to apply the change to. Teaching Lando about each linter/formatter's internals is likely to end up with divergent lists.

(In reply to :glob ✱ from comment #4)

(In reply to Mark Banner (:standard8) from comment #3)

That would probably help in some cases, but not in cases where we're touching the support-files of a linter, which causes a full re-lint.

That feels like a problem that mach lint should address if it doesn't already - ie. if mach lint --fix --outgoing (or similar) detects that one of the files touched might require a full repo run, it should internally expand the list of files to apply the change to. Teaching Lando about each linter/formatter's internals is likely to end up with divergent lists.

I'm not sure I understand. If it detects a support file changed, it does force a full repository re-run - which is what you I think are saying here. You earlier said that:

Lando runs mach lint --fix --outgoing which should only touch files touched by commits not on the remote repository.

The problem is in this case, that isn't true. It could touch more files than what are touched in the commits because of the support file updates.

(In reply to Mark Banner (:standard8) from comment #5)

That would probably help in some cases, but not in cases where we're touching the support-files of a linter, which causes a full re-lint.

That feels like a problem that mach lint should address if it doesn't already - ie. if mach lint --fix --outgoing (or similar) detects that one of the files touched might require a full repo run, it should internally expand the list of files to apply the change to. Teaching Lando about each linter/formatter's internals is likely to end up with divergent lists.

I'm not sure I understand. If it detects a support file changed, it does force a full repository re-run - which is what you I think are saying here.

Ah, I took "[it wouldn't help] in cases where we're touching the support-files of a linter, which causes a full re-lint" as the full re-lint requirement would need to be implemented in Lando; looks like we're on the same page here.

Lando runs mach lint --fix --outgoing which should only touch files touched by commits not on the remote repository.

The problem is in this case, that isn't true. It could touch more files than what are touched in the commits because of the support file updates.

I'm glad that's the case. Note I pulled that statement from mach lint --help:

  -o [OUTGOING], --outgoing [OUTGOING]
    Lint files touched by commits that are not on the remote repository. Without arguments, finds the default
    remote that would be pushed to. The remote branch can also be specified manually. Works with mercurial or
    git.

This problem applies to all linters, not just ESLint.
For example, I can't land https://phabricator.services.mozilla.com/D163567 or even https://phabricator.services.mozilla.com/D163616.

Summary: Lando's code formatting can fix more than just formatting for ESLint → Lando's code formatting can fix more than just formatting and more than just touched files

I'd like to come up with an actionable plan to solve this problem, and hopefully unblock these linting/formatting patches. This is what I propose:

  • Turn off Lando autoformatting.
  • Patches currently blocked by this bug can be landed.
  • Implement a fix for this issue in our in-tree formatters*.
  • Make any changes to Lando required to take the fix.
  • Re-enable Lando autoformatting.

*There have been a few suggestions for what the "fix" should be, either:

  • Make mach lint avoid checking the entire tree when support-file is touched if --fix is passed.
  • Implement mach format as a command that only runs formatters and is not an alias to mach lint --fix, where the latter can potentially fix non-formatting issues.

:ahal, :marco, what would your preferred solution be here? Making the tree-wide lint not take place with --fix is present is probably the quickest fix, but a few folks have expressed interest in mach format or similar that only applies formatting fixes. Whatever the solution I would be happy to implement it with some guidance.

Flags: needinfo?(mcastelluccio)
Flags: needinfo?(ahal)

(In reply to Connor Sheehan [:sheehan] from comment #8)

*There have been a few suggestions for what the "fix" should be, either:

  • Make mach lint avoid checking the entire tree when support-file is touched if --fix is passed.
  • Implement mach format as a command that only runs formatters and is not an alias to mach lint --fix, where the latter can potentially fix non-formatting issues.

:ahal, :marco, what would your preferred solution be here? Making the tree-wide lint not take place with --fix is present is probably the quickest fix, but a few folks have expressed interest in mach format or similar that only applies formatting fixes. Whatever the solution I would be happy to implement it with some guidance.

I've also been thinking about this a bit. I think having a formatting-only option for reviewbot is where we need to end up.

The first solution doesn't quite achieve that, because you might also touch files in the patch where you don't want the autofixes just yet. Additionally we'd need to make that a reviewbot only option to not run if a support file is touched - as we frequently use it locally for rolling out changes to new rules.

However, the first solution might be a reasonable interim, depending on how long the second solution would take us to implement.

:ahal, :marco, what would your preferred solution be here? Making the tree-wide lint not take place with --fix is present is probably the quickest fix

Yeah, I think we should do this. I can't really imagine a scenario where you would want to format files outside of the ones you explicitly touch / specify. This feature was added to make sure that we don't accidentally let regressions slip through when modifying lint files. But it's not needed for fixing.

The fix should be pretty trivial, simply check if --fix was used in this condition:
https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/python/mozlint/mozlint/roller.py#264

Flags: needinfo?(ahal)

I agree with that as an interim fix (as Mark mentioned, it isn't a full fix). I guess if we land it now we don't even need to turn off autoformatting.

Note "mach format" is already not exactly just a "mach lint --fix" alias, it is only running linters that are marked as formatters (that is, linters which are in this list: https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/tools/lint/mach_commands.py#26).

Flags: needinfo?(mcastelluccio)

(In reply to Andrew Halberstadt [:ahal] from comment #10)

:ahal, :marco, what would your preferred solution be here? Making the tree-wide lint not take place with --fix is present is probably the quickest fix

Yeah, I think we should do this. I can't really imagine a scenario where you would want to format files outside of the ones you explicitly touch / specify. This feature was added to make sure that we don't accidentally let regressions slip through when modifying lint files. But it's not needed for fixing.

The fix should be pretty trivial, simply check if --fix was used in this condition:
https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/python/mozlint/mozlint/roller.py#264

Perfect, I'll file a bug for this work and get started on it.

(In reply to Marco Castelluccio [:marco] from comment #11)

I agree with that as an interim fix (as Mark mentioned, it isn't a full fix). I guess if we land it now we don't even need to turn off autoformatting.

Note "mach format" is already not exactly just a "mach lint --fix" alias, it is only running linters that are marked as formatters (that is, linters which are in this list: https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/tools/lint/mach_commands.py#26).

Interesting. Should Lando run mach format --outgoing instead of mach lint --fix --outgoing?

Flags: needinfo?(mcastelluccio)
Flags: needinfo?(ahal)
Depends on: 1805804

It might be better? The downside of switching is that it would apply fewer fixes. The upside is that it wouldn't make as many unexpected changes. This is because --fix would fix things that are warnings, or in some cases (like flake8 which uses autopep8), things that aren't even issues at all.

The list of linters it runs is here:
https://searchfox.org/mozilla-central/source/tools/lint/mach_commands.py#26

(we should also add isort there)

So switching would mean there would be no auto-fixing of JS files anymore.

Alternatively maybe lando should explicitly pass in its own list of linters, so we can exclude ones like flake8 that are kind of sketchy.

Flags: needinfo?(ahal)

FYI the patch from bug 1805804 is on autoland, it should be possible to land formatting fixes that touch support-files now. Please let me know if you encounter more problems.

(In reply to Andrew Halberstadt [:ahal] from comment #13)

It might be better? The downside of switching is that it would apply fewer fixes. The upside is that it wouldn't make as many unexpected changes. This is because --fix would fix things that are warnings, or in some cases (like flake8 which uses autopep8), things that aren't even issues at all.

The list of linters it runs is here:
https://searchfox.org/mozilla-central/source/tools/lint/mach_commands.py#26

(we should also add isort there)

So switching would mean there would be no auto-fixing of JS files anymore.

Alternatively maybe lando should explicitly pass in its own list of linters, so we can exclude ones like flake8 that are kind of sketchy.

Okay, something to think about. For now I think we can keep Lando running mach lint --fix, auto-fixing JS is desirable. If we hit issues with too many fixes being applied we can switch to the more conservative mach format or pass an explicit list of linters Lando should run.

I think it would be preferable to use "mach format", but only after we make JS autoformatting run on "mach format" too.

Flags: needinfo?(mcastelluccio)
See Also: → 1807711
See Also: → 1807712

(In reply to Marco Castelluccio [:marco] from comment #15)

I think it would be preferable to use "mach format", but only after we make JS autoformatting run on "mach format" too.

Filed bug 1807711 and bug 1807712 for this.

You need to log in before you can comment on or make changes to this bug.