Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Реализация #3271: Диагностика двойных отрицаний #3308

Merged
merged 11 commits into from
Aug 20, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions docs/diagnostics/DoubleNegatives.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# Двойные отрицания (DoubleNegatives)

<!-- Блоки выше заполняются автоматически, не трогать -->
## Описание диагностики

Использование двойных отрицаний усложняет понимание кода и может приводить к ошибкам, когда вместо истины разработчик "в уме" вычислил Ложь, или наоборот.
Двойные отрицания рекомендуется заменять на выражения условий, которые прямо выражают намерения автора.

## Примеры

### Неправильно

```bsl
Если Не ТаблицаЗначений.Найти(ИскомоеЗначение, "Колонка") <> Неопределено Тогда
// Сделать действие
КонецЕсли;
```

### Правильно

```bsl
Если ТаблицаЗначений.Найти(ИскомоеЗначение, "Колонка") = Неопределено Тогда
// Сделать действие
КонецЕсли;
```

## Источники
<!-- Необходимо указывать ссылки на все источники, из которых почерпнута информация для создания диагностики -->

* Источник: [Remove double negative](https://www.refactoring.com/catalog/removeDoubleNegative.html)
28 changes: 28 additions & 0 deletions docs/en/diagnostics/DoubleNegatives.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Double negatives (DoubleNegatives)

<!-- Блоки выше заполняются автоматически, не трогать -->
## Description

Using double negatives complicates the understanding of the code and can lead to errors when instead of truth the developer "in his mind" calculated False, or vice versa. It is recommended to replace double negatives with conditions that directly express the author's intentions.

## Examples

### Wrong

```bsl
If Not ValueTable.Find(ValueToSearch, "Column") <> Undefined Тогда
// Act
EndIf;
```

### Correct

```bsl
If ValueTable.Find(ValueToSearch, "Column") = Undefined Тогда
// Act
EndIf;
```

## Sources

* Источник: [Remove double negative](https://www.refactoring.com/catalog/removeDoubleNegative.html)
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
/*
* This file is a part of BSL Language Server.
*
* Copyright (c) 2018-2024
* Alexey Sosnoviy <[email protected]>, Nikita Fedkin <[email protected]> and contributors
*
* SPDX-License-Identifier: LGPL-3.0-or-later
*
* BSL Language Server is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3.0 of the License, or (at your option) any later version.
*
* BSL Language Server is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with BSL Language Server.
*/
package com.github._1c_syntax.bsl.languageserver.diagnostics;

import com.github._1c_syntax.bsl.languageserver.context.DocumentContext;
import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticInfo;
import com.github._1c_syntax.bsl.languageserver.utils.expressiontree.ExpressionTreeBuildingVisitor;
import com.github._1c_syntax.bsl.languageserver.utils.expressiontree.ExpressionTreeVisitor;
import com.github._1c_syntax.bsl.parser.BSLParser;
import lombok.Getter;
import lombok.Setter;
import org.antlr.v4.runtime.tree.ParseTree;
import org.eclipse.lsp4j.Diagnostic;

import java.util.List;

/**
* Диагностика, анализирующая выражения BSL и предоставляющая для этого Expression Tree
*/
public abstract class AbstractExpressionTreeDiagnostic extends ExpressionTreeVisitor implements BSLDiagnostic {
@Getter
@Setter
protected DiagnosticInfo info;
protected final DiagnosticStorage diagnosticStorage = new DiagnosticStorage(this);
protected DocumentContext documentContext;

@Override
public final List<Diagnostic> getDiagnostics(DocumentContext documentContext) {
this.documentContext = documentContext;
diagnosticStorage.clearDiagnostics();

var expressionTreeBuilder = new ExpressionTreeBuilder();
expressionTreeBuilder.visitFile(documentContext.getAst());

return diagnosticStorage.getDiagnostics();
}

/**
* При входе в выражение вызывается данный метод.
* Переопределяя его можно оценить - имеет ли смысл строить дерево выражения, или данное выражение не подходит.
* Позволяет сократить время на построение дерева, если это не требуется для данного AST.
*
* @param ctx - выражение, которое в данный момент посещается.
* @return - если надо прекратить обход в глубину и построить Expression Tree на данном выражении - надо вернуть ACCEPT
* - если надо пройти дальше и посетить дочерние выражения, не затрагивая данное - надо вернуть VISIT_CHILDREN
* - если надо пропустить выражение, не ходить глубже и не строить Expression Tree - надо вернуть SKIP
*/
protected ExpressionVisitorDecision onExpressionEnter(BSLParser.ExpressionContext ctx) {
return ExpressionVisitorDecision.ACCEPT;
}

/**
* Стратегия по построению дерева выражения на основе выражения AST
*/
protected enum ExpressionVisitorDecision {

/**
* Не обрабатывать выражение
*/
SKIP,

/**
* Обработать данное выражение (построить для него ExpressionTree)
*/
ACCEPT,

/**
* Пропустить данное выражение и обойти вложенные в него выражения
*/
VISIT_CHILDREN;
}

private class ExpressionTreeBuilder extends ExpressionTreeBuildingVisitor {
@Override
public ParseTree visitExpression(BSLParser.ExpressionContext ctx) {

var result = onExpressionEnter(ctx);
return switch (result) {
case SKIP -> ctx;
case ACCEPT -> processExpression(ctx);
case VISIT_CHILDREN -> super.visitChildren(ctx);
};
}

private BSLParser.ExpressionContext processExpression(BSLParser.ExpressionContext ctx) {
super.visitExpression(ctx);
var expressionTree = getExpressionTree();
if (expressionTree != null) // нашлись выражения в предложенном файле
visitTopLevelExpression(expressionTree);

return ctx;
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
/*
* This file is a part of BSL Language Server.
*
* Copyright (c) 2018-2024
* Alexey Sosnoviy <[email protected]>, Nikita Fedkin <[email protected]> and contributors
*
* SPDX-License-Identifier: LGPL-3.0-or-later
*
* BSL Language Server is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3.0 of the License, or (at your option) any later version.
*
* BSL Language Server is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with BSL Language Server.
*/
package com.github._1c_syntax.bsl.languageserver.diagnostics;

import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticMetadata;
import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticSeverity;
import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticTag;
import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticType;
import com.github._1c_syntax.bsl.languageserver.utils.Trees;
import com.github._1c_syntax.bsl.languageserver.utils.expressiontree.BinaryOperationNode;
import com.github._1c_syntax.bsl.languageserver.utils.expressiontree.BslExpression;
import com.github._1c_syntax.bsl.languageserver.utils.expressiontree.BslOperator;
import com.github._1c_syntax.bsl.languageserver.utils.expressiontree.ExpressionNodeType;
import com.github._1c_syntax.bsl.languageserver.utils.expressiontree.UnaryOperationNode;
import com.github._1c_syntax.bsl.parser.BSLParser;

@DiagnosticMetadata(
type = DiagnosticType.CODE_SMELL,
severity = DiagnosticSeverity.MAJOR,
minutesToFix = 3,
tags = {
DiagnosticTag.BRAINOVERLOAD,
DiagnosticTag.BADPRACTICE
}
)
public class DoubleNegativesDiagnostic extends AbstractExpressionTreeDiagnostic {

@Override
protected void visitBinaryOperation(BinaryOperationNode node) {

if (node.getOperator() != BslOperator.EQUAL && node.getOperator() != BslOperator.NOT_EQUAL) {
super.visitBinaryOperation(node);
return;
}

var parent = node.getParent();

if (parent == null || !isNegationOperator(parent)) {
super.visitBinaryOperation(node);
return;
}

if (node.getOperator() == BslOperator.NOT_EQUAL || isBooleanLiteral(node.getLeft()) || isBooleanLiteral(node.getRight())) {
addDiagnostic(node);
}

super.visitBinaryOperation(node);
}

@Override
protected void visitUnaryOperation(UnaryOperationNode node) {
if (node.getOperator() == BslOperator.NOT &&
node.getParent() != null &&
node.getParent().getNodeType() == ExpressionNodeType.UNARY_OP) {
Comment on lines +70 to +72
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&& стоит слева поставить при переносе


var unaryParent = node.getParent().<UnaryOperationNode>cast();
if (unaryParent.getOperator() == BslOperator.NOT) {
addDiagnostic(node);
}
}

super.visitUnaryOperation(node);
}

private static boolean isBooleanLiteral(BslExpression node) {
if (node.getNodeType() != ExpressionNodeType.LITERAL) {
return false;
}

var constant = (BSLParser.ConstValueContext) node.getRepresentingAst();
return constant.TRUE() != null || constant.FALSE() != null;
}

private static boolean isNegationOperator(BslExpression parent) {
return parent.getNodeType() == ExpressionNodeType.UNARY_OP
&& parent.<UnaryOperationNode>cast().getOperator() == BslOperator.NOT;
}

private void addDiagnostic(BinaryOperationNode node) {
var startToken = Trees.getTokens(node.getParent().getRepresentingAst())
.stream()
.findFirst()
.orElseThrow();

var endToken = Trees.getTokens(node.getRight().getRepresentingAst())
.stream()
.reduce((one, two) -> two)
.orElseThrow();

diagnosticStorage.addDiagnostic(startToken, endToken);
}

private void addDiagnostic(UnaryOperationNode node) {
var startToken = Trees.getTokens(node.getParent().getRepresentingAst())
.stream()
.findFirst()
.orElseThrow();

var endToken = Trees.getTokens(node.getOperand().getRepresentingAst())
.stream()
.reduce((one, two) -> two)
.orElseThrow();

diagnosticStorage.addDiagnostic(startToken, endToken);
}
}
Loading
Loading