Enable "strict" compiler option #178

BigBahss · 2021-04-18T21:08:09Z

Closes #61.

Changes have been made to adhere to strict type checking:

Non-null assertions and definite-assignment assertions (! post-fix operator) have been added to please the compiler (these do not affect the compiled output). These assertions are not always strictly "safe", but are not any less safe than not having strict type checking at all. Plus, their presence in the code can be a signal that maybe the design should be changed to safely ensure that the types are not null/undefined.
Some optional chaining (?.) has been been added when properties can be null/undefined.
Some incorrect uses of null have been replaced with undefined.
Add defaultValue arguments to WorkspaceConfiguration.get() so that it will not return undefined. The default values being passed are the default values defined in the package.json. Realistically, the function should not return undefined when default values are defined in the package.json, but this pleases the compiler.

The changes I've made were aimed at not changing the behavior of the extension, and weren't necessarily aimed at fixing any potential bugs. Strict type checking will make it less likely to introduce new bugs in future changes.

src/ast.ts

src/semantic-highlighting.ts

sam-mccall

Thanks for doing this cleanup! And sorry if some of these comments are dumb, I'm not deeply familiar with TS's type system. I do think if we're going to enable strict mode then I should understand it though...

src/ast.ts

sam-mccall · 2021-04-23T08:35:24Z

src/ast.ts

@@ -163,16 +164,16 @@ class TreeAdapter implements vscode.TreeDataProvider<ASTNode> {
    return element.children || [];
  }

-  public getParent(node: ASTNode): ASTNode {
+  public getParent(node: ASTNode): ASTNode|undefined {


There are a bunch of places in this class where you've changed null to undefined - is there some fundamental reason why null is incorrect? (It makes sense to be consistent within a module of course.

My understanding is this is mostly a stylistic choice, and my preference would be to use null for "deliberately" null values, but this isn't my primary language...

Sorry if I came across as pompous saying that null was "incorrect", that really wasn't the right word. There were inconsistent usages of null vs undefined which were causing type errors. I leaned towards undefined because generally it seems to be the preferred "bottom value" for typescript, or at least in terms of working with vscode's API's (Plus null has some weird properties like having typeof 'object'. But, it's definitely a preference kind-of-thing whether to use it or not). You're right though, null is used deliberately/explicitly. But, I would make the argument that unless the logic makes a distinction between a variable being undefined vs null, then it's probably a better idea to use one or the other consistently, rather than specifying that something can be SomeType | undefined | null.

No worries at all. I think if undefined is privileged by the ?. operators etc then ergonomics is a good reason to prefer it.
(And it seems so, playground says (null as any)?.foo is undefined)
And this seems to be MS's preference, and it's mostly their APIs we're dealing with.

I agree. So just to confirm, are deciding to use undefined for values stored by the extension and accepting null for incoming values (when necessary)?

That sounds good to me!

sam-mccall · 2021-04-23T08:39:28Z

src/clangd-context.ts

@@ -58,7 +58,7 @@ export class ClangdContext implements vscode.Disposable {
          await config.getSecureOrPrompt<string[]>('arguments', workspaceState),
      options: {cwd: vscode.workspace.rootPath || process.cwd()}
    };
-    const traceFile = config.get<string>('trace');
+    const traceFile = config.get<string>('trace', '');


I think that echoing defaults already given in the package.json is unacceptable duplication (and causes practical problems e.g. someone changing the default in the wrong place).

Can we add a ! assertion in config.get() instead?

That's fair. I believe the defaults can be retrieved through WorkspaceConfiguration.inspect(), but that may be more trouble than it's worth. Realistically adding asserts here would only cause a problem if a default value was removed from package.json, but I expect that's quite unlikely.

src/config.ts

src/install.ts

sam-mccall · 2021-04-23T08:47:11Z

src/memory-usage.ts

@@ -80,8 +80,8 @@ class MemoryUsageFeature implements vscodelc.StaticFeature {
 class TreeAdapter implements vscode.TreeDataProvider<InternalTree> {
  private root_?: InternalTree;

-  get root(): InternalTree|null { return this.root_; }
-  set root(n: InternalTree|null) {
+  get root(): InternalTree|undefined { return this.root_; }


again why these null->undefined changes?

Addressed in a previous comment, but it's because private root_?: InternalTree is already declared as being InternalTree | undefined.

sam-mccall · 2021-04-23T08:48:51Z

tsconfig.json

@@ -16,7 +16,7 @@
        ],
        "sourceMap": true,
        "rootDir": ".",
-        "alwaysStrict": true,
+        "strict": true,


It's surprising that "strict" is more strict than "alwaysStrict" :-)

Yeah, it really seems like a "gcc -Wall" scenario haha.

BigBahss

Addressed comments (further consensus is needed on the null vs undefined issue).

src/ast.ts

BigBahss · 2021-04-23T10:53:47Z

src/ast.ts

@@ -163,16 +164,16 @@ class TreeAdapter implements vscode.TreeDataProvider<ASTNode> {
    return element.children || [];
  }

-  public getParent(node: ASTNode): ASTNode {
+  public getParent(node: ASTNode): ASTNode|undefined {


Sorry if I came across as pompous saying that null was "incorrect", that really wasn't the right word. There were inconsistent usages of null vs undefined which were causing type errors. I leaned towards undefined because generally it seems to be the preferred "bottom value" for typescript, or at least in terms of working with vscode's API's (Plus null has some weird properties like having typeof 'object'. But, it's definitely a preference kind-of-thing whether to use it or not). You're right though, null is used deliberately/explicitly. But, I would make the argument that unless the logic makes a distinction between a variable being undefined vs null, then it's probably a better idea to use one or the other consistently, rather than specifying that something can be SomeType | undefined | null.

BigBahss · 2021-04-23T11:01:40Z

src/clangd-context.ts

@@ -58,7 +58,7 @@ export class ClangdContext implements vscode.Disposable {
          await config.getSecureOrPrompt<string[]>('arguments', workspaceState),
      options: {cwd: vscode.workspace.rootPath || process.cwd()}
    };
-    const traceFile = config.get<string>('trace');
+    const traceFile = config.get<string>('trace', '');


That's fair. I believe the defaults can be retrieved through WorkspaceConfiguration.inspect(), but that may be more trouble than it's worth. Realistically adding asserts here would only cause a problem if a default value was removed from package.json, but I expect that's quite unlikely.

src/install.ts

BigBahss · 2021-04-23T11:11:42Z

src/memory-usage.ts

@@ -80,8 +80,8 @@ class MemoryUsageFeature implements vscodelc.StaticFeature {
 class TreeAdapter implements vscode.TreeDataProvider<InternalTree> {
  private root_?: InternalTree;

-  get root(): InternalTree|null { return this.root_; }
-  set root(n: InternalTree|null) {
+  get root(): InternalTree|undefined { return this.root_; }


Addressed in a previous comment, but it's because private root_?: InternalTree is already declared as being InternalTree | undefined.

BigBahss · 2021-04-23T11:13:03Z

tsconfig.json

@@ -16,7 +16,7 @@
        ],
        "sourceMap": true,
        "rootDir": ".",
-        "alwaysStrict": true,
+        "strict": true,


Yeah, it really seems like a "gcc -Wall" scenario haha.

BigBahss · 2021-04-23T23:53:56Z

I was going crazy there for a minute wondering how the CI was failing on a non-existent file when the tests passed on my machine 😅. Anyways I merged and modified the new file for strict type checking.

I fixed all the things that were mentioned. Additionally I changed all == and != to === and !== since that's very closely related to strict type checking. Let me know if I missed anything or whether you have further concerns.

sam-mccall

Thanks a lot!

BigBahss · 2021-04-26T08:39:23Z

Happy to help 👍

Enable "strict" compiler option

fdad202

BigBahss commented Apr 18, 2021

View reviewed changes

src/ast.ts Outdated Show resolved Hide resolved

src/semantic-highlighting.ts Show resolved Hide resolved

sam-mccall reviewed Apr 23, 2021

View reviewed changes

BigBahss commented Apr 23, 2021

View reviewed changes

BigBahss added 3 commits April 23, 2021 19:21

Address comments and change double-equals to triple-equals

555ac22

Merge remote-tracking branch 'upstream/master' into enable-strict

e37c86c

Modify open-config.ts to adhere to strict type checking

0b539b3

sam-mccall approved these changes Apr 26, 2021

View reviewed changes

sam-mccall merged commit 6ccdd94 into clangd:master Apr 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable "strict" compiler option #178

Enable "strict" compiler option #178

BigBahss commented Apr 18, 2021

sam-mccall left a comment

sam-mccall Apr 23, 2021

BigBahss Apr 23, 2021

sam-mccall Apr 23, 2021

BigBahss Apr 23, 2021

sam-mccall Apr 23, 2021

sam-mccall Apr 23, 2021

BigBahss Apr 23, 2021

sam-mccall Apr 23, 2021

BigBahss Apr 23, 2021

sam-mccall Apr 23, 2021

BigBahss Apr 23, 2021

BigBahss left a comment

BigBahss Apr 23, 2021

BigBahss Apr 23, 2021

BigBahss Apr 23, 2021

BigBahss Apr 23, 2021

BigBahss commented Apr 23, 2021

sam-mccall left a comment

BigBahss commented Apr 26, 2021

Enable "strict" compiler option #178

Enable "strict" compiler option #178

Conversation

BigBahss commented Apr 18, 2021

sam-mccall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigBahss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigBahss commented Apr 23, 2021

sam-mccall left a comment

Choose a reason for hiding this comment

BigBahss commented Apr 26, 2021