Мы используем куки, чтобы пользоваться сайтом
было удобно.
Вебинар: Каждая идиома когда-то была проблемой - 11.09
Теперь можно говорить без всяких прикрас: мы выпустили анализатор для языков JavaScript и TypeScript. А значит, это повод испытать его в полевых условиях и посмотреть, что он найдёт в исходном коде хорошо знакомого многим Open Source проекта Visual Studio Code. Если вам интересно узнать, какие ошибки и подозрительные фрагменты кода обнаружил анализатор, — добро пожаловать в статью.
Этот раздел будет полезен тем, кто не знаком с нашим форматом статей. Что вас ждёт в статье?
Мы — компания PVS-Studio, разрабатывающая инструменты статического анализа кода. Если кратко, то это программы, которые ищут логические ошибки в исходном коде других программ. И в таких статьях мы берём популярные и интересные Open Source проекты, анализируем их и делимся результатом с вами, читателями (а ещё с разработчиками, оставляя Issue и Pull requests).
В этом релизе мы выпустили новый анализатор, проверяющий JavaScript и TypeScript код. Пока он включает базовый набор диагностических правил, однако, как вы скоро увидите, уже в таком виде способен находить как реальные ошибки, так и подозрительные фрагменты кода.
И чтобы продемонстрировать его работу вам, мы решили проверить всем знакомый Visual Studio Code. Проект сочетает код на JavaScript и TypeScript, а его кодовая база достаточно велика. Поэтому нам было интересно посмотреть, что наш новый анализатор в нём нашёл.
Для проверки мы взяли актуальную на момент написания статьи релизную версию 1.128.0. Коммит, соответствующий этому релизу — fc3def677. Мы склонировали репозиторий, запустили анализ и по его завершении отобрали самые интересные результаты. Итак, приступаем!
В этом разделе, как и в последующих, я буду демонстрировать вам фрагмент кода, в котором находится ошибка или подозрительный момент. После него будет предупреждение анализатора PVS-Studio и ссылка на исходник в репозитории. Ну, и конечно же, сразу после — объяснение ошибки. Кстати, если у вас будет мнение по поводу какого-либо фрагмента, делитесь своими мыслями в комментариях. Всё читаем и на всё (почти) отвечаем.
А на очереди у нас странное обращение к массиву:
....
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;
}
}
....Предупреждение PVS-Studio: V7016 Suspicious access to an element of the 'diffs' object by a constant index inside a loop. notebookViewModelImpl.ts 252
Итак, у нас есть инициализация массива и последующая попытка по нему проитерироваться. Почему попытка? Индекс i внутри массива не используется ни разу, а в строке, что отмечена комментарием // <=, мы извлекаем всегда нулевой элемент. В результате выходит так, что абсолютно на каждой итерации мы, проверяя diff, работаем с первым элементом коллекции diffs.
Судя по самому фрагменту, это не было изначальной целью, и планировалось осуществлять проверки с каждым i-ым элементом, но опечатка есть опечатка.
Такие вещи периодически случаются и в других языках тоже: например, наш анализатор для Java также находил подобные проблемы в коде.
Ну а мы двигаемся дальше.
....
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;
}));
....Предупреждение PVS-Studio: V7001 The operands of the '&&' operator in the 'lastValueAtPosition !== undefined && lastValueAtPosition !== undefined' expression are equivalent. editorTextPropertySignalsContribution.ts 152
В коде есть две переменные — lastValueAtPosition и lastValueOnLine, которые могут быть либо boolean, либо undefined. И в лямбде, в которую они передаются, их сначала хотели проверить на то, что они обе не undefined, и уже после работать с ними как с boolean значениями.
В проверке, на которую указывает анализатор, произошла опечатка: в бинарном выражении && используются одинаковые операнды. Значение lastValueOnLine осталось не проверенным на undefined, что может приводить к нежелательному поведению во втором внутреннем if.
А здесь мы разберём коварные опечатки, связанные с оператором присваивания. На рассмотрении первый интересный случай.
Первый фрагмент:
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();
}Предупреждение PVS-Studio: V7005 The expression '[this.children[from].boundarySashes, this.children[to].boundarySashes]' is assigned to itself. gridview.ts 575
В этом фрагменте с помощью деструктуризации хотели поменять местами значения в объектах this.children[from].boundarySashes и this.children[to].boundarySashes. Деструктуризация в этом случае — удобная синтаксическая конструкция, позволяющая не заводить третью переменную. Однако для того, чтобы это операция возымела успех, объекты справа от присваивания должны были располагаться в другом порядке. И так называемого // swap boundary sashes здесь, к сожалению, не происходит.
К слову, ниже выполняется аналогичная операция, но уже с другими значениями, и там этой опечатки нет.
Второй фрагмент:
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,
};
}
}Предупреждение PVS-Studio: V7005 The variable 'audience' is assigned to itself. extHostTypes.ts 4033
Здесь тоже опечатка с присваиванием, но уже совершенно в другом контексте. У нас есть параметр конструктора audience и поле класса audience. И, присваивая значение полю audience, забыли указать ключевое слово this. В результате поле audience остаётся неинициализированным.
Третий фрагмент:
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());
}
}Предупреждение PVS-Studio: V7005 The variable 'query' is assigned to itself. extensionsViews.ts 1310
Здесь мы имеем дело со схожим моментом, но он уже вряд ли является ошибкой — скорее это просто опечатка либо немного странное использование оператора +=. Этот оператор сам по себе присваивает переменной query новое значение, поэтому предшествующая ему конструкция query = .... не нужна.
С одной стороны, конкретно в данном фрагменте это очень вряд ли является чем-то страшным. С другой, такая опечатка в коде, где обычное присвоение = нужно для другой переменной, уже могла бы привести к проблемам. Ну и, ко всему прочему, новый разработчик, что этот код увидит, должен будет потратить какое-то время, чтобы разобраться, действительно ли здесь допущена ошибка или конструкция с обычным присваиванием просто не нужна. Поэтому такие моменты в коде лучше не оставлять.
Аналогичные предупреждения на проекте:
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); // <=
....
}
}Предупреждение PVS-Studio: V7012 [CWE-1041] The conditional expression 'item.protocol === TunnelProtocol.Https ? TunnelProtocol.Https : TunnelProtocol.Https' always returns the same value. tunnelView.ts 997
Здесь анализатор говорит нам о том, что в обеих ветках тернарного оператора возвращается одно и то же значение TunnelProtocol.Https. Заглянули в TunnelProtocol, и действительно ситуация выглядит так, что ветка else должна возвращать значение TunnelProtocol.Http.
Что интересно, мы выписали это срабатывание и перед написанием статьи, перепроверяя его, увидели, что через несколько дней после проверки кто-то из пользователей также обнаружил эту проблему. Ссылку на его pull-request оставлю здесь. А статический анализ помогает отслеживать, чтобы такие ошибки не попадали в мастер
Предупреждение под тем же номером V7012 указало ещё и на следующий фрагмент:
....
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
);
}
....Предупреждение PVS-Studio: V7012 [CWE-1041] The conditional expression always returns the same value. dragAndDropCommand.ts 86
Тут анализатор также указывает на одинаковые выражения в ветках тернарного оператора. Но здесь фрагмент не такой тривиальный, как предыдущий. Складывается впечатление, что одинаковых выражений в его then и else ветках не должно быть. Но вот что должно быть внутри одной из них, остаётся загадкой. Поэтому предлагать исправление нужно тому, кто хорошо понимает назначение и логику этого фрагмента кода.
....
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;
}
....Предупреждение PVS-Studio: V7018 The expression 'prevLineEmptyOrIndented || (prevLineEmptyOrIndented && ...)' is redundant and always evaluates to 'prevLineEmptyOrIndented'. lightBulbWidget.ts 373
А в этом фрагменте анализатор обнаружил странный паттерн в условии. В самом предупреждении анализатора указано, что, если не брать в расчёт второе подусловие || endLine, мы получим prevLineEmptyOrIndented || (prevLineEmptyOrIndented && ...). Думаю, когда условие сокращается до такого вида, сразу становится понятно, что до его второй части мы либо не дойдём, либо оно не будет равно true.
....
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(', ')));
}
....Предупреждение PVS-Studio: V7010 [CWE-252] The return value of function 'join' is required to be utilized. scmAccessibilityHelp.ts 108
Ошибка заключается в том, что результат выполнения метода join никуда не записывают. Из-за этого строка, в которой его вызывают, сейчас является буквально рудиментарной.
Такие моменты в коде могут быть как следствием невнимательности, когда присвоение чему-либо просто забывают записать, так и путаницей с пониманием, как метод работает: изменяет ли он состояние объекта или возвращает новый. Судя по тому, что выше есть правильное использование метода join, мы склоняемся к первому варианту.
Первый фрагмент:
....
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;
}
}
}
....Предупреждение PVS-Studio: V7001 The operands of the '&&' operator are equivalent. renameSymbolProcessor.ts 141
Здесь анализатор говорит про следующее условие:
wordStartOffset <= gapStartOffset && gapEndOffset <= wordEndOffset
&& wordStartOffset <= gapEndOffset && gapEndOffset <= wordEndOffsetПоскольку имена проверяемых переменных в условии очень схожие, подозрительный момент может не сразу броситься в глаза. Однако в этой цепочке дважды проверяется условие gapEndOffset <= wordEndOffset.
Именно этот фрагмент потенциально опасен потому, что здесь мы можем иметь дело не просто с лишним условием. Когда в коде встречается цепочка похожих строк, названий переменных или объектов, легко по ошибке обратиться не к тому элементу. Как и в данном случае, такую проблему можно не заметить при беглом просмотре, поскольку операнды оператора && схожи друг с другом. Но мы надеемся, что это просто лишнее условие.
Второй фрагмент:
....
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
};
}
....Предупреждение PVS-Studio: V7001 The operands of the '||' operator in the 'response.statusText || response.statusText' expression are equivalent. chatMLFetcher.ts 1641
Аналогичное предыдущему срабатывание, выглядящее как опечатка. В других местах reason определяется вот так:
reason: jsonData.message || 'Invalid previous response ID'Или вот так:
reason: jsonData?.message || `token expired or invalid: ${response.status}`Третий фрагмент:
....
// 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;
}
....Предупреждение PVS-Studio: V7001 The operands of the '||' operator are equivalent. copilotcliPromptResolver.ts 136
Поле variableRef.value.scheme сравнивается с одной и той же константой из Schemas. В самой Schemas располагаются следующие кандидаты на то, чтобы быть в сравнении:
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';Четвертый фрагмент:
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);
}Предупреждение PVS-Studio: V7001 The operands of the '&&' operator are equivalent. diagnostic.ts 99
Здесь дважды встречаются a.severity === b.severity. Выглядит странно, однако все поля объекта Diagnostic проверены, так что разработчики просто добавили лишнее условие.
Абсолютно аналогичная ситуация в следующем сравнении:
V7001 The operands of the '&&' operator are equivalent. keyboardLayoutService.ts 431
В каждом из этих фрагментов ситуация складывается так, что повторяющийся операнд может быть как просто лишним и не нужным, так и действительно результатом опечатки. И человеку, впервые видящему этот код, чтобы понять о каком из двух вариантов идёт речь, зачастую нужно потратить большое количество времени. Чтобы это время не тратилось на такие моменты, их не стоит допускать.
Но такие ошибки действительно легко пропустить во время ревью кода. В отличие от человека, статический анализатор не теряет концентрацию, поэтому при работе с большим количеством однотипного кода такие инструменты становятся незаменимыми помощниками. Статический анализ полезен и во множестве других ситуаций, но эти примеры особенно наглядно показывают преимущества его использования.
if (isResourceMergeEditorInput(editor)) {
....
} if (isResourceDiffEditorInput(editor)) {
....
} else if (isResourceEditorInput(editor)) {
resources.add(editor.resource);
}Предупреждение PVS-Studio: V7030 [CWE-670] Suspicious code formatting. The 'else' keyword is probably missing. editorService.ts 739
Выше — сокращённый фрагмент кода, где мы сохранили исходное форматирование. Судя по форматированию и однородным проверкам, вторая конструкция должна быть else if — так же, как и последняя.
Мы рассмотрели этот фрагмент внимательно и не нашли проблем серьёзнее, чем лишняя проверка в случае срабатывания первого условия. Но подобная конструкция:
else, а не else if, он бы обязательно выполнился даже после выполнения первого if.Последнее особенно страшно, и хорошо, что в этом блоке мы имеем дело с else if. Как и в предыдущем разделе, такую ошибку может пропустить человеческий глаз, а статический анализатор без труда её обнаруживает.
Помимо обнаружения возможных ошибок, что мы рассмотрели выше, статический анализатор может стать хорошим подспорьем для рефакторинга кода. Многие срабатывания могут свидетельствовать о том, что определённые фрагменты кода можно, а иногда даже нужно упрощать. Как раз на такие случаи мы далее и посмотрим.
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;
}
}Предупреждение PVS-Studio: V7008 [CWE-691] Two or more case branches perform the same actions. code-no-accessor-after-await.ts 365
В этом фрагменте в case для ForInStatement и ForOfStatement указаны идентичные действия. С учётом того, что буквально ниже работа с WhileStatement и DoWhileStatement обобщена, есть весомое основание сделать то же самое и для рассматриваемых веток.
....
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;
....Предупреждение PVS-Studio: V7014 The identical expression 'byteCount' to the left and to the right of a compound assignment. ipc.net.ts 243
Этот фрагмент занесён в раздел про рефакторинг, потому что здесь с очень высокой вероятностью разработчики действительно планировали обнуление значения byteCount, чтобы гарантировано выйти из цикла. Но, чтобы это понять, пришлось потратить время, ведь если посмотреть ниже, в ветку else, то формируется впечатление, что в указанном фрагменте просто опечатались.
Чтобы явно выразить своё намерение, лучше напрямую обнулить значение, выставив переменной 0.
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();
}
....
}Предупреждение PVS-Studio: V7004 [CWE-691] The 'then' statement is equivalent to the 'else' statement. bulkEditTree.ts 595
Сейчас ситуация такова, что блок else в приведённом фрагменте является бессмысленным.
Но этот фрагмент, так же, как и все остальные, стоит рассматривать активным контрибьюторам и мейнтейнерам VSCode.
А на этом статья подходит к концу. Мы показали вам все самые интересные, по нашему мнению, моменты, которые анализатор PVS-Studio нашёл в исходниках VSCode. Для нас это очень хороший результат: несмотря на то, что анализатор для JavaScript и TypeScript только выходит в релиз в статусе MVP, он уже умеет находить подозрительные места и ошибки, пусть пока зачастую это простые опечатки. Спустя время, когда мы добавим data-flow анализ и другие технологии, анализатор научится находить более нетривиальные подозрительные участки кода.
К слову, периодически случалось и такое, что идеи для некоторых диагностических правил нам предлагали наши пользователи и читатели нашего блога. Поэтому, если у вас есть идеи того, какие ошибки наш анализатор может находить, будем очень рады прочитать их в комментариях и, возможно, реализовать.
Было очень интересно писать для вас эту статью, надеемся вам было интересно. Всем удачи, до новых встреч!
P.S. Попробовать наш анализатор на своём проекте вы можете по этой ссылке.
0
0
0
0