Improvements for eslint (and try switching to prettier) #3647

silvanshade · 2020-03-18T22:45:31Z

This PR modifies the eslint config to extend default rules for the base linter and the @typescript-eslint plugin, and then it switch from tsfmt to prettier and integrates prettier into the eslint config. This way, formatting issues now show up as linter issues.

I also fixed the lints reported by the stricter config and finally ran prettier on the output.

I don't know if y'all really want to switch from tsfmt to prettier but I figured I'd give it a try. One reason to consider switching, aside from the eslint integration, is it looks like the tsfmt repository may not be maintained anymore (https://github.com/vvakame/typescript-formatter).

Veetaha · 2020-03-18T23:46:08Z

editors/code/src/inlay_hints.ts

-            if (!hints) return;
+        // eslint-disable-next-line @typescript-eslint/no-misused-promises
+        this.sourceFiles.forEach((file, uri) =>
+            this.fetchHints(file).then(hints => {


Suggested change

this.fetchHints(file).then(hints => {

void this.fetchHints(file).then(hints => {

Does this silence the misused promises error?

Actually, it might. I'll have to check that.

Using void did work there.

Veetaha · 2020-03-18T23:53:15Z

editors/code/src/util.ts

 }

-export const log = new class {
+export const log = new (class {


Meh, the rule that requires parentheses for new calls...

Ah, yeah that one is ugly. I'll try to disable it.

Veetaha

Thanks for the cleanup! Left some arguments, tho...

Veetaha · 2020-03-19T00:09:58Z

editors/code/src/ctx.ts

    }

-    registerCommand(name: string, factory: (ctx: Ctx) => Cmd) {
+    registerCommand<T extends unknown[]>(name: string, factory: (ctx: Ctx) => Cmd<T>): void {


Strongly-typing the Cmd doesn't give us many benefits but gives only increased verbosity and repetition (the passed parameters tuple type just unnecessarily duplicates the type of lambda parameters in command handlers). VSCode doesn't provide the strongly-typed interface for commands so we won't get compile errors for them. This is actually one of the rare cases where we should use any as the function parameters type.

And if I could, I'd remove this interface from return types and rely on type inference, so we don't cast strongly-typed lambdas to type-erased Cmd, e.g.:

function createCmdHandler(): Cmd { return (a: number, b: string) => { /**/ } } createCmdHandler()(null, null); // gives no error

vs when infered

function createCmdHandler() { return (a: number, b: string) => { /* */ } } createCmdHandler()(null, null); // compile error

Yeah, I don't disagree with your thoughts about the strongly typed Cmd. In fact, I agree it doesn't really add much in practice and I almost didn't include this change.

I just really kind of dislike using any. It often seems harmless but can lead to surprising and unfortunate bugs from time to time. I suppose if it's just for the definition of Cmd and it only needs the one use of // eslint-disable @typescript-eslint/no-explicit-any, it's not so bad.

One option which might be acceptable would be to just remove the type annotations from the lambda arguments and only specify them in T for Cmd<T>. That way we don't need any, we don't lose any type information (argument types are still inferrable), and it's in the end not much more verbose.

Example:

export function showReferences(ctx: Ctx): Cmd<[string, lc.Position, lc.Location[]]> { return (uri: string, position: lc.Position, locations: lc.Location[]): void => {

would look like

export function showReferences(ctx: Ctx): Cmd<[string, lc.Position, lc.Location[]]> { return (uri, position, locations): void => {

What do you think? If you prefer, I can still switch back to any.

Yeah, I am glad, I found one more like-minded person who is passionate about static typing. I do think that any always leads to bugs. I wish all the typescript code avoided it.

However, it is unavoidable in your code due to someone's bad design decisions (e.g. #3629 (comment), and untyped vscode commands api just forces you to deal with any) and bad language design (e.g. catch error variable is always of any type and there is even no way to redefine it to unknown, but I am getting too far...).

Though I do like relying on return type inference, not everyone likes it. So, in this case, your proposal does seem viable, so we do get rid of the dreaded any at least in our code.

Veetaha · 2020-03-19T00:14:19Z

editors/code/src/highlighting.ts

    const random = randomU32Numbers(hashString(seed));
-    const randomInt = (min: number, max: number) => {
-        return Math.abs(random()) % (max - min + 1) + min;
+    const randomInt = (min: number, max: number): number => {


I do like TypeScript for not forcing you to write types here and there so that the code is very concise but it still provides you with the strong static typing guarantees.
I'd actually even remove all the return type declarations if I had enough impudence, but I'd suggest at least disabling the rule for arrow functions. @matklad ?

Okay, I can disable that. I agree it can be a bit much.

Veetaha · 2020-03-19T00:15:40Z

editors/code/src/rust-analyzer-api.ts

-function request<TParams, TResult>(method: string) {
+function request<TParams, TResult>(
+    method: string,
+): lc.RequestType<TParams, TResult, unknown, never> {


Oh, the verbosity...

Veetaha · 2020-03-19T00:17:04Z

editors/code/src/client.ts

    // Or the ability to disable the serverside component of highlighting (but this means that to do tracing we need to disable hihlighting)
    // This also requires considering our settings strategy, which is work which needs doing
    // @ts-ignore The tracer is private to vscode-languageclient, but we need access to it to not log publishDecorations requests
    res._tracer = {


One way to avoid ts-ignore here at all is

Suggested change

res._tracer = {

res['_tracer'] = {

but ts-ignore kinda brings your attention to the fact that this is a hack.

Veetaha · 2020-03-19T00:17:42Z

editors/code/src/client.ts

+    DocumentSemanticsTokensSignature,
+} from 'vscode-languageclient/lib/semanticTokens.proposed';

-export async function createClient(config: Config, serverPath: string): Promise<lc.LanguageClient> {


Nice catch!

Veetaha · 2020-03-19T00:19:39Z

editors/code/src/installation/server.ts

-            "If you feel that your platform should be supported, please create an issue " +
-            "about that [here](https://github.com/rust-analyzer/rust-analyzer/issues) and we " +
-            "will consider it."
+                'You need to manually clone rust-analyzer repository and ' +


This indentation us unfortunate ;( ...

silvanshade · 2020-03-19T02:14:03Z

I made some changes to the formatting based on your feedback:

I removed duplicated types from some expressions (e.g., expr<T>(...) where T was inferrable)
I tried to remove most of the return types on arrow functions
I left the return annotations for top-level functions (could disable that rule entirely though)
I tried to remove verbosity around Cmd without resorting to any (but could go back if preferred)

I had to leave the new (class ... as is since it seems prettier really wants to parenthesize class expressions. There may be a way to disable that rule but I couldn't find it.

Same for multi-line operator expressions. I just used String.prototype.concat for that text blurb. I know it's supposed to be slower than + but since it's only for a few lines it should be okay.

matklad · 2020-03-19T07:04:27Z

We intentionally use build-in VS Code formatter over prettier

matklad · 2020-03-19T07:15:15Z

Thought which has not occurred to me before: it probably makes sense to just copy-paste config from https://github.com/Microsoft/vscode-languageserver-node/

matklad · 2020-03-19T09:40:20Z

editors/code/.eslintrc.js

-        "@typescript-eslint"
+    plugins: ['@typescript-eslint'],
+    extends: [
+        'eslint:recommended',


We intentionally don't use recommended: #3041

We use it, but the irrelevant rules are explicitly turned off down bellow.

Veetaha · 2020-03-19T13:15:16Z

We intentionally use build-in VS Code formatter over prettier

Could you please elaborate on this, @matklad ?

matklad · 2020-03-19T13:49:36Z

"built in" in my opinion outweighs any benefits of prettier.

Veetaha · 2020-03-19T13:57:02Z

Hmm, I don't use any VSCode formatter, I just run npm run fix before the ultimate commit... So from my side, it doesn't matter what's under the hood

silvanshade · 2020-03-19T18:02:39Z

If you would rather not merge this PR because you find the use of recommended eslint rules or the prettier formatting rules too onerous, that's okay.

If you would like me to split out some of the other changes made, I can do that too.

Otherwise I'm happy to close this out.

We intentionally use build-in VS Code formatter over prettier

@matklad the built-in formatter seems reasonable but it still looks like tsfmt is no longer actively maintained. The last release was in 2018 and it apparently has compatibility issues with newer typescript configurations (see issues about newer flags). It seems likely that it will eventually need to be replaced or removed from the npm scripts.

matklad · 2020-03-20T07:50:00Z

Yeah, I just prefer to keep the nodejs side of things as simple as possible, and generally try to stray away from extra tooling. Given the size of the TypeScript codebase, we are not at the point where complex non-built-in tooling makes sense, imo.

darinmorrison added 6 commits March 18, 2020 16:48

Add typing annotation for eslint config

05000cc

Use recommended eslint and typescript-eslint rules

6472051

Fix lints

512d360

Switch to prettier instead of tsfmt

681b036

Configure eslint to use prettier

dbc069f

Run prettier

8b6ad20

Veetaha reviewed Mar 18, 2020

View reviewed changes

Veetaha reviewed Mar 19, 2020

View reviewed changes

Adjust eslint rules

8be009d

darinmorrison added 2 commits March 18, 2020 20:44

Work on formatting

2cec955

Fix instance of @typescript-eslint/no-misused-promises

c1a4caa

matklad reviewed Mar 19, 2020

View reviewed changes

silvanshade closed this Mar 20, 2020

	this.fetchHints(file).then(hints => {
	void this.fetchHints(file).then(hints => {

Improvements for eslint (and try switching to prettier) #3647

Improvements for eslint (and try switching to prettier) #3647

Uh oh!

Conversation

silvanshade commented Mar 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veetaha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silvanshade Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silvanshade commented Mar 19, 2020

Uh oh!

matklad commented Mar 19, 2020

Uh oh!

matklad commented Mar 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veetaha commented Mar 19, 2020

Uh oh!

matklad commented Mar 19, 2020

Uh oh!

Veetaha commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silvanshade commented Mar 19, 2020

Uh oh!

matklad commented Mar 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

silvanshade Mar 19, 2020 •

edited

Loading

Veetaha commented Mar 19, 2020 •

edited

Loading