-
Notifications
You must be signed in to change notification settings - Fork 106
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
nixel2007
merged 11 commits into
1c-syntax:develop
from
EvilBeaver:feature/double-negations-3271
Aug 20, 2024
Merged
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
c1d49f5
Заготовка диагностики и красный тест на построитель выражений
1ed6cfe
Фикс красных тестов "NOT" для построителя выражений
8daad34
Новый вид диагностики - по дереву выражений и еще красные тесты на пр…
1842f44
Исправлено построение дерева и сделан зеленый тест
0e597d5
Опечатка
a3c71fa
Merge remote-tracking branch 'upstream/develop' into feature/double-n…
7ad9766
gradlew precommit
656b264
Замечания сонара
7e2945b
Поломал тест с NoSuchElementException
5ef9a5d
Исправлено построение выражения
EvilBeaver 2dff72e
Убрал проверку с отрицанием равенства булева литерала.
EvilBeaver File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) |
114 changes: 114 additions & 0 deletions
114
...om/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractExpressionTreeDiagnostic.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; | ||
} | ||
} | ||
|
||
} |
125 changes: 125 additions & 0 deletions
125
.../java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DoubleNegativesDiagnostic.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) { | ||
|
||
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); | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&&
стоит слева поставить при переносе