Add semicolon preference to formatter options #33402

andrewbranch · 2019-09-12T22:47:06Z

Corresponding VS Code PR: microsoft/vscode#80828

Adds a first-class formatter option for optional semicolons, with three options:

Ignore (default): Don’t touch semicolons or lackthereof.
Insert: Add a real semicolon wherever ASI would.
Remove: Remove semicolons that are optional due to ASI.

Because new and reprinted nodes must come out of the writer with semicolons by default, a setting of “Ignore” will try to pick whether to keep or remove semicolons based on the current file content. (This is the same behavior as currently exists via #31801/#32903, but uses the formatter instead of a wrapper around EmitTextWriter. The change here is that the “smart” behavior can be disabled by picking a preference in the formatter.)

I tried this out on various files in the TypeScript codebase including checker.ts, validating by swapping our ESLint semicolon rule from always to never. The formatter successfully removed all semicolons and the file was lint-free, and same for doing the reverse.

typescript-bot · 2019-09-12T22:47:20Z

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

amcasey · 2019-09-12T22:51:32Z

FYI @jessetrinity, who's thinking about how to consume this from VS

andrewbranch · 2019-09-12T23:52:46Z

src/services/formatting/rules.ts

-            rule("NoSpaceBeforeTypeAnnotation", anyToken, SyntaxKind.ColonToken, [isOptionDisabledOrUndefined("insertSpaceBeforeTypeAnnotation"), isNonJsxSameLineTokenContext, isTypeAnnotationContext], RuleAction.Delete),
+            rule("NoSpaceBeforeTypeAnnotation", anyToken, SyntaxKind.ColonToken, [isOptionDisabledOrUndefined("insertSpaceBeforeTypeAnnotation"), isNonJsxSameLineTokenContext, isTypeAnnotationContext], RuleAction.DeleteTrivia),
+            rule("NoOptionalSemicolon", SyntaxKind.SemicolonToken, anyTokenIncludingEOF, [optionEquals("semicolons", SemicolonPreference.Remove), isSemicolonDeletionContext], RuleAction.DeleteToken),
+            rule("OptionalSemicolon", anyToken, anyTokenIncludingEOF, [optionEquals("semicolons", SemicolonPreference.Insert), isSemicolonInsertionContext], RuleAction.TrailingSemicolon),


Perf note: surprisingly, setting anyToken for both tokens doesn’t make too bad of an impact. I had thought about coming back to this and doing some optimization, but it doesn’t seem like it would be a good return on investment. Running “Format Document” on checker.ts with this rule enabled is under two seconds, which is just barely noticeably longer than it took in 3.6. (~~We don’t log timings on formatting, so my numbers here are unscientific. I would have added timings if the experience felt bad.~~ Update: I’ve been told we have them somewhere. They didn’t show up in TSServer logs with normal verbosity.)

.vscode/settings.json

src/services/formatting/formatting.ts

amcasey

Mostly questions for my own edification.
Edit: I'll bet you're wondering what my questions are. I'll let you know as soon as I figure out how to get Code to publish them.

amcasey

I probably had more questions, but I'm only willing to retype them so many times...

src/compiler/core.ts

amcasey · 2019-09-16T23:27:38Z

src/compiler/core.ts

-        return arg => f(arg) || g(arg);
+    export function or<T extends unknown>(...fs: ((arg: T) => boolean)[]): (arg: T) => boolean {
+        return arg => {
+            for (const f of fs) {


How do you choose between for-of, for-increment, and forEach?

My personal tendencies:

for-of: lets you early return enclosing function, doesn’t give you element index

for-increment: generally only use when element index is important, or if I need to start somewhere other than the first element

forEach: gives you early termination of iteration but not return of the enclosing function, result transformation, and element index. It’s generally replaceable by for-of but is more elegant if you return something, like

const firstMatchingSignature = forEach(types, type => { return firstDefined(type.getCallSignatures(), somePredicateThing); });

Array.prototype.forEach: no early termination, no return value 👎

arg => forEach(fs, f => f(arg)) || false?

Yeah, that would work. I think I also have a tendency to implement abstractions with building blocks that are less abstract than the function I’m implementing, if that makes sense. or and forEach are both fairly low-level atomic abstractions, so it makes sense to me that neither would depend on the other, but would rather be built with primitive loops. (I’ve never been consciously aware of this thought process before now; it’s typically just driven by a vague intuition.)

I guess, in my mind, if or can straightforwardly be reduced to forEach it is higher-level. forEach also seems like the sort of thing that might eventually be performance-tuned and it would be nice to pick up the benefits automatically. I don't feel strongly about it though.

src/server/protocol.ts

andrewbranch · 2019-09-23T16:48:03Z

I’m starting to feel like doing this through the existing formatter/rule infrastructure was an awkwardly roundabout approach, and it would be cleaner to add some kind of “formatter preprocessors” concept where a preprocessor operates similarly to a codefix; it gets some context (selection/range/file, formatter options) and returns some TextChanges that get applied before the main formatter (which could continue to operate exclusively on whitespace) runs. (Although separate preprocessors could potentially step on each others’ toes, which could get messy. One advantage of the approach I’ve taken here is that conflicting rules trying to take effect at the same position are properly excluded.)

andrewbranch · 2019-09-25T00:37:02Z

Update: this now removes the last semicolon from single-line object-like types (that are run through the formatter—doesn’t effect compiler emit):

- type X = { w: string; x: { y: { z: string; }; }; };
+ type X = { w: string; x: { y: { z: string } } };

(To be more specific, when the semicolon preference is “I generally like semicolons,” it removes them from synthesized nodes that get inserted and it doesn’t try to insert them into pre-existing user code on “format document.”)

This reverts commit 5625cb0.

andrewbranch · 2019-09-25T17:27:09Z

Update: this now removes the last semicolon from single-line object-like types (that are run through the formatter—doesn’t effect compiler emit):

Update: Reverted this because I think it makes more sense to do this in the emitter. Will probably look at that in a separate PR later.

sandersn

One comment so far; just finished reading tests.

tests/cases/fourslash/formatAddSemicolons1.ts

sandersn

I don't know this area of the code too well so I have mostly questions and low-level suggestions. One possible bug.

src/services/textChanges.ts

src/services/formatting/rulesMap.ts

src/services/formatting/formatting.ts

src/services/formatting/rules.ts

thermarthae · 2019-11-09T15:15:45Z

Update: this now removes the last semicolon from single-line object-like types (that are run through the formatter—doesn’t effect compiler emit):

Update: Reverted this because I think it makes more sense to do this in the emitter. Will probably look at that in a separate PR later.

@andrewbranch Are there any plans to make it work in the near future?

andrewbranch · 2019-11-11T20:10:20Z

@thermarthae thanks, opened #35042 to track 👍

andrewbranch added 16 commits September 5, 2019 14:58

Add UserPreferences for semicolons

df151ac

Prototype formatter semicolon removal

7cca9f4

Implement semicolon insertion

461e6ad

Fix existing tests

0d96d40

Start adding tests

7cae3cd

Fix some edge cases of semicolon deletion

e27bc96

Fix semicolon removal before comments

ae9a234

Fix indentation

0ac2807

Test on checker

4027433

Replace semicolon-omitting writer with formatter preference

149eaf6

Fix writing new nodes, update protocol

4555bb8

Rename option

ac30127

Really fix formatting synthetic nodes

ca8b8f0

Fix refactoring misses

9c33590

Merge branch 'master' into feature/semicolon-preference

d5d23a2

Un-update submodules gahhhh

5743d76

andrewbranch mentioned this pull request Sep 12, 2019

[typescript-language-features] Add formatter option for semicolons microsoft/vscode#80828

Merged

andrewbranch requested review from sheetalkamat, sandersn, DanielRosenwasser, orta, rbuckton, RyanCavanaugh and weswigham September 12, 2019 22:54

Update APIs

d36155f

andrewbranch commented Sep 12, 2019

View reviewed changes

orta added the Update Docs on Next Release Indicates that this PR affects docs label Sep 13, 2019

Update for ESLint

047b55f

weswigham reviewed Sep 13, 2019

View reviewed changes

.vscode/settings.json Outdated Show resolved Hide resolved

.vscode/settings.json Outdated Show resolved Hide resolved

Revert accidental test change

9bd7d54

andrewbranch commented Sep 13, 2019

View reviewed changes

src/services/formatting/formatting.ts Outdated Show resolved Hide resolved

De-kludge deduplication of EOF processing

2c1eef2

amcasey reviewed Sep 16, 2019

View reviewed changes

andrewbranch mentioned this pull request Sep 20, 2019

Await insert break ASI #33522

Closed

andrewbranch added 2 commits September 24, 2019 10:32

Merge branch 'master' into feature/semicolon-preference

4675779

Omit last element semicolon from single-line object-like types

5625cb0

Revert "Omit last element semicolon from single-line object-like types"

525e32e

This reverts commit 5625cb0.

sandersn self-assigned this Sep 25, 2019

Fix straggler test

498a320

sandersn requested changes Sep 25, 2019

View reviewed changes

tests/cases/fourslash/formatAddSemicolons1.ts Show resolved Hide resolved

Add test for leading semicolon class members

68cf56d

andrewbranch commented Sep 25, 2019

View reviewed changes

tests/cases/fourslash/formatAddSemicolons1.ts Show resolved Hide resolved

sandersn requested changes Sep 26, 2019

View reviewed changes

andrewbranch added 2 commits September 26, 2019 11:47

Rename a lot of stuff for clarity

623b4b3

Invert some boolean logic

61c3e61

sandersn approved these changes Sep 26, 2019

View reviewed changes

andrewbranch merged commit 7d9b22e into microsoft:master Sep 26, 2019

andrewbranch deleted the feature/semicolon-preference branch September 26, 2019 20:23

andrewbranch mentioned this pull request Nov 11, 2019

Drop emit of last semicolon in single-line object literal types #35042

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add semicolon preference to formatter options #33402

Add semicolon preference to formatter options #33402

andrewbranch commented Sep 12, 2019 •

edited

Loading

typescript-bot commented Sep 12, 2019

amcasey commented Sep 12, 2019

andrewbranch Sep 12, 2019 •

edited

Loading

amcasey left a comment •

edited

Loading

amcasey left a comment

amcasey Sep 16, 2019

andrewbranch Sep 17, 2019

amcasey Sep 17, 2019

andrewbranch Sep 23, 2019 •

edited

Loading

amcasey Sep 24, 2019

andrewbranch commented Sep 23, 2019

andrewbranch commented Sep 25, 2019

andrewbranch commented Sep 25, 2019

sandersn left a comment

sandersn left a comment

thermarthae commented Nov 9, 2019

andrewbranch commented Nov 11, 2019

Add semicolon preference to formatter options #33402

Add semicolon preference to formatter options #33402

Conversation

andrewbranch commented Sep 12, 2019 • edited Loading

typescript-bot commented Sep 12, 2019

amcasey commented Sep 12, 2019

andrewbranch Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

amcasey left a comment • edited Loading

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

amcasey Sep 16, 2019

Choose a reason for hiding this comment

andrewbranch Sep 17, 2019

Choose a reason for hiding this comment

amcasey Sep 17, 2019

Choose a reason for hiding this comment

andrewbranch Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

amcasey Sep 24, 2019

Choose a reason for hiding this comment

andrewbranch commented Sep 23, 2019

andrewbranch commented Sep 25, 2019

andrewbranch commented Sep 25, 2019

sandersn left a comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

thermarthae commented Nov 9, 2019

andrewbranch commented Nov 11, 2019

andrewbranch commented Sep 12, 2019 •

edited

Loading

andrewbranch Sep 12, 2019 •

edited

Loading

amcasey left a comment •

edited

Loading

andrewbranch Sep 23, 2019 •

edited

Loading