From 01edbe2f566f37f44a6f0a235a33c3dea03f326d Mon Sep 17 00:00:00 2001 From: Artur Ayukhanov Date: Sat, 11 Jun 2022 00:47:13 +0300 Subject: [PATCH 1/2] =?UTF-8?q?=D0=A4=D0=9F=20=D0=B4=D0=BB=D1=8F=20=D1=82?= =?UTF-8?q?=D0=B0=D0=B1=D0=BB=D0=B8=D1=87=D0=BD=D1=8B=D1=85=20=D1=87=D0=B0?= =?UTF-8?q?=D1=81=D1=82=D0=B5=D0=B9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - предрасчет таблиц - исправил работу с объединениями - перевод на новый парсер запросов - ускорение - замечания Сонара - рефакторинг closes #1800 --- .../diagnostics/RefOveruseDiagnostic.java | 122 ++++++++++-------- .../diagnostics/RefOveruseDiagnosticTest.java | 5 +- .../diagnostics/RefOveruseDiagnostic.bsl | 22 +++- 3 files changed, 90 insertions(+), 59 deletions(-) diff --git a/src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnostic.java b/src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnostic.java index 68698099706..4617c0d4147 100644 --- a/src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnostic.java +++ b/src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnostic.java @@ -32,12 +32,13 @@ import com.github._1c_syntax.utils.CaseInsensitivePattern; import org.antlr.v4.runtime.tree.ParseTree; -import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; +import java.util.Map; import java.util.Optional; -import java.util.Set; import java.util.regex.Pattern; import java.util.stream.Collectors; +import java.util.stream.Stream; @DiagnosticMetadata( type = DiagnosticType.CODE_SMELL, @@ -51,53 +52,79 @@ ) public class RefOveruseDiagnostic extends AbstractSDBLVisitorDiagnostic { - private static final String REF_REGEX = "Ссылка|Reference"; - private static final Pattern REF_PATTERN = CaseInsensitivePattern.compile(REF_REGEX); + private static final Pattern REF_PATTERN = CaseInsensitivePattern.compile("Ссылка|Reference"); private static final int BAD_CHILD_COUNT = 3; - private Collection dataSourceCollection = new ArrayList<>(); + public static final int COUNT_OF_TABLE_DOT_REF_DOT_REF = 5; + private Map dataSourcesWithTabularFlag = Collections.emptyMap(); @Override public ParseTree visitQuery(SDBLParser.QueryContext ctx) { + checkQuery(ctx).forEach(diagnosticStorage::addDiagnostic); + return ctx; + } + + private Stream checkQuery(SDBLParser.QueryContext ctx) { var columnsCollection = Trees.findAllRuleNodes(ctx, SDBLParser.RULE_column); - if (columnsCollection.isEmpty() - || dataSourceCollection.stream().anyMatch(Trees::treeContainsErrors)) { - return ctx; + if (columnsCollection.isEmpty()) { + return Stream.empty(); } - if (dataSourceCollection.isEmpty()) { - performSimpleCheck(columnsCollection); - return ctx; + dataSourcesWithTabularFlag = dataSourcesWithTabularSection(ctx); + if (dataSourcesWithTabularFlag.isEmpty()) { + return getSimpleOverused(columnsCollection); } - var tableNames = dataSourceCollection.stream() - .map(RefOveruseDiagnostic::getTableNameOrAlias) - .collect(Collectors.toSet()); + return getOverused(columnsCollection); + } - columnsCollection.forEach(column -> checkColumnNode((SDBLParser.ColumnContext) column, tableNames)); - return ctx; + private static Map dataSourcesWithTabularSection(SDBLParser.QueryContext ctx) { + return Trees.findAllRuleNodes(ctx, SDBLParser.RULE_dataSource).stream() + .map(SDBLParser.DataSourceContext.class::cast) + .collect(Collectors.toMap(RefOveruseDiagnostic::getTableNameOrAlias, + RefOveruseDiagnostic::isTableWithTabularSection)); + } + private static String getTableNameOrAlias(SDBLParser.DataSourceContext dataSource) { + final var value = Optional.of(dataSource); + return value + .map(SDBLParser.DataSourceContext::alias) + .map(alias -> (ParseTree)alias.name) + .or(() -> value + .map(SDBLParser.DataSourceContext::table) + .map(tableContext -> (ParseTree)tableContext.tableName)) + .or(() -> value + .map(SDBLParser.DataSourceContext::parameterTable) + .map(tableContext -> (ParseTree)tableContext.parameter())) + .map(ParseTree::getText) + .orElse(""); } - @Override - public ParseTree visitSelectQuery(SDBLParser.SelectQueryContext ctx) { - this.dataSourceCollection = Trees.findAllRuleNodes(ctx, SDBLParser.RULE_dataSource); - return super.visitSelectQuery(ctx); + private static boolean isTableWithTabularSection(SDBLParser.DataSourceContext dataSourceContext) { + final var table = dataSourceContext.table(); + if (table == null) { + return dataSourceContext.virtualTable() != null; + } + return table.tableName != null || table.objectTableName != null; } - private void performSimpleCheck(Collection columnsCollection) { - columnsCollection.stream() + private static Stream getSimpleOverused(Collection columnsCollection) { + return columnsCollection.stream() .filter(columnNode -> columnNode.getChildCount() > BAD_CHILD_COUNT) .map(column -> column.getChild(column.getChildCount() - 1)) .filter(lastChild -> REF_PATTERN.matcher(lastChild.getText()).matches()) - .forEach(node -> diagnosticStorage.addDiagnostic((BSLParserRuleContext) node)); + .map(BSLParserRuleContext.class::cast); } - private void checkColumnNode(SDBLParser.ColumnContext ctx, Set tableNames) { + private Stream getOverused(Collection columnsCollection) { + return columnsCollection.stream() + .map(SDBLParser.ColumnContext.class::cast) + .filter(column -> column.getChildCount() >= BAD_CHILD_COUNT) + .filter(this::isOveruse) + .map(BSLParserRuleContext.class::cast); + } - if (ctx.children == null) { - return; - } + private boolean isOveruse(SDBLParser.ColumnContext ctx) { // children: // @@ -111,39 +138,24 @@ private void checkColumnNode(SDBLParser.ColumnContext ctx, Set tableName final int childCount = ctx.children.size(); - if (childCount < 3) { - return; - } - - var lastChild = ctx.getChild(childCount - 1); // dots are also children of ColumnContext, // that is why -3 must be an index of penultimate identifier - var penultimateChild = ctx.getChild(childCount - 3); + var penultimateChild = ctx.getChild(childCount - BAD_CHILD_COUNT); - String lastIdentifierName = lastChild.getText(); String penultimateIdentifierName = penultimateChild.getText(); - if (REF_PATTERN.matcher(penultimateIdentifierName).matches() - || (REF_PATTERN.matcher(lastIdentifierName).matches() - && !tableNames.contains(penultimateIdentifierName))) { - diagnosticStorage.addDiagnostic(ctx); + if (REF_PATTERN.matcher(penultimateIdentifierName).matches()) { + if (childCount < COUNT_OF_TABLE_DOT_REF_DOT_REF){ + return true; + } + var prevChildID = ctx.getChild(childCount - COUNT_OF_TABLE_DOT_REF_DOT_REF).getText(); + return !dataSourcesWithTabularFlag.getOrDefault(prevChildID, false); } - } - - private static String getTableNameOrAlias(ParseTree dataSource) { - return Optional.of(dataSource) - .flatMap(dataSrc -> extractTextFromChild(dataSrc, SDBLParser.RULE_alias)) - .or(() -> Optional.of(dataSource) - .flatMap(dataSrc -> extractTextFromChild(dataSrc, SDBLParser.RULE_table))) - .or(() -> Optional.of(dataSource) - .flatMap(dataSrc -> extractTextFromChild(dataSrc, SDBLParser.RULE_parameterTable))) - .orElse(""); - } - - private static Optional extractTextFromChild(ParseTree parseTree, int childRuleType) { - return Optional.of(parseTree) - .flatMap(tree -> Trees.getFirstChild(tree, childRuleType)) - .flatMap(child -> Trees.getFirstChild(child, SDBLParser.RULE_identifier)) - .map(BSLParserRuleContext::getText); + var lastChild = ctx.getChild(childCount - 1); + String lastIdentifierName = lastChild.getText(); + if (REF_PATTERN.matcher(lastIdentifierName).matches()) { + return dataSourcesWithTabularFlag.get(penultimateIdentifierName) == null; + } + return false; } } diff --git a/src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnosticTest.java b/src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnosticTest.java index 030a0dc03f3..04d28823289 100644 --- a/src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnosticTest.java +++ b/src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnosticTest.java @@ -38,7 +38,6 @@ void test() { List diagnostics = getDiagnostics(); - assertThat(diagnostics).hasSize(12); assertThat(diagnostics, true) .hasRange(3, 28, 3, 45) .hasRange(13, 8, 13, 34) @@ -51,7 +50,7 @@ void test() { .hasRange(92, 8, 29) .hasRange(153, 13, 153, 41) .hasRange(164, 13, 164, 53) - .hasRange(178, 13, 178, 35); - + .hasRange(178, 13, 178, 35) + .hasSize(12); } } diff --git a/src/test/resources/diagnostics/RefOveruseDiagnostic.bsl b/src/test/resources/diagnostics/RefOveruseDiagnostic.bsl index 02b3e329232..bc968112e77 100644 --- a/src/test/resources/diagnostics/RefOveruseDiagnostic.bsl +++ b/src/test/resources/diagnostics/RefOveruseDiagnostic.bsl @@ -179,10 +179,30 @@ | Контрагенты.Ссылка.ИНН = &ИНН"; // ошибка КонецПроцедуры -Процедура Тест17() +Процедура Тест16() ТекстЗапроса = "ВЫБРАТЬ | Артикул КАК Артикул // не задано имя таблицы |ИЗ | Справочник.Номенклатура КАК Номенклатура"; КонецПроцедуры + +Процедура Тест17() + ТекстЗапроса = + "ВЫБРАТЬ + | Таблица.Ссылка КАК Ссылка + |ИЗ + | Документ.Документ1.ТабличнаяЧасть1 КАК Таблица + |ГДЕ + | Таблица.Ссылка.Реквизит1 = &ИНН"; // не ошибка +КонецПроцедуры + +Процедура Тест18() + ТекстЗапроса = + "ВЫБРАТЬ + | Таблица.Ссылка КАК Ссылка + |ИЗ + | РегистрСведений.РегистрСведений1.СрезПоследних КАК Таблица + |ГДЕ + | Таблица.Ссылка.Реквизит1 = &ИНН"; // не ошибка +КонецПроцедуры From 37663932f2ca2f97c7d6b2d1a5b22d332161de5e Mon Sep 17 00:00:00 2001 From: Artur Ayukhanov Date: Tue, 21 Jun 2022 14:01:04 +0300 Subject: [PATCH 2/2] =?UTF-8?q?=D0=BA=D0=B5=D0=B9=D1=81=20=D1=81=20=D0=BF?= =?UTF-8?q?=D0=B0=D0=B4=D0=B5=D0=BD=D0=B8=D0=B5=D0=BC=20=D0=B0=D0=BD=D0=B0?= =?UTF-8?q?=D0=BB=D0=B8=D0=B7=D0=B0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../diagnostics/RefOveruseDiagnostic.java | 18 +++++++++-- .../diagnostics/RefOveruseDiagnosticTest.java | 5 ++- .../diagnostics/RefOveruseDiagnostic.bsl | 32 +++++++++++++++++++ 3 files changed, 52 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnostic.java b/src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnostic.java index 4617c0d4147..5ab5375ca34 100644 --- a/src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnostic.java +++ b/src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnostic.java @@ -35,6 +35,7 @@ import java.util.Collection; import java.util.Collections; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -79,12 +80,25 @@ private Stream checkQuery(SDBLParser.QueryContext ctx) { } private static Map dataSourcesWithTabularSection(SDBLParser.QueryContext ctx) { - return Trees.findAllRuleNodes(ctx, SDBLParser.RULE_dataSource).stream() - .map(SDBLParser.DataSourceContext.class::cast) + return findAllDataSourceWithoutInnerQueries(ctx) .collect(Collectors.toMap(RefOveruseDiagnostic::getTableNameOrAlias, RefOveruseDiagnostic::isTableWithTabularSection)); } + private static Stream findAllDataSourceWithoutInnerQueries( + SDBLParser.QueryContext ctx) { + if (ctx.from == null){ + return Stream.empty(); + } + return Stream.concat( + ctx.from.dataSource().stream(), + ctx.from.dataSource().stream() + .flatMap(dataSourceContext -> dataSourceContext.joinPart().stream()) + .map(SDBLParser.JoinPartContext::dataSource) + .filter(Objects::nonNull) + ); + } + private static String getTableNameOrAlias(SDBLParser.DataSourceContext dataSource) { final var value = Optional.of(dataSource); return value diff --git a/src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnosticTest.java b/src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnosticTest.java index 04d28823289..86dc114c122 100644 --- a/src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnosticTest.java +++ b/src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnosticTest.java @@ -51,6 +51,9 @@ void test() { .hasRange(153, 13, 153, 41) .hasRange(164, 13, 164, 53) .hasRange(178, 13, 178, 35) - .hasSize(12); + .hasRange(216, 13, 37) + .hasRange(226, 13, 37) + .hasRange(238, 13, 38) + .hasSize(15); } } diff --git a/src/test/resources/diagnostics/RefOveruseDiagnostic.bsl b/src/test/resources/diagnostics/RefOveruseDiagnostic.bsl index bc968112e77..8e7833eed52 100644 --- a/src/test/resources/diagnostics/RefOveruseDiagnostic.bsl +++ b/src/test/resources/diagnostics/RefOveruseDiagnostic.bsl @@ -206,3 +206,35 @@ |ГДЕ | Таблица.Ссылка.Реквизит1 = &ИНН"; // не ошибка КонецПроцедуры + +Процедура Тест19() + ТекстЗапроса = + "ВЫБРАТЬ + | Таблица.Ссылка КАК Ссылка + |ИЗ + | (ВЫБРАТЬ Таблица.Ссылка ИЗ Таблица КАК Таблица) КАК Таблица // было падение анализа + |ГДЕ + | Таблица.Ссылка.Реквизит1 = &ИНН"; // ошибка +КонецПроцедуры + +Процедура Тест20() + ТекстЗапроса = + "ВЫБРАТЬ + | Таблица.Ссылка КАК Ссылка + |ИЗ + | (ВЫБРАТЬ Таблица.Ссылка ИЗ Таблица) КАК Таблица // было падение анализа + |ГДЕ + | Таблица.Ссылка.Реквизит1 = &ИНН"; // ошибка +КонецПроцедуры + +Процедура Тест21() + ТекстЗапроса = + "ВЫБРАТЬ + | Таблица.Ссылка КАК Ссылка + |ИЗ + | Таблица КАК Таблица + | ЛЕВОЕ СОЕДИНЕНИЕ (ВЫБРАТЬ Таблица.Ссылка ИЗ Таблица) // было падение анализа + | КАК Таблица2 ПО Истина + |ГДЕ + | Таблица2.Ссылка.Реквизит1 = &ИНН"; // ошибка +КонецПроцедуры