workflows/check-nixf-tidy.yml: init

[Aleksanaa]

Description of changes

CC @inclyc @Sigmanificient (I saw you were working on such clean-ups) @rhendric (#313981 (comment))

Here is an example test: https://github.com/Aleksanaa/nixpkgs/pull/1/files

This check will fail on:

1. Parsing error (Thanks to the recursive decent parser, it is possible to get better error reporting than nix itself) https://github.com/nix-community/nixd/tree/main/libnixf#background--motivation
2. Unused variables (function arguments, variable definition, unused rec, with, etc). This addresses Tracking issue: Removal of uneeded argument #313981
3. Escaping with (reference variables outside of with expression). This also partially addresses Tracking issue: remove overuses of with lib; #208242

It uses nixf-tidy in nixf package. The json output is processed by jq to form GitHub workflow messages, which will be rendered in PR's 'Files changed' overview.

Like the workflow of nixfmt check, this workflow only fails when regression occurs (that is, the old file has no errors, but errors occur after the change; or the new file has errors). This is to avoid some larger files that cannot pass nixf-tidy (such as all-packages.nix). A feature that filters errors based on the lines affected (in git diff) may be implemented, but may require nixf improvements and more testing to work well.

---

Add a 👍 reaction to pull requests you find important.

[Sigmanificient]

Well i did't knew about nixf-tidy, used deadnix --json for cleanup, but it can report false negatives

[Mic92]

Do we really need to run on the pull_request_target? Would pull_request not also work?

[infinisil]

GitHub Actions is a fickle beast: https://github.com/NixOS/nixpkgs/blob/d2a2dbc06347b4c5e30dd258743f6b3cae30d985/.github/workflows/check-by-name.yml#L8C1-L8C103

[infinisil]

Looking good overall!

[Sigmanificient]

for the CI output, maybe printing the line in question would be wanted?
This would provide more context than just the affected line/col

As an example:

  [line 30, column 38 ~ line 30, column 45] sema-escaping-with: this variable comes from the scope outside of the `with` expression
    [line 5, column 2 ~ line 5, column 9] var-bind-to-this: variable will bind to this definition

would become

  [line 30, column 38 ~ line 30, column 45]
  `     maintainers = with maintainers; [ version ];`
  sema-escaping-with: this variable comes from the scope outside of the `with` expression

    [line 5, column 2 ~ line 5, column 9]
    `  version = "0.35.1";`
    var-bind-to-this: variable will bind to this definition

[infinisil]

Ohh! GitHub Actions can actually do this very nicely with a workflow command:

echo "::warning file=app.js,line=1,col=5,endColumn=7::Missing semicolon"

This should highlight the problematic lines in the file view of PRs!

[Sigmanificient]

Yeah that would be even better

[Aleksanaa]

Ohh! GitHub Actions can actually do this very nicely with a workflow command:

echo "::warning file=app.js,line=1,col=5,endColumn=7::Missing semicolon"

This should highlight the problematic lines in the file view of PRs!

Great idea! It works now: https://github.com/Aleksanaa/nixpkgs/pull/1/files

addressed

[Aleksanaa]

Still another problem, it looks like the old error message is not cleared when the new error message appears. If you change the title or the like, multiple error messages may be generated

[Sigmanificient]

will an empty maintainer list mantainers = with lib.mantainers; [] tigger a warning CI?
We might want to filter this one if it causes problem

[Aleksanaa]

will an empty maintainer list mantainers = with lib.mantainers; [] tigger a warning CI? We might want to filter this one if it causes problem

Yes, it's unused with. But I want it to report this error because we don't really need that with. Also, it will not throw new errors for files that already contain errors before editing.

[infinisil]

Still another problem, it looks like the old error message is not cleared when the new error message appears. If you change the title or the like, multiple error messages may be generated

This is getting more messy, but I believe you should be able to only conditionally output the errors using something like

if [[ "${{github.event_name}}" == 'edited' ]]; then
  echo "Edited the PR"
  if [[ -z "${{github.event.edited.changes.base}}" ]]; then
    echo "But didn't change the base branch, only the description/title"
    # Don't output errors
  else
    # Do output errors, rare case of changing the base branch, which might cause different errors. Can't void the old ones unfortunately though, but should happen rarely.
  fi
else
  # Do output errors
fi

[Aleksanaa]

This is getting more messy, but I believe you should be able to only conditionally output the errors using something like

This works now

[infinisil]

This looks ready to me, nice work! While this is a rather fresh PR, I don't think we need to wait for anything else, let's just try out and see how well it works 🚀

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/we-added-a-linter-to-nixpkgs-checking-workflows/49722/2