Skip to content

Commit

Permalink
Merge branch 'feature/cfg' into feature/dead-code-improve
Browse files Browse the repository at this point in the history
  • Loading branch information
@EvilBeaver
EvilBeaver committed Aug 29, 2021
2 parents bf6fcc2 + 1a15ad1 commit 862a1ee
Show file tree
Hide file tree
Showing 7 changed files with 91 additions and 16 deletions.
6 changes: 3 additions & 3 deletions docs/diagnostics/AllFunctionPathMustHaveReturn.md
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# Все возможные пути выполнения функции должны содержать оператор Возврат (AllFunctionPathMustHaveReturn)

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

## Параметры

Expand Down
2 changes: 1 addition & 1 deletion docs/diagnostics/index.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

| Ключ | Название | Включена по умолчанию | Важность | Тип | Тэги |
| --- | --- | :-: | --- | --- | --- |
[AllFunctionPathMustHaveReturn](AllFunctionPathMustHaveReturn.md) | Все возможные пути выполнения функции должны содержать оператор Возврат | Да | Информационный | Дефект кода | `unpredictable`<br>`badpractice`<br>`suspicious`
[AllFunctionPathMustHaveReturn](AllFunctionPathMustHaveReturn.md) | Все возможные пути выполнения функции должны содержать оператор Возврат | Да | Важный | Дефект кода | `unpredictable`<br>`badpractice`<br>`suspicious`
[AssignAliasFieldsInQuery](AssignAliasFieldsInQuery.md) | Назначение псевдонимов выбранным полям в запросе | Да | Важный | Дефект кода | `standard`<br>`sql`<br>`badpractice`
[BeginTransactionBeforeTryCatch](BeginTransactionBeforeTryCatch.md) | Нарушение правил работы с транзакциями для метода 'НачатьТранзакцию' | Да | Важный | Ошибка | `standard`
[CachedPublic](CachedPublic.md) | Кеширование программного интерфейса | Да | Важный | Дефект кода | `standard`<br>`design`
Expand Down
2 changes: 1 addition & 1 deletion docs/en/diagnostics/AllFunctionPathMustHaveReturn.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

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

## Parameters

Expand Down
2 changes: 1 addition & 1 deletion docs/en/diagnostics/index.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Total: **144**

| Key | Name| Enabled by default | Severity | Type | Tags |
| --- | --- | :-: | --- | --- | --- |
[AllFunctionPathMustHaveReturn](AllFunctionPathMustHaveReturn.md) | All execution paths of a function must have a Return statement | Yes | Info | Code smell | `unpredictable`<br>`badpractice`<br>`suspicious`
[AllFunctionPathMustHaveReturn](AllFunctionPathMustHaveReturn.md) | All execution paths of a function must have a Return statement | Yes | Major | Code smell | `unpredictable`<br>`badpractice`<br>`suspicious`
[AssignAliasFieldsInQuery](AssignAliasFieldsInQuery.md) | Assigning aliases to selected fields in a query | Yes | Major | Code smell | `standard`<br>`sql`<br>`badpractice`
[BeginTransactionBeforeTryCatch](BeginTransactionBeforeTryCatch.md) | Violating transaction rules for the 'BeginTransaction' method | Yes | Major | Error | `standard`
[CachedPublic](CachedPublic.md) | Cached public methods | Yes | Major | Code smell | `standard`<br>`design`
Expand Down
89 changes: 82 additions & 7 deletions src/main/java/com/github/_1c_syntax/bsl/languageserver/cfg/CfgBuildingParseTreeVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.github._1c_syntax.bsl.parser.BSLParserRuleContext;
import org.antlr.v4.runtime.tree.ParseTree;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.Map;
import java.util.stream.Collectors;
Expand All @@ -37,6 +38,20 @@ public class CfgBuildingParseTreeVisitor extends BSLParserBaseVisitor<ParseTree>
private Map<String, LabelVertex> jumpLabels;

private boolean produceLoopIterationsEnabled = true;
private boolean producePreprocessorConditionsEnabled = true;
private boolean adjacentDeadCodeEnabled = false;

public void produceLoopIterations(boolean enable) {
produceLoopIterationsEnabled = enable;
}

public void producePreprocessorConditions(boolean enable) {
producePreprocessorConditionsEnabled = enable;
}

public void determineAdjacentDeadCode(boolean enabled) {
adjacentDeadCodeEnabled = enabled;
}

public ControlFlowGraph buildGraph(BSLParser.CodeBlockContext block) {

Expand Down Expand Up @@ -68,10 +83,6 @@ public ControlFlowGraph buildGraph(BSLParser.CodeBlockContext block) {
return graph;
}

public void produceLoopIterations(boolean flag) {
produceLoopIterationsEnabled = flag;
}

@Override
public ParseTree visitCallStatement(BSLParser.CallStatementContext ctx) {
blocks.addStatement(ctx);
Expand Down Expand Up @@ -137,7 +148,7 @@ public ParseTree visitIfStatement(BSLParser.IfStatementContext ctx) {

while (!currentLevelBlock.getBuildParts().isEmpty()) {
var blockTail = currentLevelBlock.getBuildParts().pop();
if (graph.incomingEdgesOf(blockTail).isEmpty() && blockTail instanceof BasicBlockVertex) {
if (hasNoSignificantEdges(blockTail) && blockTail instanceof BasicBlockVertex) {
// это мертвый код. Он может быть пустым блоком
// тогда он не нужен сам по себе
var basicBlock = (BasicBlockVertex) blockTail;
Expand All @@ -150,6 +161,11 @@ public ParseTree visitIfStatement(BSLParser.IfStatementContext ctx) {
return ctx;
}

private boolean hasNoSignificantEdges(CfgVertex blockTail) {
var edges = graph.incomingEdgesOf(blockTail);
return edges.isEmpty() || (adjacentDeadCodeEnabled && edges.stream().allMatch(x -> x.getType() == CfgEdgeType.ADJACENT_CODE));
}

@Override
public ParseTree visitElsifBranch(BSLParser.ElsifBranchContext ctx) {

Expand Down Expand Up @@ -240,11 +256,13 @@ public ParseTree visitGotoStatement(BSLParser.GotoStatementContext ctx) {
var labelVertex = createOrGetKnownLabel(name);

var block = blocks.getCurrentBlock();
var currentTail = blocks.getCurrentBlock().end();
connectGraphTail(block, labelVertex);

// создадим новый end, в который будут помещаться все будущие строки
block.split();
graph.addVertex(block.end());
connectAdjacentCode(currentTail);

return ctx;
}
Expand Down Expand Up @@ -272,24 +290,30 @@ public ParseTree visitLabel(BSLParser.LabelContext ctx) {
@Override
public ParseTree visitContinueStatement(BSLParser.ContinueStatementContext ctx) {
blocks.addStatement(ctx);
var currentTail = blocks.getCurrentBlock().end();
var jumps = blocks.getCurrentBlock().getJumpContext();
makeJump(jumps.loopContinue);
connectAdjacentCode(currentTail);
return ctx;
}

@Override
public ParseTree visitReturnStatement(BSLParser.ReturnStatementContext ctx) {
blocks.addStatement(ctx);
var currentTail = blocks.getCurrentBlock().end();
var jumps = blocks.getCurrentBlock().getJumpContext();
makeJump(jumps.methodReturn);
connectAdjacentCode(currentTail);
return ctx;
}

@Override
public ParseTree visitBreakStatement(BSLParser.BreakStatementContext ctx) {
blocks.addStatement(ctx);
var currentTail = blocks.getCurrentBlock().end();
var jumps = blocks.getCurrentBlock().getJumpContext();
makeJump(jumps.loopBreak);
connectAdjacentCode(currentTail);
return ctx;
}

Expand Down Expand Up @@ -334,14 +358,20 @@ public ParseTree visitTryStatement(BSLParser.TryStatementContext ctx) {
@Override
public ParseTree visitRaiseStatement(BSLParser.RaiseStatementContext ctx) {
blocks.addStatement(ctx);
var currentTail = blocks.getCurrentBlock().end();
var jumps = blocks.getCurrentBlock().getJumpContext();
makeJump(jumps.exceptionHandler);
connectAdjacentCode(currentTail);
return ctx;
}

@Override
public ParseTree visitPreproc_if(BSLParser.Preproc_ifContext ctx) {

if (!producePreprocessorConditionsEnabled) {
return ctx;
}

if (!isStatementLevelPreproc(ctx))
return super.visitPreproc_if(ctx);

Expand All @@ -365,6 +395,10 @@ public ParseTree visitPreproc_if(BSLParser.Preproc_ifContext ctx) {
@Override
public ParseTree visitPreproc_else(BSLParser.Preproc_elseContext ctx) {

if (!producePreprocessorConditionsEnabled) {
return ctx;
}

// По грамматике это может быть оторванный препроцессор, без начала
var condition = popPreprocCondition();
if (condition == null) {
Expand All @@ -386,6 +420,11 @@ public ParseTree visitPreproc_else(BSLParser.Preproc_elseContext ctx) {

@Override
public ParseTree visitPreproc_elsif(BSLParser.Preproc_elsifContext ctx) {

if (!producePreprocessorConditionsEnabled) {
return ctx;
}

// По грамматике это может быть оторванный препроцессор, без начала
var condition = popPreprocCondition();
if (condition == null) {
Expand All @@ -412,6 +451,10 @@ public ParseTree visitPreproc_elsif(BSLParser.Preproc_elsifContext ctx) {
@Override
public ParseTree visitPreproc_endif(BSLParser.Preproc_endifContext ctx) {

if (!producePreprocessorConditionsEnabled) {
return ctx;
}

// проверка маркера
var condition = popPreprocCondition();
if (condition == null) {
Expand All @@ -434,6 +477,15 @@ public ParseTree visitPreproc_endif(BSLParser.Preproc_endifContext ctx) {
// присоединяем все прямые выходы из тел условий
while (!mainIf.getBuildParts().isEmpty()) {
var blockTail = mainIf.getBuildParts().pop();
if (hasNoSignificantEdges(blockTail) && blockTail instanceof BasicBlockVertex) {
// это мертвый код. Он может быть пустым блоком
// тогда он не нужен сам по себе
var basicBlock = (BasicBlockVertex) blockTail;
if (basicBlock.statements().isEmpty()) {
graph.removeVertex(basicBlock);
continue;
}
}
graph.addVertex(blockTail);
graph.addEdge(blockTail, upperBlock.end());
}
Expand All @@ -454,6 +506,13 @@ private PreprocessorConditionVertex popPreprocCondition() {
return null;
}

private void connectAdjacentCode(CfgVertex currentTail) {
if (adjacentDeadCodeEnabled) {
var newTail = blocks.getCurrentBlock().end();
graph.addEdge(currentTail, newTail, CfgEdgeType.ADJACENT_CODE);
}
}

private void makeJump(CfgVertex jumpTarget) {
connectGraphTail(blocks.getCurrentBlock(), jumpTarget);
blocks.getCurrentBlock().split();
Expand All @@ -479,8 +538,9 @@ private void buildLoopSubgraph(BSLParser.CodeBlockContext ctx, LoopVertex loopSt

graph.addEdge(loopStart, body.begin(), CfgEdgeType.TRUE_BRANCH);
graph.addEdge(loopStart, blocks.getCurrentBlock().end(), CfgEdgeType.FALSE_BRANCH);
if (produceLoopIterationsEnabled)
if (produceLoopIterationsEnabled) {
graph.addEdge(body.end(), loopStart, CfgEdgeType.LOOP_ITERATION);
}
}

private void connectGraphTail(StatementsBlockWriter.StatementsBlockRecord currentBlock, CfgVertex vertex) {
Expand All @@ -495,6 +555,11 @@ private void connectGraphTail(StatementsBlockWriter.StatementsBlockRecord curren
// перевести все связи на новую вершину
var incoming = graph.incomingEdgesOf(currentTail);
for (var edge : incoming) {
// ребра смежности не переключаем, т.к. текущий блок удаляется
if (edge.getType() == CfgEdgeType.ADJACENT_CODE) {
continue;
}

var source = graph.getEdgeSource(edge);
graph.addEdge(source, vertex, edge.getType());
}
Expand All @@ -509,7 +574,17 @@ private void connectGraphTail(StatementsBlockWriter.StatementsBlockRecord curren

private void removeOrphanedNodes() {
var orphans = graph.vertexSet().stream()
.filter(x -> graph.edgesOf(x).isEmpty() && !(x instanceof ExitVertex))
.filter(vertex -> !(vertex instanceof ExitVertex))
.filter(vertex -> {
var edges = new ArrayList<>(graph.edgesOf(vertex));

return edges.isEmpty() ||
adjacentDeadCodeEnabled &&
edges.size() == 1
&& edges.get(0).getType() == CfgEdgeType.ADJACENT_CODE
&& graph.getEdgeTarget(edges.get(0)) == vertex;

})
.collect(Collectors.toList());

// в одном стриме бывает ConcurrentModificationException
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/com/github/_1c_syntax/bsl/languageserver/cfg/CfgEdgeType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,5 @@ public enum CfgEdgeType {
TRUE_BRANCH,
FALSE_BRANCH,
LOOP_ITERATION,
// возможно не нужно. (быстрый переход за линейный блок, без учета ветвления внутри него)
TRAVERSAL_FAST_FORWARD
ADJACENT_CODE
}
3 changes: 2 additions & 1 deletion src/test/java/com/github/_1c_syntax/bsl/languageserver/cfg/ControlFlowGraphBuilderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ void testInnerLoops() {

var parseTree = parse(code);
var builder = new CfgBuildingParseTreeVisitor();
builder.determineAdjacentDeadCode(true);
var graph = builder.buildGraph(parseTree);

var walker = new ControlFlowGraphWalker(graph);
Expand All @@ -255,7 +256,7 @@ void testInnerLoops() {
var secondLoopStart = walker.getCurrentNode();
walker.walkNext(CfgEdgeType.TRUE_BRANCH);
assertThat(textOfCurrentNode(walker)).isEqualTo("Б=1");
assertThat(graph.outgoingEdgesOf(walker.getCurrentNode()).size()).isEqualTo(1);
assertThat(graph.outDegreeOf(walker.getCurrentNode())).isEqualTo(2);
walker.walkNext();
assertThat(textOfCurrentNode(walker)).isEqualTo("Прервано=Истина");

Expand Down

0 comments on commit 862a1ee

Please sign in to comment.