Detect semicolons before writing from TextChanges

[andrewbranch]

Fixes #19882

Note that this change affects more than just auto-imports; it should affect all quick fixes that insert new nodes into a file.

Screen captures

The detection algorithm is:

1. Traverse down the source file’s parse tree, stopping to look at nodes that “usually” have a trailing semicolon (which I’ll refer to as a candidate node). I’ve loosely defined this as any grammar production for which the ES2018 spec lists a semicolon that could be a candidate for ASI (i.e., everywhere a semicolon shows up except EmptyStatement and for loop IterationStatements), plus a few TypeScript-specific productions, but not including PropertySignature, since those are commonly separated by commas.
2. If the last token of the candidate node is a semicolon, increment a counter. Otherwise, increment a different counter.
3. Continue down the tree until we’ve looked at 5 such candidate nodes or we’ve reached the end of the file.
4. If both counters are 0, or if the semicolon counter is 0 and the no-semicolon counter is 1, we don’t have enough evidence to make a decision, so we’ll default to assuming semicolons are desired. (If the no-semicolon counter is 1, it doesn’t mean you’ve written a complete statement lacking a semicolon; it probably means you’re in the middle of writing a statement and haven’t gotten to the semicolon yet.)
5. If the ratio of semicolon to no-semicolon statements is 1:4 or higher, you want semicolons. Otherwise, you don’t. The logic is that if you really don’t want semicolons, you are highly unlikely to write any that you don’t need. On the other hand, if you want semicolons but write really fast-and-loose because you’re going to rely on a formatter to fix up your code, you might write a few semicolons but omit even more. It takes a significant absence of semicolons to be able to assume that you really don’t want them.

[orta]

Cool! As a non-semi person, I'm glad to see it

[orta]

I see, these are all tests which use the writer's heuristic for semis - and they all now get the semi because it doesn't have proof that the file isn't using them. Makes sense.

[abumalick]

Thank you very much for working on this @andrewbranch !

[janosh]

@andrewbranch TypeScript 3.6.0 Beta just landed! I'm running it now but semicolons are still added to my imports when auto-sorting them in a file completely free of semicolons. Anything I need to do to activate semicolon detection?

[andrewbranch]

@janosh you’re sure VS Code is actually using 3.6, right? I don’t think I’ve tried it with sorting imports, but given that the change occurred in TextChanges, I would expect it to work 🤔

[janosh]

@andrewbranch Sorry, my mistake. I'd installed the beta via yarn global add typescript@beta and added

"typescript.tsdk": "~/.config/yarn/global/node_modules/typescript/lib"

along with

"editor.codeActionsOnSave": {
  "source.organizeImports": true
}

to my user settings. Trying to save a file still gave me semicolons but only because I'd commented out "prettier.semi": false, for testing a few days earlier and forgotten about it. I take it all back, detecting semicolons works great! 😄

[cancerberoSgx]

@andrewbranch

Thanks for working on the issue although I wonder why is so informal and opinionated instead of being a boolean in FormatCodeSettings or similar. In my case I'm building a code refactor/formatting tool on top of typescript compiler API and the solution is not useful at all.

Shouldn't be users (editor's and API's) the ones deciding this configuration ? Why is the tool detecting and opinionatedly deciding this ?

just that I feel like we’re unlikely to add additional heuristics to TypeScript to support preferences that are atypical and/or more work to test for.

But you just added heuristics that nobody can configure and forEachChild() counting semicolons... IMO you assumed too much about the user in the PR.

I don't agree this project should take decisions based on general preferences or "defacto" standards at all.And sorry to add noise to this worthless conversation, but people maintaining platform/languages like this should think more like engineers and less than artists or lawyers. This problem is as simple as 1) JavaScript has a formal specification regarding semicolons ( no matter the trends) 2) TypeScript must support JavaScript specs 100% 3) If the tool provides an API to configure how semicolons are print then it should apply to all node kinds in a generic way.

Imagine tomorrow I add a cool refactor to move declarations to existing file (which would be awesome and is missing) . Since lots of nodes/files could be potentially changed If I understand your changes, since this only applies to TextChanges, an action could result on my whole project's files being a mixing between semicolon and not semicolon statements ,... And since I don't have a simple flag to control this myself II will have to use an external tool to reformat everything.

I'm testing now the new version to see if this issue was solved in PrinterOptions which has a flag to at least an API for this which was working in the past. Will update with results here. Thanks

[andrewbranch]

@cancerberoSgx what APIs are you using to build your refactoring tool?

the solution is not useful at all

I'm testing now the new version to see if this issue was solved

I’m a little bit confused by the combination of these two statements. How can you be sure that the solution isn’t useful if you haven’t tried it yet? 😄

We’re generally receptive to having our public APIs to be useful to people building cool stuff on top of TypeScript, but it’s a little bit hard to discuss it in hypotheticals. If you find that you have a concrete use case where you’re getting undesirable behavior, I would encourage you to open a new issue with some specifics about what you’re trying to do and what’s not working.

[mjomble]

Replying to @RyanCavanaugh in #19882 (comment):

our belief was that implementing this as a heuristic would actually be the best behavior, since the tool would just "do the right thing" with no necessary configuration from the user. It seems it'd be better if TS simply inserted semicolons in files that seemed to use semicolons so that users didn't have to go find the configuration option.

Seems like a good compromise would be a setting with three options:

• always use semicolons
• never use semicolons
• auto-detect (default)

[RyanCavanaugh]

Nothing stops us from adding that setting in the future

[cancerberoSgx]

@andrewbranch

We’re generally receptive to having our public APIs to be useful to people building cool stuff on top of TypeScript, but it’s a little bit hard to discuss it in hypotheticals.

Is not hypothetical: semicolons are not configurable in CodeFormatSettings parameters in LanguageService methods. The rest of the formatting semantics are supported even some edge cases. As plugin author I can interact with editors , build tools, users to set this semantics ... with the unique exception of semicolons (which is basic).

What I didn't had the time to confirm (that is hypothetical) is if Printer options (transformer API, not this one), which has a property just for this (omitTrailingSemicolon) and was working in the past but no more as some users mention in the issue, is fixed (long time I din't test. But even if this was solved, I think the proper location is together with the rest of style properties in CodeFormatSettings / EditorSettings so consumers doesn't need to use Printer just LanguageService API.

In my case human and machine users are not interested on automatically experiences, they want to configure the style of semicolons and make sure it applies consistently across the project.

I'm currently formatting my code with a handmade tool based on TS that allows me to configure my style with a json like this https://github.com/cancerberoSgx/magica/blob/master/formatCodeSettings.json and after trying lots of technologies for formatting I can say TypeScript is the best one for this, with one exception, semicolons (a boolean) for which I needed to implement my own transformation just for this :(

Prediction, If the intention was to make it easy for users or enhance vscode experience, I think it's wrong. The user "persona" is a JavaScripter probably with strong code style opinions and preferred fools for this in his build workflow. I don't think in general TS/JS users will never want to configure semicolons (which are the an important discussion about style today). And regarding vscode, it speaks for itself, search "format" in the editor settings and you will se a lot of formatting configuration , ... with the exception of semicolons... That's the main reason of the issue if vscode was the motivation. This PR won't solve the problem and settings will still be inconsistent, one aspect behaving automatically and the rest not...

Are you sure the target user "persona" really fit with the "auto magically" concept @mjbvz proposed particularly ? A JavaScript programmer with strong code style opinions in a diverse ecosystem, with knowledge of npm and the command line ? I bet majority of them already have a preferred style and formatting tools to format offline in the built chain.

[RyanCavanaugh]

@cancerberoSgx we're very open to direct feedback on the API here. Some basic examples of things you'd like to accomplish, what you tried, and what didn't work - that would be great and would be likely to result in changes or new functionality being exposed.

It seems like you're approaching this from an impressionistic perspective and that kind of feedback is much less actionable for us. I'm not sure what point you're trying to make discussing external formatting tool chains, etc.. This change was driven directly by user feedback that they expect semicolon insertion to "just work", and that's what we've delivered (so far). The book is by no means closed and we can add more configurability if that's the right thing to do, we just need the feedback to be constructive and directed in order to help you the most.

[andrewbranch]

Yep, just to add to what Ryan said:

I bet majority of them already have a preferred style and formatting tools to format offline in the built chain

Definitely, this is our assumption. Part of the reason we wanted to auto-detect semicolon preference is that there are a lot of different tools already out there: eslint, tslint, editorconfig, prettier, etc. The original issue was full of people suggesting “why not just respect my linter settings,” which makes total sense, but the problem is that it’s a fragile investment to try to understand every linter/formatter config, as these tools come and go and change over time. Personally, I’ve never adjusted the formatting options surfaced in VS Code because I do use tools like prettier and eslint. The thought behind auto-detecting was that we can avoid having to understand the config for N different tools by observing the output they already produce, and have a pretty good guess for folks who aren’t using tools like those at all.

And as others have pointed out, we can still add a tri-state formatter option for on/off/auto. I made this PR with that possibility in mind; I just hadn’t really heard people asking for that before now. (One data point I used is that auto-imports also auto-detect quote preference without a formatter option, and as far as I know, people have been satisfied with that.)

Definitely appreciate all the feedback and I’m happy to continue discussing here, but the reason I said “open a new issue” isn’t at all to be dismissive, it’s just that eventually, feedback on closed PRs and closed issues might slip through the cracks. A new issue is just the best way to make sure we don’t forget about it.

[cancerberoSgx]

@RyanCavanaugh, @andrewbranch

ome basic examples of things you'd like to accomplish, what you tried, and what didn't work - that would be great and would be likely to result in changes or new functionality being exposed.

Sure this is my experience:

I'm building a CLI tool to refactor/format TypeScript code. https://github.com/cancerberoSgx/ts-refactor. Now just for personal use and very WIP but make my point exactly. It could be used interactively or not. I daily use is two transformations : organize imports and "format"

There are other "refactors" implemented and planned and the majority based on language service methods and the built in typescript refactors and codefixes that I'm able to invoke using their names or codes (although I don't know if this are intended to be public Is not so hacky and at the end all goes through serviceLanguage applyX() methods .

In conclusion most of this refactors will be affected by CodeFormatSettings parameter I'm able to pass. I more or less described all I can configure here https://github.com/cancerberoSgx/ts-refactor/blob/master/src/fix/formatTypes.ts (don't laugh) :)

As said I can't configure semicolons using this api so my "format" I needed to separately support removeTrailingSemicolon and addTrailingSemicolon https://github.com/cancerberoSgx/typescript-plugins-of-mine/blob/master/ts-simple-ast-extra/src/refactor/trailingSemicolons.ts

I hope you see how from my point of view is logical / reasonable to give this feedback to you. I don't give this feedback to projects not being a contributor so believe me I know I might sound even disrespectful but currently I don't have much time to seriously /elegantly contribute with a PR for this - perhaps I can make it work but I suspect adding support for semicolons at the language service level will take lots of testing/reviews/discussion/braking things - will try to reserve some time tough but probably won't be able so this feedback is my humble contribution. Thanks! Keep it up !