Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[clang-format] Add new option to support adding a space between Javascript Union and Intersection types

Authored by MyDeveloperDay on Jan 13 2022, 12:56 AM.



The following issue highlights a problem where we cannot add a space between A|B when detecting a JSTypeOperator

This patch allows clang-format to be configured to support that (which seems to match most references I can find)

Fixes: #49858

Diff Detail

Event Timeline

MyDeveloperDay requested review of this revision.Jan 13 2022, 12:56 AM
MyDeveloperDay created this revision.
mprobst added inline comments.Jan 13 2022, 12:59 AM

shouldn't you change this line here?

MyDeveloperDay added inline comments.Jan 13 2022, 1:11 AM

You know I thought the same and this was where I first put the code change in, but this code doesn't seem to have any effect and I've been caught out by this before... (anyone else seen that?)

I'm not sure if something has been changed, but I'm finding that often I add code into spaceRequiredBetween() and despite it being executed correctly it doesn't have the desired effect, which is why the code is in spaceRequiredBefore()

mprobst added inline comments.Jan 13 2022, 1:29 AM

Generally we put space between two operators. So this line must have some effect, as otherwise we'd always emit A | B. Given that, I think we need more debugging here to make this change - working around by some more code somewhere else doesn't seem like a good long term approach.

Also, note that this is all in spaceRequiredBefore, right?


Be explicit on the binding.


It's on my plan to refactor these 2 methods, because I think they can and should be made easier to understand and reason about.

So from my point of view one can accept this patch.

curdeius accepted this revision.Jan 13 2022, 2:12 AM

LGTM apart from nits.
I agree though that spaceRequired(Before|Between) need some refactoring love.


Also, why capital letters on union and intersection type?

This revision is now accepted and ready to land.Jan 13 2022, 2:12 AM
curdeius added inline comments.Jan 13 2022, 2:17 AM

Oh, you might need to add a test for this case too.

curdeius requested changes to this revision.Jan 13 2022, 2:27 AM
curdeius added inline comments.

I modified it like this and it passes all the tests. Please recheck.

This revision now requires changes to proceed.Jan 13 2022, 2:27 AM
mprobst added inline comments.Jan 13 2022, 2:28 AM

I appreciate the intention to refactor this in the future.

I still believe we really have to understand how this code works though, and why changing this line doesn't have the effect we'd expect, before we land the fix. Chances are there is some substantial misunderstanding how this all fits together, which usually leads to bugs.

curdeius added inline comments.Jan 13 2022, 2:30 AM


MyDeveloperDay added inline comments.Jan 13 2022, 5:54 AM

@mprobst I do agree, let me take another look.

MyDeveloperDay marked 10 inline comments as done.

Make suggested improvements
Add additional test case

mprobst added inline comments.Jan 14 2022, 1:21 AM

Why do we need this further qualification here? I'd have expect that you can simply "return Style.SpacesInJavaScriptUnion;"? identifier || template closer also sounds oddly specific, why exactly those?

MyDeveloperDay added inline comments.Jan 14 2022, 7:53 AM

Left = template closer, Right =JsTypeOperator is for this case

type Foo = Bar<X> | Baz;

Left = type or identifier, Right =JsTypeOperator is for this case

let x: A | B = A | B;
curdeius accepted this revision.Jan 14 2022, 7:55 AM

Looks ok from my side.

This revision is now accepted and ready to land.Jan 14 2022, 7:55 AM
mprobst added inline comments.Jan 14 2022, 8:42 AM

What about:

let x: {foo: string}&{bar: number};

I'm not sure including curlies would be comprehensive. It might be worth checking the TS grammar.

But coming back, did you try this:

if ( || {
  return Style.SpacesInJavaScriptUnion;

(We might also need to do something in spaceRequiredBefore, maybe that fires earlier?)

shafik added a subscriber: shafik.Sep 26 2023, 2:08 PM

Is this change still relevant or can we close this?

Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2023, 2:08 PM
Herald added a reviewer: rymiel. · View Herald Transcript