PSA: Moving from closure_linter to clang-format in JS code

[Dan Beam]

Hey Chromium JavaScripters!

Summary
I removed closure_linter from Chromium yesterday.  It's been causing presubmits (locally or on CQ) to fail in certain cases for a while now.

Background

closure_linter is a tool that informs of style errors in JavaScript code and recommends a fix.  It's been run on lots of JavaScript on every presubmit for a few years now.  However, closure_linter hasn't seen a lot of love lately and has had various issues over the years:

• thinks Promise#catch() is a switch statement
• dealt with Promise<> poorly
• doesn't recognize }.bind()
• doesn't recognize Polymer properties
• needed to be updated for various new @jsDoc tags
• doesn't recognize =>
• doesn't recognize `template strings`

Local hacks^Wfixes were applied; however folks have been asking to remove the linter for a while.

Alternatively, clang-format is a tool that just reformats for you, which saves developer time.  It's also maintained actively and has good ES6 support (so far).

I ran clang-format on part of Chrome's JS as an experiment and it seemed to be fast and manageable.  I'm hoping to do this for every part of the code the linter previously ran on.  Any help is appreciated.

Want to help migrate?

You can re-use existing .clang-format files (example), or add your own.  Then, run this command on some code you're comfortable changing:

  $ tools/clang-format-js <paths_to_clang_format...>

Publish your CL with BUG=567770 (or make a blocking bug).

Can I ensure the code stays formatted?

Yes!  Add an input_api.canned_checks.CheckPatchFormatted() line to your PRESUBMIT.py (like this).

Does this mess up the blame?

Not if you use `git hyper-blame -i <rev_to_ignore>`.

Format your future,

-- Dan

[Nico Weber]

On Thu, Dec 22, 2016 at 4:24 PM, Dan Beam <db...@chromium.org> wrote:

Hey Chromium JavaScripters!

Summary
I removed closure_linter from Chromium yesterday.  It's been causing presubmits (locally or on CQ) to fail in certain cases for a while now.

Background

closure_linter is a tool that informs of style errors in JavaScript code and recommends a fix.  It's been run on lots of JavaScript on every presubmit for a few years now.  However, closure_linter hasn't seen a lot of love lately and has had various issues over the years:

• thinks Promise#catch() is a switch statement
• dealt with Promise<> poorly
• doesn't recognize }.bind()
• doesn't recognize Polymer properties
• needed to be updated for various new @jsDoc tags
• doesn't recognize =>
• doesn't recognize `template strings`

Local hacks^Wfixes were applied; however folks have been asking to remove the linter for a while.

Alternatively, clang-format is a tool that just reformats for you, which saves developer time.  It's also maintained actively and has good ES6 support (so far).

I ran clang-format on part of Chrome's JS as an experiment and it seemed to be fast and manageable.  I'm hoping to do this for every part of the code the linter previously ran on.  Any help is appreciated.

Want to help migrate?

You can re-use existing .clang-format files (example), or add your own.  Then, run this command on some code you're comfortable changing:

  $ tools/clang-format-js <paths_to_clang_format...>

Publish your CL with BUG=567770 (or make a blocking bug).

You could also just turn on the presubmit without reformatting the existing code. The presubmit only checks lines that have been touched on your current branch.

 

Can I ensure the code stays formatted?

Yes!  Add an input_api.canned_checks.CheckPatchFormatted() line to your PRESUBMIT.py (like this).

Does this mess up the blame?

Not if you use `git hyper-blame -i <rev_to_ignore>`.

If you do reformat stuff, add the reformatting git hash to src/.git-blame-ignore-revs, then hyper-blame without any arguments will do the right thing.

 

Format your future,

-- Dan

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Dan Beam]

On Thu, Dec 22, 2016 at 6:38 PM, Nico Weber <tha...@chromium.org> wrote:

On Thu, Dec 22, 2016 at 4:24 PM, Dan Beam <db...@chromium.org> wrote:

Hey Chromium JavaScripters!

Summary
I removed closure_linter from Chromium yesterday.  It's been causing presubmits (locally or on CQ) to fail in certain cases for a while now.

Background

closure_linter is a tool that informs of style errors in JavaScript code and recommends a fix.  It's been run on lots of JavaScript on every presubmit for a few years now.  However, closure_linter hasn't seen a lot of love lately and has had various issues over the years:

• thinks Promise#catch() is a switch statement
• dealt with Promise<> poorly
• doesn't recognize }.bind()
• doesn't recognize Polymer properties
• needed to be updated for various new @jsDoc tags
• doesn't recognize =>
• doesn't recognize `template strings`

Local hacks^Wfixes were applied; however folks have been asking to remove the linter for a while.

Alternatively, clang-format is a tool that just reformats for you, which saves developer time.  It's also maintained actively and has good ES6 support (so far).

I ran clang-format on part of Chrome's JS as an experiment and it seemed to be fast and manageable.  I'm hoping to do this for every part of the code the linter previously ran on.  Any help is appreciated.

Want to help migrate?

You can re-use existing .clang-format files (example), or add your own.  Then, run this command on some code you're comfortable changing:

  $ tools/clang-format-js <paths_to_clang_format...>

Publish your CL with BUG=567770 (or make a blocking bug).

You could also just turn on the presubmit without reformatting the existing code. The presubmit only checks lines that have been touched on your current branch.

Yeah, might as well give it a crack first.  If I understand correctly, sub-dirs can add a .clang-format file with DisableFormat: true, which is what I was planning for parts of the code (that'll soon be updated and dropped).

 

 

Can I ensure the code stays formatted?

Yes!  Add an input_api.canned_checks.CheckPatchFormatted() line to your PRESUBMIT.py (like this).

Does this mess up the blame?

Not if you use `git hyper-blame -i <rev_to_ignore>`.

If you do reformat stuff, add the reformatting git hash to src/.git-blame-ignore-revs, then hyper-blame without any arguments will do the right thing.

Awesome.  Will do.

-- Dan

[Nico Weber]

On Thu, Dec 22, 2016 at 9:58 PM, Dan Beam <db...@chromium.org> wrote:

On Thu, Dec 22, 2016 at 6:38 PM, Nico Weber <tha...@chromium.org> wrote:

On Thu, Dec 22, 2016 at 4:24 PM, Dan Beam <db...@chromium.org> wrote:

Hey Chromium JavaScripters!

Summary
I removed closure_linter from Chromium yesterday.  It's been causing presubmits (locally or on CQ) to fail in certain cases for a while now.

Background

closure_linter is a tool that informs of style errors in JavaScript code and recommends a fix.  It's been run on lots of JavaScript on every presubmit for a few years now.  However, closure_linter hasn't seen a lot of love lately and has had various issues over the years:

• thinks Promise#catch() is a switch statement
• dealt with Promise<> poorly
• doesn't recognize }.bind()
• doesn't recognize Polymer properties
• needed to be updated for various new @jsDoc tags
• doesn't recognize =>
• doesn't recognize `template strings`

Local hacks^Wfixes were applied; however folks have been asking to remove the linter for a while.

Alternatively, clang-format is a tool that just reformats for you, which saves developer time.  It's also maintained actively and has good ES6 support (so far).

I ran clang-format on part of Chrome's JS as an experiment and it seemed to be fast and manageable.  I'm hoping to do this for every part of the code the linter previously ran on.  Any help is appreciated.

Want to help migrate?

You can re-use existing .clang-format files (example), or add your own.  Then, run this command on some code you're comfortable changing:

  $ tools/clang-format-js <paths_to_clang_format...>

Publish your CL with BUG=567770 (or make a blocking bug).

You could also just turn on the presubmit without reformatting the existing code. The presubmit only checks lines that have been touched on your current branch.

Yeah, might as well give it a crack first.  If I understand correctly, sub-dirs can add a .clang-format file with DisableFormat: true, which is what I was planning for parts of the code (that'll soon be updated and dropped).

Please don't add a ton of .clang-format files all over the place. We should have ~1 in the root dir, not tens of them.

[Daniel Cheng]

+1 to fewer .clang-formats. I saw https://codereview.chromium.org/2603443002/diff/60001/chrome/browser/resources/.clang-format land recently, and of the 8 options set in there, 6 are just setting things to the default Chromium style values.

Of the two that aren't just using the default values:

-  AllowShortFunctionsOnASingleLine should be in the top-level config, scoped to JS. Assuming this is the style change we want, we should make this change upstream in https://github.com/llvm-mirror/clang/blob/master/lib/Format/Format.cpp#L634

- JavaScriptQuotes makes sense to scope to this directory, given the issues with changing quoting. Unfortunately, having a .clang-format in this directory means that we'll have to duplicate the config option for AllowShortFunctionsOnASingleLine until we fix it upstream and roll clang-format.

Daniel

[Dan Beam]

I'm not currently trying to making global choices for all JavaScript in Chrome because I don't know how similar WebUI (my experience) code is to that of file manager or mojo (for example) nor who's using clang-format on JS in Chrome.  I can start a separate thread about this before changing src/.clang-format or clang-format's "Chromium" JS defaults.

I am interested in clang-formatting code that was previously checked by closure linter.  The linter only ran on specific parts of the codebase because:

a) nobody fixed all the errors the tool flagged when adding it, so it never felt right to turn on and add extra work for others

b) people couldn't agree on its usefulness (i.e. some folks dropped it or refused to use it)

c) it just broke for many reasons and incentive was low to change or fix it

I've added 2 .clang-format files to the tree, but they're really 1 as they'll never differ.  I just haven't found a way to have only 1 canonical file yet.

+1 to fewer .clang-formats. I saw https://codereview.chromium.org/2603443002/diff/60001/chrome/browser/resources/.clang-format land recently, and of the 8 options set in there, 6 are just setting things to the default Chromium style values.

Removes defaults and adds a comment about keeping both files the same content:

https://codereview.chromium.org/2609453002/

I do not wish to reformat the code for UIs that will soon be replaced.  .clang-format files with DisableFormat: true allows running clang-format at a higher level while specific subdirectories should (theoretically) be ignored.  The alternative is to cherry-pick specific places to run clang-format on / use PRESUBMIT blacklists.  This does not prevent folks from wasting their time reformatting e.g. deprecated code that we don't want to change.

Of the two that aren't just using the default values:

-  AllowShortFunctionsOnASingleLine should be in the top-level config, scoped to JS. Assuming this is the style change we want, we should make this change upstream in https://github.com/llvm-mirror/clang/blob/master/lib/Format/Format.cpp#L634

- JavaScriptQuotes makes sense to scope to this directory, given the issues with changing quoting.

That should be fixed by my grit patch soon, btw.

 

Unfortunately, having a .clang-format in this directory means that we'll have to duplicate the config option for AllowShortFunctionsOnASingleLine until we fix it upstream and roll clang-format.

Yep, it'd be cool to have a file: directive like OWNERS files to share configs.

-- Dan

[Nico Weber]

There's no need to reformat a directory before adding a formatting presubmit. The presubmit only checks that lines that have been touched are formatted, and `git cl format` only formats lines that have already been touched. What's more, clang-format's behavior, for better or worse, changes in details from time to time, so even if you auto-format all your code now, a future clang-format might format it differently. (Since presubmit and `git cl format` only look at touched lines, this isn't a problem in practice.) So I don't think there's any need for DisableFormat files.

file: references in .format files are only needed if a) there are multiple .format files over a long time b) they are more than a line or two long. We shouldn't do either of these things, so at least for chrome we hopefully won't need this feature.

We should have a single global JS style for stuff in src, that's kind of the point of an autoformatter. Like in c++, unless there's some really compelling reason, that style should probably be the same as standard Google style. Do you know why default Google JS style doesn't set AllowFunsSingleLine to true? If that's intentional, we probably shouldn't set it either.

[Dan Beam]

On Wed, Dec 28, 2016 at 1:44 PM, Nico Weber <tha...@chromium.org> wrote:

There's no need to reformat a directory before adding a formatting presubmit. The presubmit only checks that lines that have been touched are formatted, and `git cl format` only formats lines that have already been touched. What's more, clang-format's behavior, for better or worse, changes in details from time to time, so even if you auto-format all your code now, a future clang-format might format it differently. (Since presubmit and `git cl format` only look at touched lines, this isn't a problem in practice.) So I don't think there's any need for DisableFormat files.

That's nice, and maybe I'll try turning on presubmit enforcement without formatting.

However, not pre-fixing stuff just means that I'm deferring style arguments or problems with clang-format until somebody touches the code.  The point of a style guide (and tool to enforce it) is really just to save comprehension and time for developers.  Bumping up against tools, especially at inconvenient times, goes against this and might give developers a negative impression.  Pre-solving common issues and pre-formatting the code seems to help save folks time.

For example, after running clang-format I've found that it poorly handles:

* <if> and <include>

* code using automatic semi-colon insertion (ASI)

I expected complications with <if>/<include>.  They're GRIT-specific, generally weird, and both closure compiler and the linter previously need[ed] special casing for this.

There are bugs filed on the ASI issues internally, but the verdict was to just ignore them and add semicolons.  ASI is not a Good Part of JS (in my opinion), so I'm happy to fix these cases.  However, if I changed some code and got an insane diff from `git cl format` (which will happen for ASI), it'd be pretty hard to figure out it's because a semi-colon is missing.

file: references in .format files are only needed if a) there are multiple .format files over a long time b) they are more than a line or two long. We shouldn't do either of these things, so at least for chrome we hopefully won't need this feature.

We should have a single global JS style for stuff in src, that's kind of the point of an autoformatter. Like in c++, unless there's some really compelling reason, that style should probably be the same as standard Google style.

Seems like folks have found a lot of compelling reasons over time:
https://cs.chromium.org/search/?q=%5C.clang-format&sq=package:chromium&type=cs

Various parts of Chrome's JS came from / were written with different style (i.e. parts of closure library, chromevox, print preview, many third_party projects, etc.).  It's generally a mixed bag as to whether the JS you're currently editing is formatted like:

* Chromium's C++ (most of webui)

* Google's JS (print preview, chromevox, closure)

* something totally different, i.e. the outside world (third_party, probably not clang-formatting this)

I don't want to block clang-formatting JS on finding a central set of agreed-upon rules as I'm not sure we'll come to consensus easily.  Running clang-format (previewing future required changes) has received a bit of push back already.

Do you know why default Google JS style doesn't set AllowFunsSingleLine to true? If that's intentional, we probably shouldn't set it either.

Google's JS style sets AllowShortFunctionsOnASingleLine: Empty, which is promptly overridden by Chromium's style, but this is probably just an oversight as Chromium's style doesn't differentiate between C++ and JS.  This probably hasn't mattered much until now, and we should probably make getChromiumStyle() aware of JS (like you did for Java a while back).

-- Dan

[Daniel Cheng]

On Wed, Dec 28, 2016 at 3:04 PM Dan Beam <db...@chromium.org> wrote:

On Wed, Dec 28, 2016 at 1:44 PM, Nico Weber <tha...@chromium.org> wrote:

There's no need to reformat a directory before adding a formatting presubmit. The presubmit only checks that lines that have been touched are formatted, and `git cl format` only formats lines that have already been touched. What's more, clang-format's behavior, for better or worse, changes in details from time to time, so even if you auto-format all your code now, a future clang-format might format it differently. (Since presubmit and `git cl format` only look at touched lines, this isn't a problem in practice.) So I don't think there's any need for DisableFormat files.

That's nice, and maybe I'll try turning on presubmit enforcement without formatting.

However, not pre-fixing stuff just means that I'm deferring style arguments or problems with clang-format until somebody touches the code.  The point of a style guide (and tool to enforce it) is really just to save comprehension and time for developers.  Bumping up against tools, especially at inconvenient times, goes against this and might give developers a negative impression.  Pre-solving common issues and pre-formatting the code seems to help save folks time.

For example, after running clang-format I've found that it poorly handles:

* <if> and <include>

* code using automatic semi-colon insertion (ASI)

I expected complications with <if>/<include>.  They're GRIT-specific, generally weird, and both closure compiler and the linter previously need[ed] special casing for this.

There are bugs filed on the ASI issues internally, but the verdict was to just ignore them and add semicolons.  ASI is not a Good Part of JS (in my opinion), so I'm happy to fix these cases.  However, if I changed some code and got an insane diff from `git cl format` (which will happen for ASI), it'd be pretty hard to figure out it's because a semi-colon is missing.

file: references in .format files are only needed if a) there are multiple .format files over a long time b) they are more than a line or two long. We shouldn't do either of these things, so at least for chrome we hopefully won't need this feature.

We should have a single global JS style for stuff in src, that's kind of the point of an autoformatter. Like in c++, unless there's some really compelling reason, that style should probably be the same as standard Google style.

Seems like folks have found a lot of compelling reasons over time:
https://cs.chromium.org/search/?q=%5C.clang-format&sq=package:chromium&type=cs

Within the Chromium repository, there (were) only three .clang-format files checked in:

$ git ls-files | grep "\.clang-format"

.clang-format

third_party/WebKit/Source/devtools/.clang-format

third_party/crashpad/crashpad/.clang-format

So I think that as guideline, that's actually holding up quite well in Chromium.

 

Various parts of Chrome's JS came from / were written with different style (i.e. parts of closure library, chromevox, print preview, many third_party projects, etc.).  It's generally a mixed bag as to whether the JS you're currently editing is formatted like:

* Chromium's C++ (most of webui)

* Google's JS (print preview, chromevox, closure)

* something totally different, i.e. the outside world (third_party, probably not clang-formatting this)

I don't want to block clang-formatting JS on finding a central set of agreed-upon rules as I'm not sure we'll come to consensus easily.  Running clang-format (previewing future required changes) has received a bit of push back already.

Can't we just start from Google's JS style guide as a default baseline?

 

Do you know why default Google JS style doesn't set AllowFunsSingleLine to true? If that's intentional, we probably shouldn't set it either.

Google's JS style sets AllowShortFunctionsOnASingleLine: Empty, which is promptly overridden by Chromium's style, but this is probably just an oversight as Chromium's style doesn't differentiate between C++ and JS.  This probably hasn't mattered much until now, and we should probably make getChromiumStyle() aware of JS (like you did for Java a while back).

Sound good to me.

[Nico Weber]

On Wed, Dec 28, 2016 at 5:04 PM, Dan Beam <db...@chromium.org> wrote:

On Wed, Dec 28, 2016 at 1:44 PM, Nico Weber <tha...@chromium.org> wrote:

There's no need to reformat a directory before adding a formatting presubmit. The presubmit only checks that lines that have been touched are formatted, and `git cl format` only formats lines that have already been touched. What's more, clang-format's behavior, for better or worse, changes in details from time to time, so even if you auto-format all your code now, a future clang-format might format it differently. (Since presubmit and `git cl format` only look at touched lines, this isn't a problem in practice.) So I don't think there's any need for DisableFormat files.

That's nice, and maybe I'll try turning on presubmit enforcement without formatting.

However, not pre-fixing stuff just means that I'm deferring style arguments or problems with clang-format until somebody touches the code.  The point of a style guide (and tool to enforce it) is really just to save comprehension and time for developers.  Bumping up against tools, especially at inconvenient times, goes against this and might give developers a negative impression.  Pre-solving common issues and pre-formatting the code seems to help save folks time.

For example, after running clang-format I've found that it poorly handles:

* <if> and <include>

* code using automatic semi-colon insertion (ASI)

Sure, there's definitely value in running clang-format on all code to make sure it actually works. I just mean there's not necessarily a need to check its output in. (I don't think there's a big drawback either, I'm just trying to say if other directories want to use clang-format for their .js, the barrier to entry is actually _lower_ than "run this on all your code".) For Java, I ran it over most .java files locally, fixed all bugs I could see myself, then asked a few dogfooders to use it, then sent a wider "give it a try" email, and now it's used in a few presubmits. We never reformatted all .java files.

I expected complications with <if>/<include>.  They're GRIT-specific, generally weird, and both closure compiler and the linter previously need[ed] special casing for this.

There are bugs filed on the ASI issues internally, but the verdict was to just ignore them and add semicolons.  ASI is not a Good Part of JS (in my opinion), so I'm happy to fix these cases.  However, if I changed some code and got an insane diff from `git cl format` (which will happen for ASI), it'd be pretty hard to figure out it's because a semi-colon is missing.

file: references in .format files are only needed if a) there are multiple .format files over a long time b) they are more than a line or two long. We shouldn't do either of these things, so at least for chrome we hopefully won't need this feature.

We should have a single global JS style for stuff in src, that's kind of the point of an autoformatter. Like in c++, unless there's some really compelling reason, that style should probably be the same as standard Google style.

Seems like folks have found a lot of compelling reasons over time:
https://cs.chromium.org/search/?q=%5C.clang-format&sq=package:chromium&type=cs

cs indexes LLVM's code for some reason, so over half of those are test inputs for clang-format from its test suite. Like Daniel says, almost all of those are in non-src repos (v8, skia, etc), and we used to actually have a single .clang-format file until more got added for .js things. (crashpad is third-party code living in a third-party repo that gets checked in to src periodically, and devtools used to be in a separate repo before we merged in blink).

 

Various parts of Chrome's JS came from / were written with different style (i.e. parts of closure library, chromevox, print preview, many third_party projects, etc.).  It's generally a mixed bag as to whether the JS you're currently editing is formatted like:

* Chromium's C++ (most of webui)

* Google's JS (print preview, chromevox, closure)

* something totally different, i.e. the outside world (third_party, probably not clang-formatting this)

Sure, I'm only saying stuff in src.git should share a single JS style.

 

I don't want to block clang-formatting JS on finding a central set of agreed-upon rules as I'm not sure we'll come to consensus easily.  Running clang-format (previewing future required changes) has received a bit of push back already.

Do you know why default Google JS style doesn't set AllowFunsSingleLine to true? If that's intentional, we probably shouldn't set it either.

Google's JS style sets AllowShortFunctionsOnASingleLine: Empty, which is promptly overridden by Chromium's style, but this is probably just an oversight as Chromium's style doesn't differentiate between C++ and JS.  This probably hasn't mattered much until now, and we should probably make getChromiumStyle() aware of JS (like you did for Java a while back).

Yes, that sounds just like a bug, and setting this does make "Chromium's" JS style more like Google style, so that sounds like a good thing. So setting this temporary in a .clang-format file (like you have done), fixing this upstream, and then removing the local .clang-format files once that's rolled in sounds like a good plan.

[Dan Beam]

I just tried and we still want some aspects of the Chromium defaults:
https://github.com/llvm-mirror/clang/blob/master/lib/Format/Format.cpp#L644-L645

 

Do you know why default Google JS style doesn't set AllowFunsSingleLine to true? If that's intentional, we probably shouldn't set it either.

Google's JS style sets AllowShortFunctionsOnASingleLine: Empty, which is promptly overridden by Chromium's style, but this is probably just an oversight as Chromium's style doesn't differentiate between C++ and JS.  This probably hasn't mattered much until now, and we should probably make getChromiumStyle() aware of JS (like you did for Java a while back).

Sound good to me.

https://reviews.llvm.org/D28165 (we'll see how it goes).

-- Dan

[Dan Beam]

thakis@ helped me tweak the "Chromium" clang-format style defaults for JS here (still needs a roll):

http://llvm.org/viewvc/llvm-project?view=revision&revision=290930

I'm also working to enable formatting presubmits and figure out the existing issues (i.e. improving single vs double quotes in grit) that have come up while running clang-format on parts of the codebase (i.e. net_internals and hotword).

-- Dan