Our website uses cookies to enhance your browsing experience.
No more tiptoeing, we can finally say it out loud: we've released a static code analyzer for JavaScript and TypeScript. So naturally, it's time to send it out into the wild and see what it catches in the source code of Visual Studio Code, an open-source project most developers know all too well. Here's what turned up.
This section is for anyone who isn't familiar with our article format, so you know what to expect.
At PVS-Studio, we build a static code analysis tool. Basically, a static analyzer is a program that looks for logical errors in the source code of other programs. For this kind of article, we pick a popular open-source project, run our analyzer on it, and share what we find with our readers, as well as with the project's developers, by filing issues and pull requests.
With the current release, we're adding a new analyzer for JavaScript and TypeScript. Right now it runs on a basic set of diagnostic rules, but it's already capable of catching real errors and flagging suspicious code.
To show it tackling something real, we picked a project pretty much everyone's heard of, Visual Studio Code. It combines JavaScript and TypeScript code, and the codebase is quite large, so we were curious to see what our newcomer would find in it.
We ran the analysis on release version 1.128.0, the latest version at the time of writing, at commit fc3def677. We cloned the repo, ran the analysis, and once it finished, picked out the most interesting results. Now for the results.
In this section, and in the ones that follow, you'll see a code snippet containing either an outright error or code that looks off and is worth a second look, followed by the PVS-Studio warning and a link to the source in the repository. Right after that, we'll explore what's wrong with the actual code.
The first error for today is a rather extravagant way to access an array:
....
const diffs = changes.map(splice => {
return [splice[0], splice[1], splice[2].map(....)]
as [number, number, CellViewModel[]];
});
....
for (let i = 0; i < diffs.length; i++) {
const diff = diffs[0]; // <=
if (diff[0] + diff[1] <= primarySelectionIndex) {
delta += diff[2].length - diff[1];
continue;
}
if (diff[0] > primarySelectionIndex) {
endSelectionHandles = [primaryHandle];
break;
}
if (diff[0] + diff[1] > primarySelectionIndex) {
endSelectionHandles = [this._viewCells[diff[0] + delta].handle];
break;
}
}
....The PVS-Studio warning: V7016 Suspicious access to an element of the 'diffs' object by a constant index inside a loop. notebookViewModelImpl.ts 252
Here, we have an array being built, followed by an attempt to loop through it. Why an attempt? The i index is never actually used within the array, instead, the line marked with the comment // <= always pulls the element at index 0. As a result, on every single iteration, checking diff means dealing with the first element of the diffs collection.
Judging by the snippet itself, that wasn't the intent. The plan was clearly to check each i-th element, but a typo is a typo.
This is a recurring pattern across other languages too. Our Java analyzer, for instance, has caught similar issues before.
So we're moving on.
....
let lastValueAtPosition: boolean | undefined = undefined;
let lastValueOnLine: boolean | undefined = undefined;
timeouts.add(autorun(reader => {
const newValueAtPosition =
s.source.isPresentAtPosition(args.position, reader);
const newValueOnLine =
s.source.isPresentOnLine(args.position.lineNumber, reader);
if ( lastValueAtPosition !== undefined
&& lastValueAtPosition !== undefined) { // <=
if (!lastValueAtPosition && newValueAtPosition) {
trigger(s.property, s.source, 'positional');
}
if (!lastValueOnLine && newValueOnLine) {
trigger(s.property, s.source, 'line');
}
}
lastValueAtPosition = newValueAtPosition;
lastValueOnLine = newValueOnLine;
}));
....The PVS-Studio warning: V7001 The operands of the '&&' operator in the 'lastValueAtPosition !== undefined && lastValueAtPosition !== undefined' expression are equivalent. editorTextPropertySignalsContribution.ts 152
The code has two variables, lastValueAtPosition and lastValueOnLine, both typed as boolean or undefined. And in the arrow function they're passed into, the intent was to first check that neither one is undefined, and only then treat them as boolean values.
There's a typo in the condition the analyzer flagged: the binary expression && uses identical operands. As a result, lastValueOnLine never gets checked against undefined, meaning the second nested if statement can behave incorrectly.
The assignment operator comes with its share of sneaky typos, here's the first one.
The first snippet:
swapChildren(from: number, to: number): void {
from = validateIndex(from, this.children.length);
to = validateIndex(to, this.children.length);
if (from === to) {
return;
}
this.splitview.swapViews(from, to);
// swap boundary sashes
[this.children[from].boundarySashes,
this.children[to].boundarySashes] =
[this.children[from].boundarySashes,
this.children[to].boundarySashes]; // <=
// swap children
[this.children[from], this.children[to]] =
[this.children[to], this.children[from]];
this.onDidChildrenChange();
}The PVS-Studio warning: V7005 The expression '[this.children[from].boundarySashes, this.children[to].boundarySashes]' is assigned to itself. gridview.ts 575
In this code, the goal was to use destructuring to swap the values of this.children[from].boundarySashes and this.children[to].boundarySashes. Destructuring is a handy way to do this kind of swap without introducing a third variable. But for it to actually work, the values on the right side of the assignment needed to be in the opposite order. As it stands, the // swap boundary sashes comment doesn't match what the code actually does.
By the way, a similar swap happens further down, with different values, and that one doesn't have this typo.
The second snippet:
export class LanguageModelTextPart implements vscode.LanguageModelTextPart2 {
value: string;
audience: vscode.LanguageModelPartAudience[] | undefined;
constructor(value: string, audience?: vscode.LanguageModelPartAudience[]) {
this.value = value;
audience = audience; // <=
}
toJSON() {
return {
$mid: MarshalledId.LanguageModelTextPart,
value: this.value,
audience: this.audience,
};
}
}The PVS-Studio warning: V7005 The variable 'audience' is assigned to itself. extHostTypes.ts 4033
Another assignment typo, but in a different context this time. There's a constructor parameter named audience and a class field also named audience. When assigning a value to the audience field, the code forgot the this keyword. As a result, the audience field never gets initialized.
The third snippet:
export class ServerInstalledExtensionsView extends ExtensionsListView {
override async show(query: string): Promise<IPagedModel<IExtension>> {
query = query ? query : '@installed';
if (....) {
query = query += ' @installed'; // <=
}
return super.show(query.trim());
}
}The PVS-Studio warning: V7005 The variable 'query' is assigned to itself. extensionsViews.ts 1310
This one's similar, but it's hardly an error, more of a typo or an odd use of the += operator. The += operator already assigns the new value to query on its own, so the query = .... part in front of it is redundant.
In this particular spot, it's very unlikely to cause any real trouble. But the same kind of typo, if it landed on a plain = assignment meant for a different variable, could actually cause problems. And beyond that, a new developer reading this code would have to stop and figure out whether there's a real bug here or whether the redundant assignment is just harmless. That's reason enough not to leave code like this as is.
There're similar warnings in the project:
private onFocusChanged(event: ITableEvent<ITunnelItem>) {
if (event.indexes.length > 0 && event.elements.length > 0) {
this.lastFocus = [...event.indexes];
}
const elements = event.elements;
const item = elements && elements.length ? elements[0] : undefined;
if (item) {
this.tunnelViewSelectionContext.set(
makeAddress(item.remoteHost, item.remotePort));
this.tunnelTypeContext.set(item.tunnelType);
this.tunnelCloseableContext.set(!!item.closeable);
this.tunnelPrivacyContext.set(item.privacy.id);
this.tunnelProtocolContext.set(item.protocol === TunnelProtocol.Https
? TunnelProtocol.Https
: TunnelProtocol.Https); // <=
....
}
}The PVS-Studio warning: V7012 [CWE-1041] The conditional expression 'item.protocol === TunnelProtocol.Https ? TunnelProtocol.Https : TunnelProtocol.Https' always returns the same value. tunnelView.ts 997
Here, the analyzer flags that both branches of the ternary return the same value, TunnelProtocol.Https. We checked TunnelProtocol, and sure enough, the else branch should be returning TunnelProtocol.Http instead.
Interestingly, we'd already flagged this one ourselves, and while double-checking it before writing this article, we noticed that a few days after our check, one of VS Code's users had spotted the same bug independently. Here's a link to their pull request. This is a good example of static analysis catching errors before they make it into the master branch.
Another warning for V7012 also flagged the following snippet:
....
if (this.selection.endColumn <= this.targetPosition.column) {
// The target position is after the selection's end position
this.targetSelection = new Selection(
this.targetPosition.lineNumber - this.selection.endLineNumber
+ this.selection.startLineNumber,
this.selection.startLineNumber === this.selection.endLineNumber
? this.targetPosition.column - this.selection.endColumn
+ this.selection.startColumn
: this.targetPosition.column - this.selection.endColumn
+ this.selection.startColumn, // <=
this.targetPosition.lineNumber,
this.selection.startLineNumber === this.selection.endLineNumber ?
this.targetPosition.column :
this.selection.endColumn
);
}
....The PVS-Studio warning: V7012 [CWE-1041] The conditional expression always returns the same value. dragAndDropCommand.ts 86
Here too, the analyzer points out identical expressions in the branches of the ternary operator, but this one isn't as clear-cut as the last. It looks like the then and else branches probably aren't meant to be identical, but what one of them should actually contain is anyone's guess. This is a case where the fix should come from someone who fully understands what this code is meant to do.
....
const lineCount = model.getLineCount();
const endLine = lineNumber === lineCount;
const prevLineEmptyOrIndented =
lineNumber > 1 && isLineEmptyOrIndented(lineNumber - 1);
const nextLineEmptyOrIndented =
!endLine && isLineEmptyOrIndented(lineNumber + 1);
const currLineEmptyOrIndented = isLineEmptyOrIndented(lineNumber);
const notEmpty = !nextLineEmptyOrIndented && !prevLineEmptyOrIndented;
// check above and below. if both are blocked, display lightbulb in the gutter.
if (!nextLineEmptyOrIndented && !prevLineEmptyOrIndented && !hasDecoration) {
this._gutterState.set(....);
this.renderGutterLightbub();
return this.hide();
} else if (prevLineEmptyOrIndented || endLine ||
(prevLineEmptyOrIndented && !currLineEmptyOrIndented)) { // <=
effectiveLineNumber -= 1;
}
....The PVS-Studio warning: V7018 The expression 'prevLineEmptyOrIndented || (prevLineEmptyOrIndented && ...)' is redundant and always evaluates to 'prevLineEmptyOrIndented'. lightBulbWidget.ts 373
In this snippet, the analyzer caught an odd pattern in the condition. The warning itself points out that if you set aside the second subcondition, || endLine, what's left is prevLineEmptyOrIndented || (prevLineEmptyOrIndented && ...). Once the condition is reduced to that form, it's clear that we'll either never reach the second part, or it won't evaluate to true anyway.
....
if (focusedRepository) {
....
// Resource Groups
const resourceGroups: string[] = [];
for (const resourceGroup of focusedRepository.provider.groups) {
resourceGroups.push(
`${resourceGroup.label} (${resourceGroup.resources.length} resource(s))`);
}
focusedRepository.provider.groups.map(g => g.label).join(', '); // <=
content.push(
localize(
'state-msg6',
"Resource groups: {0}", resourceGroups.join(', ')));
}
....The PVS-Studio warning: V7010 [CWE-252] The return value of function 'join' is required to be utilized. scmAccessibilityHelp.ts 108
The problem is that the result of calling join is never stored anywhere. Because of that, the line calling it is doing literally nothing.
Bugs like this usually come from one of two things: either a developer just forgets to write the assignment, or there's confusion about how a method actually works, whether it mutates the object in place or returns a new value. Since the code above shows the correct usage of join, we're leaning toward the first explanation.
The first snippet:
....
if (gapOriginalLength > 0) {
const gapStartOffset =
nesOffset + lastChange.originalStart + lastChange.originalLength;
const gapStartPos = textModel.getPositionAt(gapStartOffset);
const wordRange = textModel.getWordAtPosition(gapStartPos);
if (wordRange) {
const wordStartOffset =
textModel.getOffsetAt(
new Position(gapStartPos.lineNumber, wordRange.startColumn));
const wordEndOffset =
textModel.getOffsetAt(
new Position(gapStartPos.lineNumber, wordRange.endColumn));
const gapEndOffset = gapStartOffset + gapOriginalLength;
if (wordStartOffset <= gapStartOffset && gapEndOffset <= wordEndOffset
&& wordStartOffset <= gapEndOffset
&& gapEndOffset <= wordEndOffset) { // <=
lastChange.originalLength =
(change.originalStart + change.originalLength)
- lastChange.originalStart;
lastChange.modifiedLength =
(change.modifiedStart + change.modifiedLength)
- lastChange.modifiedStart;
continue;
}
}
}
....The PVS-Studio warning: V7001 The operands of the '&&' operator are equivalent. renameSymbolProcessor.ts 141
Here, the analyzer is pointing at this condition:
wordStartOffset <= gapStartOffset && gapEndOffset <= wordEndOffset
&& wordStartOffset <= gapEndOffset && gapEndOffset <= wordEndOffsetSince the variable names in this condition are so similar to each other, the issue isn't obvious at a glance. But look closely and gapEndOffset <= wordEndOffset is checked twice in that chain.
That repetition is what makes this potentially risky, since this could be more than just a harmless extra condition. When code has a run of similar-looking lines, variable names, or object names, it's easy to accidentally reference the wrong one. That's exactly the kind of mistake a quick read-through can miss, since the operands of && all look alike here. Hopefully it really is just an unnecessary condition, nothing more.
The second snippet:
....
if ( response.status === 401
&& text.includes('authorize_url')
&& jsonData?.authorize_url
) {
return {
type: FetchResponseKind.Failed,
modelRequestId: modelRequestIdObj,
failKind: ChatFailKind.AgentUnauthorized,
reason: response.statusText || response.statusText, // <=
data: jsonData
};
}
....The PVS-Studio warning: V7001 The operands of the '||' operator in the 'response.statusText || response.statusText' expression are equivalent. chatMLFetcher.ts 1641
Another one like the last, and it looks like a typo too. Elsewhere in the code, reason is set as follows:
reason: jsonData.message || 'Invalid previous response ID'Or like this:
reason: jsonData?.message || `token expired or invalid: ${response.status}`The third snippet:
....
// Notebooks are not supported yet.
if (URI.isUri(variableRef.value)) {
if (await this.ignoreService.isCopilotIgnored(variableRef.value)) {
return;
}
if (
variableRef.value.scheme === Schemas.vscodeNotebookCellOutput
|| variableRef.value.scheme === Schemas.vscodeNotebookCellOutput
) {
return;
}
....
validReferences.push(variableRef);
fileFolderReferences.push(variableRef);
return;
}
....The PVS-Studio warning: V7001 The operands of the '||' operator are equivalent. copilotcliPromptResolver.ts 136
The variableRef.value.scheme field is compared against the same constant from Schemas. Here are the other candidates in Schemas that this comparison could have been checking against instead:
export const vscodeNotebookCell = 'vscode-notebook-cell';
export const vscodeNotebookCellMetadata = 'vscode-notebook-cell-metadata';
export const vscodeNotebookCellMetadataDiff =
'vscode-notebook-cell-metadata-diff';
export const vscodeNotebookCellOutput = 'vscode-notebook-cell-output';
export const vscodeNotebookCellOutputDiff = 'vscode-notebook-cell-output-diff';
export const vscodeNotebookMetadata = 'vscode-notebook-metadata';
export const vscodeInteractiveInput = 'vscode-interactive-input';The fourth snippet:
static isEqual(a: Diagnostic | undefined, b: Diagnostic | undefined): boolean {
if (a === b) {
return true;
}
if (!a || !b) {
return false;
}
return a.message === b.message
&& a.severity === b.severity
&& a.code === b.code
&& a.severity === b.severity
&& a.source === b.source
&& a.range.isEqual(b.range)
&& equals(a.tags, b.tags)
&& equals(a.relatedInformation,
b.relatedInformation,
DiagnosticRelatedInformation.isEqual);
}The PVS-Studio warning: V7001 The operands of the '&&' operator are equivalent. diagnostic.ts 99
The expression a.severity === b.severity shows up twice here. It looks odd, but every field of Diagnostic is already checked, so the developers just tacked on a redundant condition.
The exact same thing happens in this next comparison:
V7001 The operands of the '&&' operator are equivalent. keyboardLayoutService.ts 431
Every example here comes down to the same question: is the repeated operand just redundant, or is it a real typo? Figuring that out usually eats up more time than it should, which is exactly why this kind of thing shouldn't slip into the code in the first place.
But mistakes like this are genuinely easy to miss during a code review. Unlike a person, a static analyzer never loses focus, which makes it invaluable when working through large amounts of repetitive code. Static analysis pays off in plenty of other situations too, but these examples make its benefits especially clear.
if (isResourceMergeEditorInput(editor)) {
....
} if (isResourceDiffEditorInput(editor)) {
....
} else if (isResourceEditorInput(editor)) {
resources.add(editor.resource);
}The PVS-Studio warning: V7030 [CWE-670] Suspicious code formatting. The 'else' keyword is probably missing. editorService.ts 739
Above is a shortened snippet, with the original formatting kept intact. Judging by the formatting and the pattern of checks, that second block should be an else if, just like the last one.
We looked closely at this snippet and didn't find anything worse than an extra check running when the first condition is met. But code like this:
else instead of else if, it would run even after the first if had already executed.That last point is the scary one, and it's a good thing this particular block uses else if. Just like before, this is the kind of thing a human reviewer can easily miss, but a static analyzer catches it without any trouble.
Besides catching potential errors, as we covered above, a static analyzer can also be a real asset for refactoring. A high number of warnings in one area can be a sign that certain code could, and sometimes should, be simplified. That's what we'll look at next.
function walkChildren(
node: TSESTree.Node,
visit: (child: TSESTree.Node) => void
) {
switch (node.type) {
....
case 'ForInStatement': // <=
visit(node.left);
visit(node.right);
visit(node.body);
break;
case 'ForOfStatement':
visit(node.left);
visit(node.right);
visit(node.body);
break;
case 'WhileStatement':
case 'DoWhileStatement':
visit(node.test);
visit(node.body);
break;
....
default:
break;
}
}The PVS-Studio warning: V7008 [CWE-691] Two or more case branches perform the same actions. code-no-accessor-after-await.ts 365
In this snippet, the case branches for ForInStatement and ForOfStatement do exactly the same thing. Given that WhileStatement and DoWhileStatement are already stacked just below, there's a good argument for doing the same here.
....
while (byteCount > 0) {
const chunk = this._chunks[chunkIndex];
if (chunk.byteLength > byteCount) {
// this chunk will survive
const chunkPart = chunk.slice(0, byteCount);
result.set(chunkPart, resultOffset);
resultOffset += byteCount;
if (advance) {
this._chunks[chunkIndex] = chunk.slice(byteCount);
this._totalLength -= byteCount;
}
byteCount -= byteCount; // <=
} else {
// this chunk will be entirely read
....
byteCount -= chunk.byteLength;
}
}
return result;
....The PVS-Studio warning: V7014 The identical expression 'byteCount' to the left and to the right of a compound assignment. ipc.net.ts 243
This one made it into the refactoring section because it's very likely the developers actually meant to zero out byteCount here, to guarantee the loop would exit. Figuring that out took a moment, though, since the else branch just below makes it look like this could just as easily be a typo.
To make the intent clear, it's better to just set the value to 0 directly.
set(element: TextEditElement) {
this._localDisposables.clear();
this._localDisposables.add(
dom.addDisposableListener(this._checkbox, 'change', e => {
element.setChecked(this._checkbox.checked);
e.preventDefault();
}));
if (element.parent.isChecked()) {
this._checkbox.checked = element.isChecked();
this._checkbox.disabled = element.isDisabled();
} else {
this._checkbox.checked = element.isChecked();
this._checkbox.disabled = element.isDisabled();
}
....
}The PVS-Studio warning: V7004 [CWE-691] The 'then' statement is equivalent to the 'else' statement. bulkEditTree.ts 595
As things stand, the else block in this snippet doesn't do anything meaningful.
But like all the other snippets here, this one is best left for VS Code's active contributors and maintainers to look into.
And that brings us to the end of the article. We've walked you through what we think are the most interesting things the PVS-Studio analyzer found in the VS Code source code. That's a solid outcome for us: even though the JavaScript and TypeScript analyzer is only just launching as an MVP, it's already catching suspicious code and real errors, even if a lot of them are simple typos for now. Down the line, once we add data-flow analysis and other features, the analyzer will get better at spotting more complex issues too.
By the way, some of our diagnostic rules started out as ideas our users and blog readers suggested to us. So if you've got thoughts on what kinds of errors our analyzer should be catching in JavaScript and TypeScript code, we'd love to hear them, some might even become real rules.
It was a lot of fun writing this article for you, and we hope you enjoyed reading it. Good luck out there, and see you next time.
P.S. You can try our analyzer on your own project here.
0
0
0
0