Skip to content

Commit fe8c909

Browse files
authored
Merge pull request #2810 from artbear/rewrite-parameter-1834
Правило "Перезапись параметров метода"
2 parents a5f3247 + e43d276 commit fe8c909

File tree

10 files changed

+449
-56
lines changed

10 files changed

+449
-56
lines changed
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
# Перезапись параметров метода (RewriteMethodParameter)
2+
3+
<!-- Блоки выше заполняются автоматически, не трогать -->
4+
## Описание диагностики
5+
<!-- Описание диагностики заполняется вручную. Необходимо понятным языком описать смысл и схему работу -->
6+
Иногда разработчик пишут функции таким образом, когда аргументы функции перезаписываются сразу на входе в функцию/процедуру.
7+
8+
Такое поведение вводит в заблуждение других разработчиков, которые вызывают подобные функции/процедуры.
9+
Эти функции нужно исправить. Например, убрать параметры, преобразовав их в локальные переменные.
10+
11+
## Примеры
12+
<!-- В данном разделе приводятся примеры, на которые диагностика срабатывает, а также можно привести пример, как можно исправить ситуацию -->
13+
Подозрительный код
14+
```bsl
15+
Процедура Конфигуратор(Знач СтрокаПодключения, Знач Пользователь = "", Знач Пароль = "") Экспорт
16+
СтрокаПодключения = "/F""" + КаталогБазы + """"; // Здесь
17+
...
18+
КонецФункции
19+
```
20+
21+
Исправленный код
22+
```bsl
23+
Процедура Конфигуратор(Знач КаталогБазы, Знач Пользователь = "", Знач Пароль = "") Экспорт
24+
СтрокаПодключения = "/F""" + КаталогБазы + """"; // Здесь
25+
...
26+
КонецФункции
27+
```
28+
или
29+
```bsl
30+
Процедура Конфигуратор(Знач КаталогБазы, Знач Пользователь = "", Знач Пароль = "") Экспорт
31+
Если Не ПустаяСтрока(КаталогБазы) Тогда
32+
НоваяСтрокаПодключения = "/F""" + КаталогБазы + """";
33+
Иначе
34+
НоваяСтрокаПодключения = СтрокаПодключения;
35+
КонецЕсли;
36+
...
37+
КонецФункции
38+
```
39+
40+
## Источники
41+
<!-- Необходимо указывать ссылки на все источники, из которых почерпнута информация для создания диагностики -->
42+
<!-- Примеры источников
43+
44+
* [PVS-Studio V763. Parameter is always rewritten in function body before being used](https://pvs-studio.com/ru/docs/warnings/v6023)
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
# Rewrite method parameter (RewriteMethodParameter)
2+
3+
<!-- Блоки выше заполняются автоматически, не трогать -->
4+
## Description
5+
<!-- Описание диагностики заполняется вручную. Необходимо понятным языком описать смысл и схему работу -->
6+
7+
## Examples
8+
<!-- В данном разделе приводятся примеры, на которые диагностика срабатывает, а также можно привести пример, как можно исправить ситуацию -->
9+
10+
## Sources
11+
<!-- Необходимо указывать ссылки на все источники, из которых почерпнута информация для создания диагностики -->
12+
<!-- Примеры источников
13+
14+
* Источник: [Стандарт: Тексты модулей](https://its.1c.ru/db/v8std#content:456:hdoc)
15+
* Полезная информация: [Отказ от использования модальных окон](https://its.1c.ru/db/metod8dev#content:5272:hdoc)
16+
* Источник: [Cognitive complexity, ver. 1.4](https://www.sonarsource.com/docs/CognitiveComplexity.pdf) -->
Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
/*
2+
* This file is a part of BSL Language Server.
3+
*
4+
* Copyright (c) 2018-2022
5+
* Alexey Sosnoviy <labotamy@gmail.com>, Nikita Fedkin <nixel2007@gmail.com> and contributors
6+
*
7+
* SPDX-License-Identifier: LGPL-3.0-or-later
8+
*
9+
* BSL Language Server is free software; you can redistribute it and/or
10+
* modify it under the terms of the GNU Lesser General Public
11+
* License as published by the Free Software Foundation; either
12+
* version 3.0 of the License, or (at your option) any later version.
13+
*
14+
* BSL Language Server is distributed in the hope that it will be useful,
15+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
16+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
17+
* Lesser General Public License for more details.
18+
*
19+
* You should have received a copy of the GNU Lesser General Public
20+
* License along with BSL Language Server.
21+
*/
22+
package com.github._1c_syntax.bsl.languageserver.diagnostics;
23+
24+
import com.github._1c_syntax.bsl.languageserver.context.symbol.MethodSymbol;
25+
import com.github._1c_syntax.bsl.languageserver.context.symbol.ParameterDefinition;
26+
import com.github._1c_syntax.bsl.languageserver.context.symbol.VariableSymbol;
27+
import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticMetadata;
28+
import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticSeverity;
29+
import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticTag;
30+
import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticType;
31+
import com.github._1c_syntax.bsl.languageserver.references.ReferenceIndex;
32+
import com.github._1c_syntax.bsl.languageserver.references.model.OccurrenceType;
33+
import com.github._1c_syntax.bsl.languageserver.references.model.Reference;
34+
import com.github._1c_syntax.bsl.languageserver.utils.RelatedInformation;
35+
import com.github._1c_syntax.bsl.languageserver.utils.Trees;
36+
import com.github._1c_syntax.bsl.parser.BSLParser;
37+
import lombok.RequiredArgsConstructor;
38+
import org.antlr.v4.runtime.tree.RuleNode;
39+
import org.antlr.v4.runtime.tree.TerminalNode;
40+
import org.apache.commons.lang3.tuple.Pair;
41+
import org.apache.commons.lang3.tuple.Triple;
42+
import org.eclipse.lsp4j.DiagnosticRelatedInformation;
43+
import org.eclipse.lsp4j.Position;
44+
import org.eclipse.lsp4j.Range;
45+
import org.eclipse.lsp4j.SymbolKind;
46+
47+
import java.util.ArrayList;
48+
import java.util.List;
49+
import java.util.Optional;
50+
import java.util.stream.Collectors;
51+
import java.util.stream.Stream;
52+
53+
@DiagnosticMetadata(
54+
type = DiagnosticType.CODE_SMELL,
55+
severity = DiagnosticSeverity.MAJOR,
56+
minutesToFix = 2,
57+
tags = {
58+
DiagnosticTag.SUSPICIOUS
59+
}
60+
)
61+
62+
@RequiredArgsConstructor
63+
public class RewriteMethodParameterDiagnostic extends AbstractDiagnostic {
64+
private static final int COUNT_OF_PAIR_FOR_SELF_ASSIGN = 2;
65+
private final ReferenceIndex referenceIndex;
66+
67+
@Override
68+
public void check() {
69+
documentContext.getSymbolTree().getMethods().stream()
70+
.flatMap(RewriteMethodParameterDiagnostic::getParametersByValue)
71+
.flatMap(pair -> getVariableByParameter(pair.getLeft(), pair.getRight()))
72+
.map(this::isOverwrited)
73+
.filter(Optional::isPresent)
74+
.map(Optional::get)
75+
.forEach(variableSymbolReferenceListTriple -> fireIssue(variableSymbolReferenceListTriple.getLeft(),
76+
variableSymbolReferenceListTriple.getMiddle(),
77+
variableSymbolReferenceListTriple.getRight()));
78+
79+
}
80+
81+
private static Stream<Pair<MethodSymbol, ParameterDefinition>> getParametersByValue(MethodSymbol methodSymbol) {
82+
return methodSymbol.getParameters().stream()
83+
.filter(ParameterDefinition::isByValue)
84+
.map(parameterDefinition -> Pair.of(methodSymbol, parameterDefinition));
85+
}
86+
87+
private static Stream<VariableSymbol> getVariableByParameter(MethodSymbol method, ParameterDefinition parameterDefinition) {
88+
return method.getChildren().stream()
89+
// в будущем могут появиться и другие символы, подчиненные методам
90+
.filter(sourceDefinedSymbol -> sourceDefinedSymbol.getSymbolKind() == SymbolKind.Variable)
91+
.filter(variable -> parameterDefinition.getRange().getStart().equals(variable.getSelectionRange().getStart()))
92+
.filter(VariableSymbol.class::isInstance)
93+
.map(VariableSymbol.class::cast)
94+
.findFirst().stream();
95+
}
96+
97+
private Optional<Triple<VariableSymbol, Reference, List<Reference>>> isOverwrited(VariableSymbol variable) {
98+
final var references = getSortedReferencesByLocation(variable);
99+
return isOverwrited(references)
100+
.map(defReference -> Triple.of(variable, defReference, references));
101+
}
102+
103+
private List<Reference> getSortedReferencesByLocation(VariableSymbol variable) {
104+
final var references = referenceIndex.getReferencesTo(variable);
105+
return references.stream()
106+
.sorted((o1, o2) -> compare(o1.getSelectionRange(), o2.getSelectionRange()))
107+
.collect(Collectors.toList());
108+
}
109+
110+
private static int compare(Range o1, Range o2) {
111+
return compare(o1.getStart(), o2.getStart());
112+
}
113+
114+
public static int compare(Position pos1, Position pos2) {
115+
// 1,1 10,10
116+
if (pos1.getLine() < pos2.getLine()) {
117+
return -1;
118+
}
119+
// 10,10 1,1
120+
if (pos1.getLine() > pos2.getLine()) {
121+
return 1;
122+
}
123+
// 1,4 1,9
124+
return Integer.compare(pos1.getCharacter(), pos2.getCharacter());
125+
// 1,9 1,4
126+
}
127+
128+
private Optional<Reference> isOverwrited(List<Reference> references) {
129+
if (!references.isEmpty()) {
130+
final var firstDefIntoAssign = references.get(0);
131+
if (firstDefIntoAssign.getOccurrenceType() == OccurrenceType.DEFINITION) {
132+
if (references.size() == 1) {
133+
return Optional.of(firstDefIntoAssign);
134+
}
135+
var refContextInsideDefAssign = getRefContextInsideDefAssign(firstDefIntoAssign, references.get(1));
136+
if (refContextInsideDefAssign.isEmpty()) {
137+
return Optional.of(firstDefIntoAssign);
138+
}
139+
var isSelfAssign = Boolean.TRUE.equals(refContextInsideDefAssign
140+
.map(RewriteMethodParameterDiagnostic::isVarNameOnlyIntoExpression)
141+
.orElseThrow());
142+
if (isSelfAssign && references.size() > COUNT_OF_PAIR_FOR_SELF_ASSIGN) {
143+
return isOverwrited(references.subList(COUNT_OF_PAIR_FOR_SELF_ASSIGN, references.size()));
144+
}
145+
}
146+
}
147+
return Optional.empty();
148+
}
149+
150+
private Optional<RuleNode> getRefContextInsideDefAssign(Reference defRef, Reference nextRef) {
151+
final var defNode = Trees.findTerminalNodeContainsPosition(documentContext.getAst(),
152+
defRef.getSelectionRange().getStart());
153+
final var assignment = defNode
154+
.map(TerminalNode::getParent)
155+
.filter(BSLParser.LValueContext.class::isInstance)
156+
.map(RuleNode::getParent)
157+
.filter(BSLParser.AssignmentContext.class::isInstance)
158+
.map(BSLParser.AssignmentContext.class::cast);
159+
160+
return assignment.flatMap(assignContext ->
161+
Trees.findTerminalNodeContainsPosition(assignContext, nextRef.getSelectionRange().getStart()))
162+
.map(TerminalNode::getParent);
163+
}
164+
165+
private static boolean isVarNameOnlyIntoExpression(RuleNode refContext) {
166+
return Optional.of(refContext)
167+
.filter(BSLParser.ComplexIdentifierContext.class::isInstance)
168+
.map(BSLParser.ComplexIdentifierContext.class::cast)
169+
.filter(node -> node.getChildCount() == 1)
170+
.map(RuleNode::getParent)
171+
.filter(BSLParser.MemberContext.class::isInstance)
172+
.map(RuleNode::getParent)
173+
.filter(expression -> expression.getChildCount() == 1)
174+
.filter(BSLParser.ExpressionContext.class::isInstance)
175+
.isPresent();
176+
}
177+
178+
private void fireIssue(VariableSymbol variable, Reference nodeForIssue, List<Reference> references) {
179+
var refsForIssue = references.stream()
180+
.map(reference -> RelatedInformation.create(
181+
documentContext.getUri(),
182+
reference.getSelectionRange(),
183+
"+1"
184+
)).collect(Collectors.toList());
185+
var resultRefs = new ArrayList<DiagnosticRelatedInformation>();
186+
resultRefs.add(RelatedInformation.create(
187+
documentContext.getUri(),
188+
variable.getVariableNameRange(),
189+
"0"));
190+
resultRefs.addAll(refsForIssue);
191+
192+
diagnosticStorage.addDiagnostic(nodeForIssue.getSelectionRange(), info.getMessage(variable.getName()), resultRefs);
193+
}
194+
}

src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SelectionRangeProvider.java

Lines changed: 1 addition & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -27,14 +27,9 @@
2727
import com.github._1c_syntax.bsl.parser.BSLParser;
2828
import com.github._1c_syntax.bsl.parser.BSLParserRuleContext;
2929
import org.antlr.v4.runtime.ParserRuleContext;
30-
import org.antlr.v4.runtime.Token;
3130
import org.antlr.v4.runtime.tree.ParseTree;
32-
import org.antlr.v4.runtime.tree.TerminalNode;
33-
import org.antlr.v4.runtime.tree.Tree;
34-
import org.eclipse.lsp4j.Position;
3531
import org.eclipse.lsp4j.SelectionRange;
3632
import org.eclipse.lsp4j.SelectionRangeParams;
37-
import org.eclipse.lsp4j.util.Positions;
3833
import org.springframework.stereotype.Component;
3934

4035
import javax.annotation.CheckForNull;
@@ -91,7 +86,7 @@ public List<SelectionRange> getSelectionRange(DocumentContext documentContext, S
9186

9287
// Result must contains all elements from input
9388
return positions.stream()
94-
.map(position -> findNodeContainsPosition(ast, position))
89+
.map(position -> Trees.findTerminalNodeContainsPosition(ast, position))
9590
.map(terminalNode -> terminalNode.orElse(null))
9691
.map(SelectionRangeProvider::toSelectionRange)
9792
.collect(Collectors.toList());
@@ -216,54 +211,4 @@ private static boolean ifBranchMatchesIfStatement(BSLParserRuleContext ctx) {
216211
return ifStatement.elseBranch() == null && ifStatement.elsifBranch().isEmpty();
217212
}
218213

219-
private static Optional<TerminalNode> findNodeContainsPosition(BSLParserRuleContext tree, Position position) {
220-
221-
if (tree.getTokens().isEmpty()) {
222-
return Optional.empty();
223-
}
224-
225-
var start = tree.getStart();
226-
var stop = tree.getStop();
227-
228-
if (!(positionIsAfterOrOnToken(position, start) && positionIsBeforeOrOnToken(position, stop))) {
229-
return Optional.empty();
230-
}
231-
232-
var children = Trees.getChildren(tree);
233-
234-
for (Tree child : children) {
235-
if (child instanceof TerminalNode) {
236-
var terminalNode = (TerminalNode) child;
237-
var token = terminalNode.getSymbol();
238-
if (tokenContainsPosition(token, position)) {
239-
return Optional.of(terminalNode);
240-
}
241-
} else {
242-
Optional<TerminalNode> node = findNodeContainsPosition((BSLParserRuleContext) child, position);
243-
if (node.isPresent()) {
244-
return node;
245-
}
246-
}
247-
}
248-
249-
return Optional.empty();
250-
}
251-
252-
private static boolean tokenContainsPosition(Token token, Position position) {
253-
var tokenRange = Ranges.create(token);
254-
return Ranges.containsPosition(tokenRange, position);
255-
}
256-
257-
private static boolean positionIsBeforeOrOnToken(Position position, Token token) {
258-
var tokenRange = Ranges.create(token);
259-
var end = tokenRange.getEnd();
260-
return Positions.isBefore(position, end) || end.equals(position);
261-
}
262-
263-
private static boolean positionIsAfterOrOnToken(Position position, Token token) {
264-
var tokenRange = Ranges.create(token);
265-
var start = tokenRange.getStart();
266-
return Positions.isBefore(start, position) || start.equals(position);
267-
}
268-
269214
}

0 commit comments

Comments
 (0)