Defend HelpCompletion from null message from VSCode

[rjmholt]

Small change to ensure that null messages from VSCode don't cause errors, to fix #1230.

[rkeithhill]

Defensive programming FTW! However I wish we knew why we sometimes get an undefined changeEvent.

[rkeithhill]

Actually, the error is that contentChanges[0] is undefined. I wonder if you need to check for that before accessing the text property.

[rjmholt]

Ah, I see. This particular nasty seems to crop up a bit. I'll see if I consolidate the reports here. I'd love to know what VSCode is sending us (I tried to install Wireshark, but apparently there's extra stuff you need to install on Windows if you want to listen to localhost/loopback...)

[rjmholt]

So in addition to #1230, there is also #1189 and #1043. Do we think they are the same bug?

[rkeithhill]

It certainly looks that way. My only concern is if the dev tools console err msg is just a harmless coincidence instead of what is actually causing the Intellisense problem in #1189.

[TylerLeonhardt]

I tried to install Wireshark, but apparently there's extra stuff you need to install on Windows if you want to listen to localhost/loopback...

@rjmholt maybe fiddler can do this?

[TylerLeonhardt]

watch this fix all of the DocumentChangeEvent bugs lol

LGTM

[rkeithhill]

Don't forget to check contentChanges[0] for undefined. If changeEvent itself was getting thru as undefined then we should have gotten an error on changeEvent.document first.

[rjmholt]

I've added a logger call to document bad messages we receive

[rkeithhill]

So I'm running this change and I see this log message during debugging of the extension:

typeDefinition.js:18
Bad change event message: {"document":{"uri":{"$mid":1,"fsPath":"c:\\Users\\Keith\\Documents\\WindowsPowerShell\\Profile.ps1","external":"file:///c%3A/Users/Keith/Documents/WindowsPowerShell/Profile.ps1","path":"/C:/Users/Keith/Documents/WindowsPowerShell/Profile.ps1","scheme":"file"},"fileName":"c:\\Users\\Keith\\Documents\\WindowsPowerShell\\Profile.ps1","isUntitled":false,"languageId":"powershell","version":2,"isClosed":false,"isDirty":true,"eol":2,"lineCount":31},"contentChanges":[]}

In looking at the docs I see this explanation of the onDidChangeTextDocument() method:

An event that is emitted when a text document is changed. This usually happens when the contents changes but also when other things like the dirty-state changes.

I get this when I add a space to the end of a line but because I have VSCode set to trim trailing whitespace, I get this event but the contentChanges array is empty (0 length). So I think the current code will generate many incorrect "bad change" messages.

Here is what I recommend we change this to:

    public onEvent(changeEvent: TextDocumentChangeEvent): void {
        if (!(changeEvent && changeEvent.contentChanges)) {
            this.log.write(`Bad TextDocumentChangeEvent message: ${JSON.stringify(changeEvent)}`);
            return;
        }

        if (changeEvent.contentChanges.length > 0) {
            this.helpCompletionProvider.updateState(
                changeEvent.document,
                changeEvent.contentChanges[0].text,
                changeEvent.contentChanges[0].range);

            // Todo raise an event when trigger is found, and attach complete() to the event.
            if (this.helpCompletionProvider.triggerFound) {
                this.helpCompletionProvider.complete().then(() => this.helpCompletionProvider.reset());
            }
        }
    }

[rjmholt]

Aha! Man what an edge case! Yeah, I will implement that 😄 Thanks @rkeithhill!

[rkeithhill]

Ya know, given this TypeScript def of TextDocumentChangeEvent:

	export interface TextDocumentChangeEvent {

		/**
		 * The affected document.
		 */
		document: TextDocument;

		/**
		 * An array of content changes.
		 */
		contentChanges: TextDocumentContentChangeEvent[];
	}

Doesn't this mean those two fields can't be null or undefined - cc @daviwil ? Admittedly my TypeScript skillz are still pretty weak so I'm not 100% sure on that. If it is true, we might as well just get rid of that first null check since it should never be invoked.

[TylerLeonhardt]

@rkeithhill good find on the edge case! FWIW, I'm pretty sure and empty array is falsey so changeEvent.contentChanges.length > 0 doesn't need the length check but it's easier to read so I'm cool with that. Just thought I'd point that out.

Also, I'm pretty sure that those two fields can still be null just like in a C# class. I don't think they can be undefined. I'm like 85% confident in this statement.

[rkeithhill]

After a quick experiment, it looks like you're right so leave the null check in. But in the future, C# 8 won't allow null assignment unless the type is defined as nullable. I'm looking forward to this. :-)

[rjmholt]

Yeah, TypeScript's interface explanation page isn't very helpful. I think it's a compile-time check, which doesn't help much from arbitrary deserialised network messages, just gives completions on them. (I think the intent is that you use an interface to describe the JSON you're expecting, then you feed it into a constructor of a full TypeScript object to rehydrate it and give it methods, etc.).

Had no idea C# was planning that — finally we won't have to do null checks all the way down, and hopefully the CLR will be able to use the information to run quicker.

[rkeithhill]

Check out this MSDN article on C# 8 - Nullable Reference Types.

[rkeithhill]

Time to merge this one.