Skip to content

Commit

Permalink
Merge pull request #3210 from artbear/magic-number
Browse files Browse the repository at this point in the history 
[MOD] MagicNumber - ловим магические числа везде, в т.ч. и при передаче параметров - ГОТОВО
  • Loading branch information
@theshadowco
theshadowco authored Dec 18, 2023
2 parents 8afa0bb + 9c820db commit b174b48
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 50 deletions.
49 changes: 33 additions & 16 deletions ...main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,14 @@
import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticType;
import com.github._1c_syntax.bsl.languageserver.utils.DiagnosticHelper;
import com.github._1c_syntax.bsl.parser.BSLParser;
import org.antlr.v4.runtime.ParserRuleContext;
import com.github._1c_syntax.bsl.parser.BSLParserRuleContext;
import org.antlr.v4.runtime.tree.ParseTree;

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

@DiagnosticMetadata(
type = DiagnosticType.CODE_SMELL,
Expand All @@ -49,7 +50,6 @@ public class MagicNumberDiagnostic extends AbstractVisitorDiagnostic {
private static final String DEFAULT_AUTHORIZED_NUMBERS = "-1,0,1";
private static final boolean DEFAULT_ALLOW_MAGIC_NUMBER = true;


@DiagnosticParameter(
type = String.class,
defaultValue = "" + DEFAULT_AUTHORIZED_NUMBERS
Expand All @@ -75,37 +75,54 @@ public void configure(Map<String, Object> configuration) {
}
}

private static Optional<BSLParser.ExpressionContext> getExpression(BSLParserRuleContext ctx) {
return Optional.of(ctx)
.filter(context -> context.getChildCount() == 1)
.map(BSLParserRuleContext::getParent)
.filter(context -> context.getChildCount() == 1)
.map(BSLParserRuleContext::getParent)
.filter(BSLParser.ExpressionContext.class::isInstance)
.map(BSLParser.ExpressionContext.class::cast);
}

private static boolean isNumericExpression(BSLParser.ExpressionContext expression) {
return (expression.getChildCount() <= 1);
}

private static boolean insideCallParam(BSLParser.ExpressionContext expression) {
return expression.getParent() instanceof BSLParser.CallParamContext;
}

@Override
public ParseTree visitNumeric(BSLParser.NumericContext ctx) {
String checked = ctx.getText();

if (checked != null && !isExcluded(checked)) {
ParserRuleContext expression = ctx.getParent().getParent().getParent();
if (expression instanceof BSLParser.ExpressionContext
&& (!isNumericExpression((BSLParser.ExpressionContext) expression)
|| mayBeNumberAccess((BSLParser.ExpressionContext) expression))) {
if (checked != null && isAllowed(checked)) {
final var parent = ctx.getParent();
if (parent.getParent() instanceof BSLParser.DefaultValueContext || isWrongExpression(parent)) {
diagnosticStorage.addDiagnostic(ctx.stop, info.getMessage(checked));
}
}

return ctx;
return defaultResult();
}

private boolean isExcluded(String s) {
private boolean isAllowed(String s) {
for (String elem : this.authorizedNumbers) {
if (s.compareTo(elem) == 0) {
return true;
return false;
}
}
return true;
}

return false;
private boolean isWrongExpression(BSLParserRuleContext numericContextParent) {
return getExpression(numericContextParent)
.filter((BSLParser.ExpressionContext expression) ->
(!isNumericExpression(expression) || mayBeNumberAccess(expression) || insideCallParam(expression)))
.isPresent();
}

private boolean mayBeNumberAccess(BSLParser.ExpressionContext expression) {
return !allowMagicIndexes && expression.getParent() instanceof BSLParser.AccessIndexContext;
}

private static boolean isNumericExpression(BSLParser.ExpressionContext expression) {
return (expression.getChildCount() <= 1);
}
}
61 changes: 38 additions & 23 deletions .../java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnosticTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,20 @@ void runTest() {
List<Diagnostic> diagnostics = getDiagnostics();

// then
assertThat(diagnostics).hasSize(7);
assertThat(diagnostics).hasSize(12);
assertThat(diagnostics, true)
.hasRange(3, 18, 3, 20)
.hasRange(3, 23, 3, 25)
.hasRange(7, 31, 7, 33)
.hasRange(11, 20, 11, 21)
.hasRange(20, 21, 20, 23)
.hasRange(23, 24, 23, 26)
.hasRange(27, 34, 27, 35);
.hasRange(3, 18, 20)
.hasRange(3, 23, 25)
.hasRange(7, 31, 33)
.hasRange(11, 20, 21)
.hasRange(20, 21, 23)
.hasRange(23, 24, 26)
.hasRange(27, 34, 35)
.hasRange(33, 37, 38)
.hasRange(34, 37, 38)
.hasRange(36, 87, 88)
.hasRange(37, 55, 56)
.hasRange(41, 16, 19);
}

@Test
Expand All @@ -64,12 +69,17 @@ void testConfigure() {
List<Diagnostic> diagnostics = getDiagnostics();

// then
assertThat(diagnostics).hasSize(4);
assertThat(diagnostics).hasSize(9);
assertThat(diagnostics, true)
.hasRange(7, 31, 7, 33)
.hasRange(11, 20, 11, 21)
.hasRange(20, 21, 20, 23)
.hasRange(23, 24, 23, 26);
.hasRange(7, 31, 33)
.hasRange(11, 20, 21)
.hasRange(20, 21, 23)
.hasRange(23, 24, 26)
.hasRange(33, 37, 38)
.hasRange(34, 37, 38)
.hasRange(36, 87, 88)
.hasRange(37, 55, 56)
.hasRange(41, 16, 19);
}

@Test
Expand All @@ -83,17 +93,22 @@ void testIndexes() {
List<Diagnostic> diagnostics = getDiagnostics();

// then
assertThat(diagnostics).hasSize(9);
assertThat(diagnostics).hasSize(14);
assertThat(diagnostics, true)
.hasRange(3, 18, 3, 20)
.hasRange(3, 23, 3, 25)
.hasRange(7, 31, 7, 33)
.hasRange(11, 20, 11, 21)
.hasRange(20, 21, 20, 23)
.hasRange(23, 24, 23, 26)
.hasRange(27, 34, 27, 35)
.hasRange(53, 32, 53, 34)
.hasRange(54, 18, 54, 20);
.hasRange(3, 18, 20)
.hasRange(3, 23, 25)
.hasRange(7, 31, 33)
.hasRange(11, 20, 21)
.hasRange(20, 21, 23)
.hasRange(23, 24, 26)
.hasRange(27, 34, 35)
.hasRange(33, 37, 38)
.hasRange(34, 37, 38)
.hasRange(36, 87, 88)
.hasRange(37, 55, 56)
.hasRange(41, 16, 19)
.hasRange(52, 32, 34)
.hasRange(53, 18, 20);
}

}
21 changes: 10 additions & 11 deletions src/test/resources/diagnostics/MagicNumberDiagnostic.bsl
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
Процедура ПроверкаЧисел()

ПонятнаяПеременная = 6; // Нет ошибки
СекундВЧасе = 60 * 60; // Ошибкат на двух числах
СекундВЧасе = 60 * 60; // Ошибка на двух числах

Если ТекущаяДатаИВремя > СекундВЧасе Тогда // Нет ошибки

Expand All @@ -25,22 +25,21 @@

КонецЕсли;

ЭтоВоскресенье = ДеньНедели = 7; // Тут ошибка, хоть и вглядит нормально.
ЭтоВоскресенье = ДеньНедели = 7; // Тут ошибка, хоть и выглядит нормально.
ДеньНеделиВоскресенье = 7;
ЭтоВоскресенье = ДеньНедели = ДеньНеделиВоскресенье; // А вот тут уже ошибки нет

// Далеше без ошибок
ПроверочноеПеречисление = Новый Массив;
ПроверочноеПеречисление.Добавить(1);
ПроверочноеПеречисление.Добавить(2);
ПроверочноеПеречисление.Добавить(3);
ПроверочноеПеречисление.Добавить(1); // Нет ошибки из-за исключения
ПроверочноеПеречисление.Добавить(2); // ошибка
ПроверочноеПеречисление.Добавить(3); // ошибка

ПроверочнаяСтруктура = Новый Структура("Авто,ПростойВариант,СложныйВариант", 0, 1, 2);
ПроверочнаяСтруктура.Добавить("ЭкспертныйВариант", 3);
ПроверочнаяСтруктура = Новый Структура("Авто,ПростойВариант,СложныйВариант", 0, 1, 2); // ошибка только на 2
ПроверочнаяСтруктура.Добавить("ЭкспертныйВариант", 3); // ошибка

КонецПроцедуры

Процедура А(А = 566)
Процедура А(А = 566) // пропущенная ошибка

КонецПроцедуры

Expand All @@ -51,6 +50,6 @@
КонецФункции

Процедура Индексы()
Индекс1 = Коллекция.Индексы[20];
Метод(Индексы[21])
Индекс1 = Коллекция.Индексы[20]; // замечание при allowMagicIndexes = false
Метод(Индексы[21]) // замечание при allowMagicIndexes = false
КонецПроцедуры

0 comments on commit b174b48

Please sign in to comment.