Setup clang-format as an Arcanist linter

[starsid]

Summary

This uses clang-format-diff as a linter for Arcanist.

arc lint flow, also run as part of arc diff unless skipped with
--nolint, will now run the linter shell script on the changed files,
and prompt the user to accept the suggested changes.

Message when clang-format-diff is not installed:

Example of the noise during code review when clang-format-diff is not installed:
https://reviews.llvm.org/differential/changeset/?ref=1115809

Prompt when clang-format-diff is installed and suggests edits:

[starsid]

typo in comments

[dberris]

.arcanist/linters/clang-format.sh

13–14

What happens when the script exits with a non-zero value? Does it stop the upload/update of the patch? I suspect we don't really want to make that a hard requirement yet, unless we get agreement from llvm-dev that this is something we should enforce for all patches moving forward.

.arclint

9

Should this also include '.cc' files?

[JDevlieghere]

I'd move the script into utils/arcanist instead of creating a (although hidden) top level directory.

[starsid]

Move linter script to utils/arcanist.
Add message on how to skip linting.

[starsid]

Also include .cc files

[starsid]

.arcanist/linters/clang-format.sh

13–14

When the script exits with a non-zero value (which should only happen if clang-format-diff is not installed), the user's upload workflow is stopped, and the user is messaged saying that the linter failed.

I have now added a message saying that the user can skip linting by passing --nolint to arc diff. So it's not a hard requirement, but it is a little disruptive. I can make it silently fail by returning an exit code of 0 instead of 1, but I don't feel like it's a good idea.

[dberris]

.arcanist/linters/clang-format.sh

13–14

I would push back on this a bit -- let's make it noisy when you don't have clang-format installed, but not stop the upload flow and provide a way to silence the warning. When we (llvm-dev) decide that we will require clang-format'ing all new patches through arcanist/Phabricator then we can change the default.

.arclint

9

I don't see the change to support .cc files...

[starsid]

Make the lint flow completely non-disruptive when clang-format-diff is not installed.

[starsid]

PTAL. I have updated the revision summary to demonstrate how the flows will look like.

.arclint

9

I think this regex captures all the files we want to see -- "(\\.(cc|cpp|h)$)" -- no?

[dberris]

One concern here would be whether this would even work on Windows, and whether it might make more sense to use a more portable scripting language to do it instead (say, Python). I don't use Windows so I'll let someone else who does (and uses Arcanist as well) to chime in as to whether this is going to be a problem.

.arclint

9

Ah, yes. I was looking at an old version of the diff.

[starsid]

In D49116#1158292, @dberris wrote:

One concern here would be whether this would even work on Windows, and whether it might make more sense to use a more portable scripting language to do it instead (say, Python). I don't use Windows so I'll let someone else who does (and uses Arcanist as well) to chime in as to whether this is going to be a problem.

I don't have a Windows machine to test, but maybe someone can port the bash script to python, while testing it on Windows. I don't mind if we don't commit this revision now, but someone takes over, and changes it to work on Windows.

[danilaml]

Would it make clang-format linker available to Phabricator automatically or it would still be required to be run manually by those submitting the diffs?

[starsid]

In D49116#1230759, @danilaml wrote:

Would it make clang-format linker available to Phabricator automatically or it would still be required to be run manually by those submitting the diffs?

People will need to install it once manually but it will run automatically on every arc diff.

[JonasToth]

Drive-by +1, really would like to see clang-format and linters integrated for automatic nitting.

[starsid]

In D49116#1370993, @JonasToth wrote:

Drive-by +1, really would like to see clang-format and linters integrated for automatic nitting.

The revision is up for grabs. Last I checked, the only issue was that I did not have a Windows machine to test the change.

[JonasToth]

In D49116#1371143, @starsid wrote:

In D49116#1370993, @JonasToth wrote:

Drive-by +1, really would like to see clang-format and linters integrated for automatic nitting.

The revision is up for grabs. Last I checked, the only issue was that I did not have a Windows machine to test the change.

@MyDeveloperDay your on windows right? If you have a moment, could you check that out? :)

[MyDeveloperDay]

I run on Windows 10 with cygwin

I made a whitespace change using notepad++ in llvm/lib/CommandLine.cpp, then ran

arc lint

I get the following

$ arc lint
>>> Lint for lib/Support/CommandLine.cpp:


   Auto-Fix  (S&RX) Lint
    clang-format suggested style edits found:

             273
             274   void printOptionValues();
             275
    >>> -    276   void
        -    277     registerCategory(OptionCategory *cat) {
        +          void registerCategory(OptionCategory *cat) {
             278     assert(count_if(RegisteredOptionCategories,
             279                     [cat](const OptionCategory *Category) {
             280              return cat->getName() == Category->getName();
             281            }) == 0 &&
--- /buildareas/clang/llvm/lib/Support/CommandLine.cpp  2019-01-25 14:07:42.608422800 +0000
+++ /tmp/659mblmvmukogwc4/21064-Sz7Mx7  2019-01-25 14:08:06.033582000 +0000
@@ -273,8 +273,7 @@

   void printOptionValues();

-  void
-    registerCategory(OptionCategory *cat) {
+  void registerCategory(OptionCategory *cat) {
     assert(count_if(RegisteredOptionCategories,
                     [cat](const OptionCategory *Category) {
              return cat->getName() == Category->getName();


    Apply this patch to lib/Support/CommandLine.cpp? [Y/n] n

 OKAY  No lint warnings.

If I try and run this in cmd.exe I get

$ cmd
Microsoft Windows [Version 10.0.17134.523]
(c) 2018 Microsoft Corporation. All rights reserved.

C:\cygwin64\Repos\clang\llvm\lib\Support>arc lint
PHP Warning:  require_once(/Repos/clang/llvm/lib/Support/__init_script__.php): failed to open stream: No such file or directory in /Repos/clang/llvm/lib/Support/C:\Repos\arcanist\arcanist\bin\..\scripts\arcanist.php on line 6
PHP Fatal error:  require_once(): Failed opening required '/Repos/clang/llvm/lib/Support/__init_script__.php' (include_path='.:/usr/share/pear:/usr/share/php/php') in /Repos/clang/llvm/lib/Support/C:\cygwin64\Repos\arcanist\arcanist\bin\..\scripts\arcanist.php on line 6

C:\Repos\clang\llvm\lib\Support>

but this is more likely because arc help doesn't even work inside the cmd prompt... (I don't use cmd.exe)

c:\cygwin64\Repos\clang\llvm\lib\Support>arc help
PHP Warning:  require_once(/Repos/clang/llvm/lib/Support/__init_script__.php): failed to open stream: No such file or directory in /Repos/clang/llvm/lib/Support/c:\cygwin64\Repos\arcanist\arcanist\bin\..\scripts\arcanist.php on line 6
PHP Fatal error:  require_once(): Failed opening required '/Repos/clang/llvm/lib/Support/__init_script__.php' (include_path='.:/usr/share/pear:/usr/share/php/php') in /Repos/clang/llvm/lib/Support/c:\cygwin64\Repos\arcanist\arcanist\bin\..\scripts\arcanist.php on line 6

If its important that it works in cmd.exe (why would anyone develop inside cmd.exe!!) then I can try and get it working..

[MyDeveloperDay]

Slight aside there is already a large amount of things in LLVM that don't work in the cmd.exe prompt (or even sometimes in cygwin), so I personally wouldn't set that as a gate for this.

However once you add a .arclint file in the root then I believe all arc diff will run the lint UNLESS you do arc diff --no-lint, but that's probably been mentioned before.

That could easily catch people out.. but there are workarounds using arc alias (https://secure.phabricator.com/Q113)

@JonasToth we could have a got at making an .arclint configuration that would run the validate_check.py with the --rst option to pass a rst filename

[JonasToth]

@JonasToth we could have a got at making an .arclint configuration that would run the validate_check.py with the --rst option to pass a rst filename

Didn't get that one :D But yes, adding the validate_check.py (and maybe even more) to .arclint would be great. But lets wait first if we get fallout from this patch (like people complaining) and then continue :)

[scott.linder]

+1 from me too. It is a little annoying that diffs can become less clear when old code is touched and gets reformatted along with a real change, but I think the alternative of (nearly) ever commit being preceded by an NFC change to fix surrounding formatting is even worse.

[starsid]

Based on the comments, I don't think the revision needs more work. It is just waiting for an accept.

[scott.linder]

I don't know if I'm the right person to approve this, but it seems like there is pretty reasonable consensus that this is a good thing, and with Harbormaster beginning to mark formatting errors with big scary red X's I think landing this sooner rather than later will cut down on some of the review noise.

I left a small nit, but I think the script looks good as-is.

utils/arcanist/clang-format.sh

13 ↗

(On Diff #154938)

This could be changed to use hash, which is a bit shorter and means you skip the PATH lookup later.

i.e. if ! hash clang-format-diff 2>/dev/null

[starsid]

Thank you. I made the change as per your nit. Now landing the change.

[starsid]

use hash instead of command -v

[starsid]

In D49116#1945129, @starsid wrote:

Thank you. I made the change as per your nit. Now landing the change.

Actually, I can't land. Someone with write permissions will have to land.

[scott.linder]

I'll land this today.

[mehdi_amini]

utils/arcanist/clang-format.sh

13 ↗

(On Diff #253665)

I have clang-format installed, I can run git clang-format as well, but I don't have a clang-format-diff executable and I don't even know how to get it.

However I see clang/tools/clang-format/clang-format-diff.py in the repo, can we call this directly?

[mehdi_amini]

utils/arcanist/clang-format.sh

43 ↗

(On Diff #253665)

Can we just use git clang-format indeed?

46 ↗

(On Diff #253665)

This does not seem relevant now that we're on GitHub.

[starsid]

utils/arcanist/clang-format.sh

13 ↗

(On Diff #253665)

Sure, clang-format-diff was a suggestion from the last code review. I don't think git-clang-format was available at the time. Please feel free to send an update to people to get it approved and landed.

43 ↗

(On Diff #253665)

I think so. Please send a review.

46 ↗

(On Diff #253665)

Can be replaced with an error message I believe.

[mehdi_amini]

utils/arcanist/clang-format.sh

13 ↗

(On Diff #253665)

Do you mind addressing all these now? You just landed it I'd expect you to be in position to do this.

[starsid]

utils/arcanist/clang-format.sh

13 ↗

(On Diff #253665)

Sent D77363: Use git-clang-format as Arcanist linter.

[stellaraccident]

In D49116#1158298, @starsid wrote:

In D49116#1158292, @dberris wrote:

One concern here would be whether this would even work on Windows, and whether it might make more sense to use a more portable scripting language to do it instead (say, Python). I don't use Windows so I'll let someone else who does (and uses Arcanist as well) to chime in as to whether this is going to be a problem.

I don't have a Windows machine to test, but maybe someone can port the bash script to python, while testing it on Windows. I don't mind if we don't commit this revision now, but someone takes over, and changes it to work on Windows.

I use arcanist on Windows via msys2, and out of the box, this does indeed fail to launch clang-format.sh (giving the normal error about Windows not knowing how to execute that kind of file -- sorry, the specific shell history is gone now). I've been passing --nolint and typing in a warning every time, which is a bit annoying. I suspect that since arcanist is a native windows command, it is trying to launch the script "natively", whereas if arcanist is installed through some other means (cygwin? msys2 package?), this might have a better time.

The next time I do some arcanist patches work on Windows, I'll see if I can fiddle with it. But in general, +1 on python vs bash as likely being more portable.

[rnk]

This doesn't work on Windows:

$ arc diff
Linting...
 Exception
Some linters failed:
    - CommandException: Command failed with error #1!
      COMMAND
      utils/arcanist/clang-format.sh "lld/COFF/Driver.cpp"

      STDOUT
      (empty)

      STDERR
      'utils' is not recognized as an internal or external command,
      operable program or batch file.

(Run with `--trace` for a full exception trace.)

I would complain that one should not assume bash exists when writing portable code, but now that LLVM has git as a dependency, we can assume bash exists. I have a fix I can send along.

.arcanist/linters/clang-format.sh

+#!/bin/bash
+
+set -euo pipefail
+
+# Arcanist linter that invokes clang-format.
+# stdout from this script is parsed into a regex and used by Arcanist.
+# https://secure.phabricator.com/book/phabricator/article/arcanist_lint_script_and_regex/
+
+if ! command -v clang-format-diff >/dev/null; then
+  RED='\033[0;31m'
+  NC='\033[0m' # No Color
+  echo -e "${RED}Please install clang-format-diff for arcanist to lint C/C++ source code.${NC}" >&2
+  exit 1
+fi
+
+src_file="${1}"
+original_file="$(mktemp)"
+formatted_file="$(mktemp)"
+readonly src_file
+readonly original_file
+readonly formatted_file
+cp -p "${src_file}" "${original_file}"
+cp -p "${src_file}" "${formatted_file}"
+
+cleanup() {
+  rc=$?
+  rm "${formatted_file}" "${original_file}"
+  exit ${rc}
+}
+trap 'cleanup' INT HUP QUIT TERM EXIT
+
+# Arcanist can filter out lint messages for unchanged lines, but for that, we
+# need to generate line by line list messages. Instead, we generate one lint
+# message on line 1, char 1 with file content edited using clang-format-diff.
+if git rev-parse --git-dir >/dev/null; then
+  arc_base_commit=$(arc which --show-base)
+  # An alternative is to use git-clang-format.
+  git diff -U0 --no-color "${arc_base_commit}"| clang-format-diff -style LLVM -i -p1
+else
+  svn diff --diff-cmd=diff -x -U0 "${src_file}" | clang-format-diff -style LLVM -i
+fi
+
+cp -p "${src_file}" "${formatted_file}"
+cp -p "${original_file}" "${src_file}"
+if ! diff -q "${src_file}" "${formatted_file}" > /dev/null ; then
+  echo "#clang-format suggested style edits found:"
+  echo "1,1"  # line,char of start of replacement.
+  cat "${src_file}"
+  echo ">>>>"
+  cat "${formatted_file}"
+fi

.arclint

+{
+  "linters": {
+    "clang-format": {
+      "type": "script-and-regex",
+      "script-and-regex.script": ".arcanist/linters/clang-format.sh",
+      "script-and-regex.regex": "/^(?P<autofix>#)(?P<message>.*?)\n(?P<line>\\d),(?P<char>\\d)\n(?P<original>.*)>>>>\n(?P<replacement>.*)$/s",
+      "include": [
+        "(\\.(cpp|h)$)"
+      ]
+    }
+  }
+}