Skip to content

Commit

Permalink
Feature/in memory db references to variables (#1871)
Browse files Browse the repository at this point in the history
  • Loading branch information
qtLex authored Jan 2, 2022
1 parent 99b2622 commit c2b2ba5
Show file tree
Hide file tree
Showing 37 changed files with 822 additions and 92 deletions.
28 changes: 28 additions & 0 deletions docs/diagnostics/UnusedLocalVariable.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Неиспользуемая локальная переменная (UnusedLocalVariable)

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

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

Если локальная переменная объявлена, но не используется, это мертвый код, который следует удалить.
Это улучшит удобство обслуживания, поскольку разработчики не будут удивляться, для чего используется переменная.

## Сниппеты

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

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

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

```json
"UnusedLocalVariable": false
```
5 changes: 3 additions & 2 deletions docs/diagnostics/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@

## Список реализованных диагностик

Общее количество: **157**
Общее количество: **158**

* Потенциальная уязвимость: **4**
* Уязвимость: **4**
* Ошибка: **52**
* Дефект кода: **97**
* Дефект кода: **98**


| Ключ | Название | Включена по умолчанию | Важность | Тип | Тэги |
Expand Down Expand Up @@ -152,6 +152,7 @@
[UnreachableCode](UnreachableCode.md) | Недостижимый код | Да | Незначительный | Ошибка | `design`<br>`suspicious`
[UnsafeSafeModeMethodCall](UnsafeSafeModeMethodCall.md) | Небезопасное использование функции БезопасныйРежим() | Да | Блокирующий | Ошибка | `deprecated`<br>`error`
[UnusedLocalMethod](UnusedLocalMethod.md) | Неиспользуемый локальный метод | Да | Важный | Дефект кода | `standard`<br>`suspicious`<br>`unused`
[UnusedLocalVariable](UnusedLocalVariable.md) | Неиспользуемая локальная переменная | Да | Важный | Дефект кода | `brainoverload`<br>`badpractice`
[UnusedParameters](UnusedParameters.md) | Неиспользуемый параметр | Да | Важный | Дефект кода | `design`<br>`unused`
[UsageWriteLogEvent](UsageWriteLogEvent.md) | Неверное использование метода "ЗаписьЖурналаРегистрации" | Да | Информационный | Дефект кода | `standard`<br>`badpractice`
[UseLessForEach](UseLessForEach.md) | Бесполезный перебор коллекции | Да | Критичный | Ошибка | `clumsy`
Expand Down
28 changes: 28 additions & 0 deletions docs/en/diagnostics/UnusedLocalVariable.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Unused local variable (UnusedLocalVariable)

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

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

If a local variable is declared but not used, it is dead code and should be removed.
Doing so will improve maintainability because developers will not wonder what the variable is used for.

## Snippets

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

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

### Parameter for config

```json
"UnusedLocalVariable": false
```
5 changes: 3 additions & 2 deletions docs/en/diagnostics/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ To escape individual sections of code or files from triggering diagnostics, you

## Implemented diagnostics

Total: **157**
Total: **158**

* Security Hotspot: **4**
* Vulnerability: **4**
* Error: **52**
* Code smell: **97**
* Code smell: **98**


| Key | Name| Enabled by default | Severity | Type | Tags |
Expand Down Expand Up @@ -152,6 +152,7 @@ Total: **157**
[UnreachableCode](UnreachableCode.md) | Unreachable Code | Yes | Minor | Error | `design`<br>`suspicious`
[UnsafeSafeModeMethodCall](UnsafeSafeModeMethodCall.md) | Unsafe SafeMode method call | Yes | Blocker | Error | `deprecated`<br>`error`
[UnusedLocalMethod](UnusedLocalMethod.md) | Unused local method | Yes | Major | Code smell | `standard`<br>`suspicious`<br>`unused`
[UnusedLocalVariable](UnusedLocalVariable.md) | Unused local variable | Yes | Major | Code smell | `brainoverload`<br>`badpractice`
[UnusedParameters](UnusedParameters.md) | Unused parameter | Yes | Major | Code smell | `design`<br>`unused`
[UsageWriteLogEvent](UsageWriteLogEvent.md) | Incorrect use of the method "WriteLogEvent" | Yes | Info | Code smell | `standard`<br>`badpractice`
[UseLessForEach](UseLessForEach.md) | Useless collection iteration | Yes | Critical | Error | `clumsy`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,11 @@ public void handleEvent(DocumentContextContentChangedEvent event) {
for (SDBLTokenizer sdblTokenizer : documentContext.getQueries()) {
measureCollector.measureIt(sdblTokenizer::getAst, "context: queryAst");
}
measureCollector.measureIt(documentContext.getSymbolTree()::getChildrenFlat, "context: symbolTree - childrenFlat");
measureCollector.measureIt(documentContext.getSymbolTree()::getMethods, "context: symbolTree - methods");
measureCollector.measureIt(documentContext.getSymbolTree()::getVariables, "context: symbolTree - variables");
measureCollector.measureIt(documentContext.getSymbolTree()::getVariablesByName, "context: symbolTree - variablesByName");
measureCollector.measureIt(documentContext.getSymbolTree()::getMethodsByName, "context: symbolTree - methodsByName");
measureCollector.measureIt(documentContext::getDiagnosticIgnorance, "context: diagnosticIgnorance");
measureCollector.measureIt(documentContext::getCognitiveComplexityData, "context: cognitiveComplexity");
measureCollector.measureIt(documentContext::getCyclomaticComplexityData, "context: cyclomaticComplexity");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public SymbolTree compute() {
ModuleSymbol moduleSymbol = new ModuleSymbolComputer(documentContext).compute();
List<MethodSymbol> methods = new MethodSymbolComputer(documentContext).compute();
List<RegionSymbol> regions = new RegionSymbolComputer(documentContext).compute();
List<VariableSymbol> variables = new VariableSymbolComputer(documentContext).compute();
List<VariableSymbol> variables = new VariableSymbolComputer(documentContext, moduleSymbol, methods).compute();

List<SourceDefinedSymbol> allOfThem = new ArrayList<>(methods);
allOfThem.addAll(regions);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
package com.github._1c_syntax.bsl.languageserver.context.computer;

import com.github._1c_syntax.bsl.languageserver.context.DocumentContext;
import com.github._1c_syntax.bsl.languageserver.context.symbol.MethodSymbol;
import com.github._1c_syntax.bsl.languageserver.context.symbol.ModuleSymbol;
import com.github._1c_syntax.bsl.languageserver.context.symbol.SourceDefinedSymbol;
import com.github._1c_syntax.bsl.languageserver.context.symbol.VariableSymbol;
import com.github._1c_syntax.bsl.languageserver.context.symbol.variable.VariableDescription;
import com.github._1c_syntax.bsl.languageserver.context.symbol.variable.VariableKind;
Expand All @@ -32,60 +35,170 @@
import com.github._1c_syntax.bsl.parser.BSLParserRuleContext;
import org.antlr.v4.runtime.Token;
import org.antlr.v4.runtime.tree.ParseTree;
import org.eclipse.lsp4j.Range;

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.TreeMap;
import java.util.function.Function;

import static java.util.stream.Collectors.groupingBy;
import static java.util.stream.Collectors.toMap;

public class VariableSymbolComputer extends BSLParserBaseVisitor<ParseTree> implements Computer<List<VariableSymbol>> {

private final DocumentContext documentContext;
private final ModuleSymbol module;
private final Map<Range, SourceDefinedSymbol> methods;
private final Set<VariableSymbol> variables = new HashSet<>();
private final Map<String, String> currentMethodVariables = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
private final Map<String, String> moduleVariables = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);

private SourceDefinedSymbol currentMethod;

public VariableSymbolComputer(DocumentContext documentContext) {
public VariableSymbolComputer(DocumentContext documentContext, ModuleSymbol module, List<MethodSymbol> methods) {
this.documentContext = documentContext;
this.module = module;
this.methods = methods.stream().collect(toMap(MethodSymbol::getSubNameRange, Function.identity()));
this.currentMethod = module;
}

@Override
public List<VariableSymbol> compute() {
variables.clear();
moduleVariables.clear();
visitFile(documentContext.getAst());
return new ArrayList<>(variables);
}

@Override
public ParseTree visitModuleVarDeclaration(BSLParser.ModuleVarDeclarationContext ctx) {
var symbol = createVariableSymbol(ctx, ctx.var_name(), ctx.EXPORT_KEYWORD() != null, VariableKind.MODULE);
var symbol = VariableSymbol.builder()
.name(ctx.var_name().getText())
.owner(documentContext)
.range(Ranges.create(ctx))
.variableNameRange(Ranges.create(ctx.var_name()))
.export(ctx.EXPORT_KEYWORD() != null)
.kind(VariableKind.MODULE)
.description(createDescription(ctx))
.scope(module)
.build();
variables.add(symbol);

moduleVariables.put(ctx.var_name().getText(), ctx.var_name().getText());
return ctx;
}

@Override
public ParseTree visitSub(BSLParser.SubContext ctx) {
this.currentMethod = getVariableScope(ctx);
ParseTree tree = super.visitSub(ctx);
currentMethodVariables.clear();
this.currentMethod = module;
return tree;
}

@Override
public ParseTree visitSubVarDeclaration(BSLParser.SubVarDeclarationContext ctx) {
var symbol = createVariableSymbol(ctx, ctx.var_name(), false, VariableKind.LOCAL);
var symbol = VariableSymbol.builder()
.name(ctx.var_name().getText())
.owner(documentContext)
.range(Ranges.create(ctx))
.variableNameRange(Ranges.create(ctx.var_name()))
.export(false)
.kind(VariableKind.LOCAL)
.description(createDescription(ctx))
.scope(getVariableScope(ctx))
.build();
variables.add(symbol);
currentMethodVariables.put(ctx.var_name().getText(), ctx.var_name().getText());
return ctx;
}

@Override
public ParseTree visitParam(BSLParser.ParamContext ctx) {
if (ctx.IDENTIFIER() == null) {
return ctx;
}

var variable = VariableSymbol.builder()
.name(ctx.IDENTIFIER().getText())
.scope(currentMethod)
.owner(documentContext)
.range(Ranges.create(ctx))
.variableNameRange(Ranges.create(ctx.IDENTIFIER()))
.export(false)
.kind(VariableKind.PARAMETER)
.description(Optional.empty())
.build();
variables.add(variable);

currentMethodVariables.put(ctx.IDENTIFIER().getText(), ctx.IDENTIFIER().getText());
return ctx;
}

private VariableSymbol createVariableSymbol(
BSLParserRuleContext ctx,
BSLParser.Var_nameContext varName,
boolean export,
VariableKind kind
) {
return VariableSymbol.builder()
.name(varName.getText())
@Override
public ParseTree visitLValue(BSLParser.LValueContext ctx) {
if (
ctx.getChildCount() > 1
|| currentMethodVariables.containsKey(ctx.getText())
|| moduleVariables.containsKey(ctx.getText())
) {
return ctx;
}

var variable = VariableSymbol.builder()
.name(ctx.getText())
.owner(documentContext)
.range(Ranges.create(ctx))
.variableNameRange(Ranges.create(varName))
.export(export)
.kind(kind)
.variableNameRange(Ranges.create(ctx))
.export(false)
.kind(VariableKind.DYNAMIC)
.scope(currentMethod)
.description(createDescription(ctx))
.build();
variables.add(variable);

currentMethodVariables.put(ctx.getText(), ctx.getText());
return ctx;
}

private SourceDefinedSymbol getVariableScope(BSLParser.SubVarDeclarationContext ctx) {
var sub = (BSLParser.SubContext) Trees.getRootParent(ctx, BSLParser.RULE_sub);
if (sub == null) {
return module;
}

return getVariableScope(sub);
}

private SourceDefinedSymbol getVariableScope(BSLParser.SubContext ctx) {
BSLParserRuleContext subNameNode;
if (Trees.nodeContainsErrors(ctx)) {
return module;
} else if (ctx.function() != null) {
subNameNode = ctx.function().funcDeclaration().subName();
} else {
subNameNode = ctx.procedure().procDeclaration().subName();
}

return methods.getOrDefault(Ranges.create(subNameNode), module);
}

private Optional<VariableDescription> createDescription(BSLParser.LValueContext ctx) {
var trailingComments = Trees.getTrailingComment(documentContext.getTokens(), ctx.getStop());

if (trailingComments.isEmpty()) {
return Optional.empty();
}

return Optional.of(
new VariableDescription(Collections.emptyList(), trailingComments)
);
}

private Optional<VariableDescription> createDescription(BSLParserRuleContext ctx) {
Expand All @@ -108,5 +221,4 @@ private Optional<VariableDescription> createDescription(BSLParserRuleContext ctx

}


}
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
@EqualsAndHashCode(onlyExplicitlyIncluded = true)
@ToString(exclude = {"children", "parent"})
public class MethodSymbol implements SourceDefinedSymbol, Exportable, Describable {
@EqualsAndHashCode.Include
String name;

@Builder.Default
Expand Down
Loading

1 comment on commit c2b2ba5

@github-actions
Copy link

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'BSL LS perfomance measurement (SSL 3.1)'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.10.

Benchmark suite Current: c2b2ba5 Previous: 99b2622 Ratio
.github/scripts/benchmark.py::test_analyze_ssl31 40.13981000582377 sec (stddev: 0.8490682125805986) 34.97286677360535 sec (stddev: 0.7048670471193371) 1.15

This comment was automatically generated by workflow using github-action-benchmark.

CC: @otymko

Please sign in to comment.