Skip to content

Add useless ternary operator diagnostic #1820

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

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
47 changes: 47 additions & 0 deletions docs/diagnostics/UselessTernaryOperator.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# Бесполезный тернарный оператор (UselessTernaryOperator)

| Тип | Поддерживаются<br>языки | Важность | Включена<br>по умолчанию | Время на<br>исправление (мин) | Теги |
|:-------------:|:-----------------------------:|:----------------:|:------------------------------:|:-----------------------------------:|:-----------------------------------:|
| `Дефект кода` | `BSL` | `Информационный` | `Да` | `1` | `badpractice`<br>`suspicious` |

<!-- Блоки выше заполняются автоматически, не трогать -->
## Описание диагностики
Размещение в тернарном операторе булевых констант "Истина" или "Ложь" указывает на плохую продуманность кода.

## Примеры
Бессмысленные операторы

```Bsl
А = ?(Истина, 1, 0);
```
```Bsl
А = ?(Б = 1, Истина, Ложь);
```
```Bsl
А = ?(Б = 0, False, True);
```

Подозрительные операторы

```Bsl
А = ?(Б = 1, True, Истина);
```
```Bsl
А = ?(Б = 0, 0, False);
```

## Сниппеты

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

```bsl
// BSLLS:UselessTernaryOperator-off
// BSLLS:UselessTernaryOperator-on
```

### Параметр конфигурационного файла

```json
"UselessTernaryOperator": false
```
47 changes: 47 additions & 0 deletions docs/en/diagnostics/UselessTernaryOperator.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# Useless ternary operator (UselessTernaryOperator)

| Type | Scope | Severity | Activated<br>by default | Minutes<br>to fix | Tags |
|:------------:|:-----:|:--------:|:-----------------------------:|:-----------------------:|:-----------------------------------:|
| `Code smell` | `BSL` | `Info` | `Yes` | `1` | `badpractice`<br>`suspicious` |

<!-- Блоки выше заполняются автоматически, не трогать -->
## Description
The placement of Boolean constants "True" or "False" in the ternary operator indicates poor code thoughtfulness.

## Examples
Useless operators

```Bsl
A = ?(True, 1, 0);
```
```Bsl
A = ?(B = 1, True, False);
```
```Bsl
A = ?(B = 0, False, True);
```

Suspicious operators

```Bsl
A = ?(B = 1, True, True);
```
```Bsl
A = ?(B = 0, 0, False);
```

## Snippets

<!-- Блоки ниже заполняются автоматически, не трогать -->
### Diagnostic ignorance in code

```bsl
// BSLLS:UselessTernaryOperator-off
// BSLLS:UselessTernaryOperator-on
```

### Parameter for config

```json
"UselessTernaryOperator": false
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
/*
* This file is a part of BSL Language Server.
*
* Copyright (c) 2018-2021
* Alexey Sosnoviy <[email protected]>, Nikita Gryzlov <[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.DiagnosticMetadata;
import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticScope;
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.providers.CodeActionProvider;
import com.github._1c_syntax.bsl.parser.BSLParser;
import com.github._1c_syntax.mdclasses.mdo.MDLanguage;
import org.antlr.v4.runtime.tree.ParseTree;
import org.eclipse.lsp4j.CodeAction;
import org.eclipse.lsp4j.CodeActionParams;
import org.eclipse.lsp4j.Diagnostic;
import org.eclipse.lsp4j.TextEdit;

import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

@DiagnosticMetadata(
type = DiagnosticType.CODE_SMELL,
severity = DiagnosticSeverity.INFO,
scope = DiagnosticScope.BSL,
minutesToFix = 1,
tags = {
DiagnosticTag.BADPRACTICE,
DiagnosticTag.SUSPICIOUS
}

)
public class UselessTernaryOperatorDiagnostic extends AbstractVisitorDiagnostic implements QuickFixProvider {

private static final int SKIPPED_RULE_INDEX = 0;

@Override
public ParseTree visitTernaryOperator(BSLParser.TernaryOperatorContext ctx){

var exp = ctx.expression();
var condition = getBooleanToken(exp.get(0));
var trueBranch = getBooleanToken(exp.get(1));
var falseBranch = getBooleanToken(exp.get(2));
Copy link
Member

@nixel2007 nixel2007 Oct 6, 2021

Choose a reason for hiding this comment

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

я бы на всякий случай перед всеми этими дерганиями по индексу проверил, что exp.size() == 3. опасаюсь IndexOutOfBoundException при наборе текста

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Если не 3 падаем?


if (condition != SKIPPED_RULE_INDEX) {
diagnosticStorage.addDiagnostic(ctx);
} else if (trueBranch == BSLParser.TRUE && falseBranch == BSLParser.FALSE){
var dgs = diagnosticStorage.addDiagnostic(ctx);
dgs.ifPresent(diagnostic -> diagnostic.setData(exp.get(0).getText()));
} else if (trueBranch == BSLParser.FALSE && falseBranch == BSLParser.TRUE){
var dgs = diagnosticStorage.addDiagnostic(ctx);
dgs.ifPresent(diagnostic -> diagnostic.setData(getAdaptedText(exp.get(0).getText())));
} else if (trueBranch != SKIPPED_RULE_INDEX || falseBranch != SKIPPED_RULE_INDEX){
diagnosticStorage.addDiagnostic(ctx);
}

return super.visitTernaryOperator(ctx);
}

@Override
public List<CodeAction> getQuickFixes(
List<Diagnostic> diagnostics,
CodeActionParams params,
DocumentContext documentContext
) {

List<TextEdit> textEdits = new ArrayList<>();

diagnostics.forEach((Diagnostic diagnostic) -> {
var range = diagnostic.getRange();
var textEdit = new TextEdit(range, (String) diagnostic.getData());
textEdits.add(textEdit);
});

return CodeActionProvider.createCodeActions(
textEdits,
info.getResourceString("quickFixMessage"),
documentContext.getUri(),
diagnostics
);

}

private String getAdaptedText(String text) {
if(documentContext.getServerContext().getConfiguration().getDefaultLanguage() == MDLanguage.ENGLISH) {
Copy link
Member

Choose a reason for hiding this comment

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

лучше заменить на вызов documentContext.getScriptVariantLocale()

Copy link
Member

Choose a reason for hiding this comment

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

ну и вообще бы это перевесить на работу с Resources#getResourceString

return "NOT (" + text + ")";
} else {
return "НЕ (" + text + ")";
}
}

private int getBooleanToken(BSLParser.ExpressionContext expCtx){

var tmpCtx = Optional.of(expCtx)
.filter(ctx -> ctx.children.size() == 1)
.map(ctx -> (BSLParser.MemberContext) ctx.getChild(0))
Copy link
Member

Choose a reason for hiding this comment

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

просто .map(ctx -> ctx.member()) или сразу .map(BSLParser.ExpressionContext::member)), нет нужды работать с детьми в грязную

.map(ctx -> ctx.getChild(0))
Copy link
Member

Choose a reason for hiding this comment

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

вместо этого вызова и двух вызовов ниже можно взять .map(ctx -> ctx.constValue(). если constValue в этом expression нет, то метод вернет null и Optional станет empty

.filter(BSLParser.ConstValueContext.class::isInstance)
.map(BSLParser.ConstValueContext.class::cast);

return tmpCtx
Copy link
Member

Choose a reason for hiding this comment

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

тэкс, а зачем все это ниже, если можно просто tmpCtx.filter(ctx -> ctx.TRUE() || ctx.FALSE).isPresent() и сменить возвращаемое значение с int на boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Дальше по коду нужно знать в какой ветке у нас Истина, в какой ложь

.map(ctx -> ctx.getToken(BSLParser.TRUE, 0))
.map(ctx -> BSLParser.TRUE)
.or(() -> tmpCtx
.map(ctx -> ctx.getToken(BSLParser.FALSE, 0))
.map(ctx -> BSLParser.FALSE)
)
.orElse(SKIPPED_RULE_INDEX);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1810,6 +1810,16 @@
"title": "Useless collection iteration",
"$id": "#/definitions/UseLessForEach"
},
"UselessTernaryOperator": {
"description": "Useless ternary operator",
"default": true,
"type": [
"boolean",
"object"
],
"title": "Useless ternary operator",
"$id": "#/definitions/UselessTernaryOperator"
},
"UsingCancelParameter": {
"description": "Using parameter \"Cancel\"",
"default": true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,9 @@
"UseLessForEach": {
"$ref": "parameters-schema.json#/definitions/UseLessForEach"
},
"UselessTernaryOperator": {
"$ref": "parameters-schema.json#/definitions/UselessTernaryOperator"
},
"UsingCancelParameter": {
"$ref": "parameters-schema.json#/definitions/UsingCancelParameter"
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
diagnosticMessage=Useless ternary operator
diagnosticName=Useless ternary operator
quickFixMessage=Fix some useless ternary operators
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
diagnosticMessage=Бесполезный тернарный оператор
diagnosticName=Бесполезный тернарный оператор
quickFixMessage=Исправить некоторые бесполезные тернарные операторы
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* This file is a part of BSL Language Server.
*
* Copyright (c) 2018-2021
* Alexey Sosnoviy <[email protected]>, Nikita Gryzlov <[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 org.eclipse.lsp4j.CodeAction;
import org.eclipse.lsp4j.Diagnostic;
import org.junit.jupiter.api.Test;

import java.util.List;

import static com.github._1c_syntax.bsl.languageserver.util.Assertions.assertThat;

class UselessTernaryOperatorDiagnosticTest extends AbstractDiagnosticTest<UselessTernaryOperatorDiagnostic> {

UselessTernaryOperatorDiagnosticTest() {
super(UselessTernaryOperatorDiagnostic.class);
}

@Test
void test() {

List<Diagnostic> diagnostics = getDiagnostics();

assertThat(diagnostics).hasSize(8);
assertThat(diagnostics, true)
.hasRange(1, 4, 1, 26)
.hasRange(2, 4, 2, 25)
.hasRange(3, 4, 3, 26)
.hasRange(4, 4, 4, 25)
.hasRange(5, 4, 5, 21)
.hasRange(6, 4, 6, 22)
.hasRange(7, 4, 7, 19)
.hasRange(8, 4, 8, 18);

}

@Test
void testQuickFix() {

final DocumentContext documentContext = getDocumentContext();
List<Diagnostic> diagnostics = getDiagnostics();

final Diagnostic directDiagnostic = diagnostics.get(0);
List<CodeAction> directQuickFixes = getQuickFixes(directDiagnostic);
assertThat(directQuickFixes).hasSize(1);
final CodeAction directQuickFix = directQuickFixes.get(0);
assertThat(directQuickFix)
.of(diagnosticInstance)
.in(documentContext)
.fixes(directDiagnostic)
.hasChanges(1)
.hasNewText("Б=1");

final Diagnostic reversDiagnostic = diagnostics.get(1);
List<CodeAction> reversQuickFixes = getQuickFixes(reversDiagnostic);
assertThat(reversQuickFixes).hasSize(1);
final CodeAction reversQuickFix = reversQuickFixes.get(0);
assertThat(reversQuickFix)
.of(diagnosticInstance)
.in(documentContext)
.fixes(reversDiagnostic)
.hasChanges(1)
.hasNewText("NOT (Б=0)");
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Бессмысленные тернарники
А = ?(Б = 1, Истина, Ложь);// прямой, фиксится в А = Б = 1;
А = ?(Б = 0, False, True);// обратный, фиксится в А = НЕ (Б = 0);
А = ?(Б = 1, True, Истина);
А = ?(Б = 0, Ложь, False);
А = ?(Б = 1, True, 1);
А = ?(Б = 0, 0, False);
А = ?(истина, 1, 0);
А = ?(false, 0, 1);

// валидный
ОбластьМакета.Параметры.ДебетСубСчета = ОбластьМакета.Параметры.ДебетСубСчета
+ ?(ПустаяСтрока(ОбластьМакета.Параметры.ДебетСубСчета), "", ", ")
+ СчетДт;