Agent: handle `null` values where we previously only handled `undefined` #4286

sqs · 2024-05-24T11:29:56Z

To prevent bugs when communicating with peers implemented in different programming languages cross-process RPC protocols must not differentiate between undefined, ECMAScript property presence (hasOwnProperty), and null. Anywhere that a value can be undefined, our code needs to handle null values or no such property existing, and all must be handled in the same way (and vice versa).

If a property's TypeScript type includes any of null, undefined, or ? (optional field), then it must use all of them in its TypeScript type. All of those possible TypeScript types must be handled.

This makes it impossible to accidentally only check if (message.foo === undefined) { ... in cases where message came from a peer that may have sent message.foo === null.

Also adds tests that parse all of the protocol .ts files to report any regressions.

Closes https://linear.app/sourcegraph/issue/CODY-1829/handle-null-values-in-the-protocol.

Test plan

CI

sqs · 2024-05-24T11:32:25Z

I don't see any obvious bugs this fixes, but it would have prevented the bugs fixed in #4279, and it will hopefully prevent new bugs like that.

sqs · 2024-05-24T11:33:16Z

agent/src/AgentWorkspaceDocuments.ts

@@ -86,20 +86,6 @@ export class AgentWorkspaceDocuments implements vscode_shim.WorkspaceDocuments {
            document.underlying.visibleRange = fromCache.protocolDocument.visibleRange
        }

-        // The client may send null values that we convert to undefined here.


These if-statements are now guaranteed to be false, but I understand if you'd like to keep them in, @olafurpg, from the battle scars.

These if statements should be removed, I added them only out of paranoia

olafurpg · 2024-05-24T13:50:57Z

agent/src/AgentWorkspaceDocuments.ts

@@ -66,7 +66,7 @@ export class AgentWorkspaceDocuments implements vscode_shim.WorkspaceDocuments {
            const changes = applyContentChanges(fromCache, document.contentChanges)
            contentChanges.push(...changes.contentChanges)
            document.underlying.content = changes.newText
-        } else if (typeof document.content === 'string') {
+        } else if (document.content !== undefined) {


Hmmm, this looks like a behavioral change because document.content can be be null. I need to review this diff more closely, only have ~40 more minutes today and need to land a PR in JB. Will come back to this PR on Monday!

Suggested change

} else if (document.content !== undefined) {

} else if (document.content) {

Would it work if we just check if document.content is true or not?

The protocol type changes I made forced this change in ProtocolTextDocumentWithUri.content: https://github.com/sourcegraph/cody/pull/4286/files#diff-9a8aa7b97ec7b062f534e08aac6f2964faf42c55cfa790267eba4f81387d3b1bR51

So it's now:

public get content(): string | undefined { return this.underlying.content ?? undefined }

So, yes, we can certainly check against null as well to be safe, but in this case it made more sense for the code in ProtocolTextDocumentWithUri.content that sits between AgentWorkspaceDocuments and the prototol typings to perform that ?? undefined and make it so it can never be null.

@abeatrix I'm not sure if that would break when files are the empty string. That would work, but it's unnecessary.

But if you both have battle scars from issues and want to keep more extra checks in, that's fine!

@sqs oh no, no battle scars for me YET. I was simply asking if if (document.content) { would work here to address @olafurpg 's behavioral change comment where document.content could be null 😄

Good point on the empty string. Maybe we can check the length so that it will catch cases where content is undefined | null | "" ?

Suggested change

} else if (document.content !== undefined) {

} else if (document.content?.length) {

Just a thought though! Will defer to @olafurpg since he's more familiar with agent!

:)

It should never be able to be null anymore, though, and that is now well enforced at runtime (not just by assuming the peer follows the protocol types correctly). So, it's not that checking for null would hurt, it's just that it's unnecessary here because:

I made the protocol types (that describe the values received from the JetBrains Kotlin code) basically have this field be typed as content?: string | undefined | null

This immediately made ProtocolTextDocumentWithUri.content have typechecker errors because now it was trying to return a value string | undefined | null as string | undefined...

Which meant I added (and needed to do so or else the typechecker would fail) the ?? undefined to ensure its value is never null.

And if you hover over that value in your editor (the content in if (document.content !== undefined) {, you'll see that it can't be null:

And we can trust this more because of the protocol type changes I made.

@sqs You're right here! I only skimmed the diff super quickly on Friday since I was deep working on other stuff.

To prevent bugs when communicating with peers implemented in different programming languages cross-process RPC protocols must not differentiate between `undefined`, ECMAScript property presence (`hasOwnProperty`), and `null`. Anywhere that a value can be `undefined`, our code needs to handle `null` values or no such property existing, and all must be handled in the same way (and vice versa). If a property's TypeScript type includes any of `null`, `undefined`, or `?` (optional field), then it must use *all* of them in its TypeScript type. All of those possible TypeScript types must be handled. This makes it impossible to accidentally only check `if (message.foo === undefined) { ...` in cases where `message` came from a peer that may have sent `message.foo === null`. Also adds tests that parse all of the protocol `.ts` files to report any regressions. Closes https://linear.app/sourcegraph/issue/CODY-1829/handle-null-values-in-the-protocol.

olafurpg

LGTM 👍🏻 👍🏻 Awesome work @sqs this is great!

olafurpg · 2024-05-25T18:31:40Z

agent/src/AgentWorkspaceDocuments.ts

@@ -86,20 +86,6 @@ export class AgentWorkspaceDocuments implements vscode_shim.WorkspaceDocuments {
            document.underlying.visibleRange = fromCache.protocolDocument.visibleRange
        }

-        // The client may send null values that we convert to undefined here.


These if statements should be removed, I added them only out of paranoia

olafurpg · 2024-05-25T18:33:04Z

vscode/src/chat/protocol.ts

@@ -50,7 +50,7 @@ export type WebviewMessage =
           */
          command: 'event'
          eventName: string
-          properties: TelemetryEventProperties | undefined
+          properties?: TelemetryEventProperties | undefined | null


Is | undefined needed when using ?:?

I'm fine keeping it, but I don't think I would have added them myself.

If | undefined is only added for the linter, then it's possible to check for .questionToken on ts.PropertyDeclaration like here https://github.com/sourcegraph/scip-typescript/blob/olafurpg/signatures/src/FileIndexer.ts#L460C1-L462C75

olafurpg · 2024-05-25T18:40:40Z

vscode/src/jsonrpc/protocol.test.ts

+import * as ts from 'typescript'
+import { describe, expect, test } from 'vitest'
+
+describe('protocol null/undefined robustness', () => {


😍 Love this!

I was reviewing the diff thinking we need a linter to prevent future regressions, only to scroll down to the bottom of the diff and see you already hooked into tsc to do the static analysis. Well done!

Double-checked that this catches unwanted undefined properties that don't accept | undefined

keegancsmith · 2024-05-26T08:27:58Z

vscode/src/jsonrpc/TextDocumentWithUri.ts

@@ -48,18 +48,18 @@ export class ProtocolTextDocumentWithUri {
    }

    public get content(): string | undefined {
-        return this.underlying.content
+        return this.underlying.content ?? undefined


is this value being the empty string meaningful?

sqs requested review from olafurpg and a team May 24, 2024 11:29

sqs commented May 24, 2024

View reviewed changes

olafurpg reviewed May 24, 2024

View reviewed changes

sqs force-pushed the sqs/protocol-null branch from ca0e853 to cb594e7 Compare May 25, 2024 03:50

sqs force-pushed the sqs/protocol-null branch from cb594e7 to 0fd23be Compare May 25, 2024 04:03

olafurpg approved these changes May 25, 2024

View reviewed changes

olafurpg changed the title ~~make protocol types robust to ?|undefined|null~~ Agent: handle null values where we previously only handled undefined May 25, 2024

olafurpg merged commit 8d50564 into main May 25, 2024
20 of 21 checks passed

olafurpg deleted the sqs/protocol-null branch May 25, 2024 18:41

olafurpg mentioned this pull request May 25, 2024

Reseting document content properties is happening too late in the code #4296

Merged

keegancsmith reviewed May 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Agent: handle `null` values where we previously only handled `undefined` #4286

Agent: handle `null` values where we previously only handled `undefined` #4286

sqs commented May 24, 2024

sqs commented May 24, 2024

sqs May 24, 2024

olafurpg May 25, 2024

olafurpg May 24, 2024

abeatrix May 24, 2024

sqs May 24, 2024

abeatrix May 24, 2024

sqs May 24, 2024

olafurpg May 25, 2024

olafurpg left a comment

olafurpg May 25, 2024

olafurpg May 25, 2024

olafurpg May 25, 2024

olafurpg May 25, 2024

keegancsmith May 26, 2024

	} else if (document.content !== undefined) {
	} else if (document.content) {

	} else if (document.content !== undefined) {
	} else if (document.content?.length) {

Agent: handle null values where we previously only handled undefined #4286

Agent: handle null values where we previously only handled undefined #4286

Conversation

sqs commented May 24, 2024

Test plan

sqs commented May 24, 2024

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

olafurpg 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

Agent: handle `null` values where we previously only handled `undefined` #4286

Agent: handle `null` values where we previously only handled `undefined` #4286