Skip to content

Все параметры запроса инициализированы #1882

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 9 commits into
base: develop
Choose a base branch
from
36 changes: 36 additions & 0 deletions docs/en/diagnostics/MissingQueryParameter.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# <Diagnostic name> (MissingQueryParameter)

<Metadata>

## <Params>

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

## Examples
<!-- В данном разделе приводятся примеры, на которые диагностика срабатывает, а также можно привести пример, как можно исправить ситуацию -->

## Sources
<!-- Необходимо указывать ссылки на все источники, из которых почерпнута информация для создания диагностики -->
<!-- Примеры источников

* Источник: [Стандарт: Тексты модулей](https://its.1c.ru/db/v8std#content:456:hdoc)
* Полезная информация: [Отказ от использования модальных окон](https://its.1c.ru/db/metod8dev#content:5272:hdoc)
* Источник: [Cognitive complexity, ver. 1.4](https://www.sonarsource.com/docs/CognitiveComplexity.pdf) -->

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

### Diagnostic ignorance in code

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

### Parameter for config

```json
"MissingQueryParameter": <DiagnosticConfig>
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,331 @@
/*
* This file is a part of BSL Language Server.
*
* Copyright (c) 2018-2021
* Alexey Sosnoviy <labotamy@gmail.com>, Nikita Gryzlov <nixel2007@gmail.com> and contributors
*
* SPDX-License-Identifier: LGPL-3.0-or-later
*
* BSL Language Server is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3.0 of the License, or (at your option) any later version.
*
* BSL Language Server is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with BSL Language Server.
*/
package com.github._1c_syntax.bsl.languageserver.diagnostics;

import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticMetadata;
import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticSeverity;
import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticTag;
import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticType;
import com.github._1c_syntax.bsl.languageserver.utils.Ranges;
import com.github._1c_syntax.bsl.languageserver.utils.Trees;
import com.github._1c_syntax.bsl.parser.BSLParser;
import com.github._1c_syntax.bsl.parser.BSLParser.AssignmentContext;
import com.github._1c_syntax.bsl.parser.BSLParser.CodeBlockContext;
import com.github._1c_syntax.bsl.parser.BSLParserRuleContext;
import com.github._1c_syntax.bsl.parser.SDBLParser;
import com.github._1c_syntax.bsl.parser.SDBLParser.ParameterContext;
import com.github._1c_syntax.bsl.parser.SDBLParser.QueryPackageContext;
import com.github._1c_syntax.bsl.parser.Tokenizer;
import com.github._1c_syntax.utils.CaseInsensitivePattern;
import lombok.AllArgsConstructor;
import lombok.Getter;
import org.antlr.v4.runtime.tree.ParseTree;
import org.apache.commons.lang3.tuple.Pair;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

@DiagnosticMetadata(
type = DiagnosticType.CODE_SMELL,
severity = DiagnosticSeverity.INFO,
minutesToFix = 1,
tags = {
DiagnosticTag.SUSPICIOUS
}

)
public class MissingQueryParameterDiagnostic extends AbstractVisitorDiagnostic {
Copy link
Member

Choose a reason for hiding this comment

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

@artbear на текущий момент диагностика будет сильно фонить. Причины:

  • Нет извлечения текста запроса из результата выполнения локальной функции
  • Нет очистки (замены) значения экземпляра запроса при новом объявлении
  • Не обработан метод ЗаполнитьЗначенияСвойств(...)
  • Не обработан метод СтрЗаменить(..)
  • Инициализация и установка параметрах могут быть в разных "ветках" кода
  • Не обработано заполнение параметров в другой функции, хотя бы в локальной


private static final Pattern QUERY_PATTERN = CaseInsensitivePattern.compile(
"Запрос|Query");
private static final Pattern QUERY_TEXT_PATTERN = CaseInsensitivePattern.compile(
"Текст|Text");
private static final Pattern SET_PARAMETER_PATTERN = CaseInsensitivePattern.compile(
"УстановитьПараметр|SetParameter");
public static final int SET_PARAMETER_PARAMS_COUNT = 2;

private Collection<CodeBlockContext> codeBlocks = Collections.emptyList();
private final Map<CodeBlockContext, List<QueryTextSetupData>> codeBlockAssignments = new HashMap<>();
private final Map<CodeBlockContext, List<BSLParser.CallStatementContext>> codeBlockCallStatements = new HashMap<>();

enum QueryVarKind {
NEW_QUERY,
QUERY_TEXT,
EMPTY,
VAR
}

@AllArgsConstructor
@Getter
private static class QueryTextSetupData {
private QueryVarKind kind;
private String varName;
private BSLParser.ExpressionContext rightExpr;
private Optional<BSLParser.CallParamListContext> newQueryExpr;

boolean isQueryObject(){
return kind == QueryVarKind.NEW_QUERY || kind == QueryVarKind.QUERY_TEXT;
}
boolean isVar(){
return kind == QueryVarKind.VAR;
}
}

@Override
public ParseTree visitFile(BSLParser.FileContext file) {

final var queriesWithParams = documentContext.getQueries().stream()
Copy link
Member

Choose a reason for hiding this comment

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

@artbear если это вынести в отдельный класс, который будет описывать экземпляр Запрос, то диагностика сразу станет легче.

.map(Tokenizer::getAst)
.map(ctx -> Pair.of(ctx, getParams(ctx)))
.collect(Collectors.toList());

if (!queriesWithParams.isEmpty()) {
codeBlocks = getCodeBlocks();

queriesWithParams.forEach(query -> visitQuery(query.getLeft(), query.getRight()));

codeBlocks.clear();
codeBlockAssignments.clear();
codeBlockCallStatements.clear();

}

return super.visitFile(file);
Copy link
Member

Choose a reason for hiding this comment

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

@artbear нет смысла спускаться ниже. Просто return file;

}

private static Map<String, ParameterContext> getParams(QueryPackageContext queryPackage) {
return Trees.findAllRuleNodes(queryPackage, SDBLParser.RULE_parameter).stream()
.filter(ParameterContext.class::isInstance)
.map(ParameterContext.class::cast)
// .map(ctx -> Pair.of(ctx, "\"" + ctx.name.getText() + "\""))
// если есть несколько одинаковых параметров в запросе
.collect(Collectors.toMap(ctx -> "\"" + ctx.name.getText() + "\"", ctx -> ctx,
Copy link
Member

Choose a reason for hiding this comment

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

@artbear String.format

(parameterContext, parameterContext2) -> parameterContext));
// .collect(Collectors.toSet());
}

private Collection<CodeBlockContext> getCodeBlocks() {
final var ast = documentContext.getAst();
var blocks = getSubBlocks(ast);
final var fileCodeBlock = Optional.ofNullable(ast.fileCodeBlock()).map(BSLParser.FileCodeBlockContext::codeBlock);
Copy link
Member

Choose a reason for hiding this comment

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

@artbear зачем разделение на 2 операции? Почему сразу не написать:

Optional.ofNullable(ast.fileCodeBlock())
    .map(BSLParser.FileCodeBlockContext::codeBlock)
    .ifPresent(blocks::add);

fileCodeBlock.ifPresent(blocks::add);
final var fileCodeBlockBeforeSub =
Copy link
Member

Choose a reason for hiding this comment

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

@artbear зачем разделение на 2 операции?

Optional.ofNullable(ast.fileCodeBlockBeforeSub())
.map(BSLParser.FileCodeBlockBeforeSubContext::codeBlock);
fileCodeBlockBeforeSub.ifPresent(blocks::add);
return blocks;
}

private static Collection<CodeBlockContext> getSubBlocks(BSLParser.FileContext ast) {
if (ast.subs() == null) {
return new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

@artbear элементы блока дальше не изменяются, может тогда Collections.emptyList(); ?

}
return ast.subs().sub().stream().map((BSLParser.SubContext sub) -> {
if (sub.procedure() == null) {
return sub.function().subCodeBlock().codeBlock();
} else {
return sub.procedure().subCodeBlock().codeBlock();
}
}).collect(Collectors.toList());
}

private void visitQuery(QueryPackageContext queryPackage, Map<String, ParameterContext> params) {
final var codeBlockByQuery = getCodeBlockByQuery(queryPackage);
codeBlockByQuery.ifPresent(codeBlock -> getQueryTextAssignmentsInsideBlock(codeBlock, queryPackage)
.forEach(queryTextAssignment -> checkAssignment(codeBlock, params, queryTextAssignment)));
}

private Optional<CodeBlockContext> getCodeBlockByQuery(QueryPackageContext key) {
Copy link
Member

Choose a reason for hiding this comment

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

@artbear key?

return codeBlocks.stream()
.filter(block -> Ranges.containsRange(Ranges.create(block), Ranges.create(key)))
.findFirst();
}

private List<QueryTextSetupData> getQueryTextAssignmentsInsideBlock(CodeBlockContext codeBlock,
QueryPackageContext queryPackage) {

final var queryAssignments = codeBlockAssignments.computeIfAbsent(codeBlock,
MissingQueryParameterDiagnostic::getAllQueryAssignmentInsideBlock);

final var queryObjectTextAssignments = queryAssignments.stream()
.filter(QueryTextSetupData::isQueryObject)
.collect(Collectors.toList());

final var queryTextAssignments = queryAssignments.stream()
.filter(QueryTextSetupData::isVar)
.flatMap(queryTextSetupData -> getQueryTextAssignment(queryObjectTextAssignments,
queryTextSetupData.getRightExpr(), queryTextSetupData.getVarName())
.stream())
.collect(Collectors.toList());

final var queryRange = Ranges.create(queryPackage);
queryTextAssignments.addAll(queryObjectTextAssignments.stream()
.filter(queryTextSetupData -> Ranges.containsRange(Ranges.create(queryTextSetupData.getRightExpr()), queryRange))
.collect(Collectors.toList())
);

return queryTextAssignments;
}

private static List<QueryTextSetupData> getAllQueryAssignmentInsideBlock(CodeBlockContext codeBlock) {
return Trees.findAllRuleNodes(codeBlock, BSLParser.RULE_assignment).stream()
Copy link
Member

Choose a reason for hiding this comment

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

@artbear используй cfg и подписку на assignment

.filter(AssignmentContext.class::isInstance)
.map(AssignmentContext.class::cast)
.map(MissingQueryParameterDiagnostic::computeQueryVarData)
.filter(queryTextSetupData -> queryTextSetupData.getKind() != QueryVarKind.EMPTY)
Copy link
Member

Choose a reason for hiding this comment

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

@artbear можно воспользоваться Predicate.not(..)

.collect(Collectors.toList());
}

private static QueryTextSetupData computeQueryVarData(AssignmentContext assignment) {
final var newQueryExpr = isNewQueryExpr(assignment);
Copy link
Member

Choose a reason for hiding this comment

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

@artbear Expr не слишком читаемое сокращение выражения. Может просто newQuery?

Copy link
Member

Choose a reason for hiding this comment

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

Ну и снова разбивка на 2 операции

if (newQueryExpr.isPresent()) {
return new QueryTextSetupData(QueryVarKind.NEW_QUERY, assignment.lValue().getText(), assignment.expression(),
newQueryExpr);
}
final var pair = computeQueryVarNameFromLValue(assignment.lValue());
return new QueryTextSetupData(pair.getRight(), pair.getLeft(), assignment.expression(), Optional.empty());
Copy link
Member

Choose a reason for hiding this comment

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

@artbear у assignment точно будет всегда expression при наборе в том же vscode?

}

private static Optional<BSLParser.CallParamListContext> isNewQueryExpr(AssignmentContext assignment) {
return Trees.findAllRuleNodes(assignment, BSLParser.RULE_newExpression).stream()
.filter(BSLParser.NewExpressionContext.class::isInstance)
.map(BSLParser.NewExpressionContext.class::cast)
.filter(ctx -> ctx.typeName() != null)
.filter(ctx -> QUERY_PATTERN.matcher(ctx.typeName().getText()).matches())
.map(BSLParser.NewExpressionContext::doCall)
.filter(Objects::nonNull)
Copy link
Member

Choose a reason for hiding this comment

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

@artbear кажется здесь это излишне

Copy link
Contributor Author

Choose a reason for hiding this comment

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

без этого точно падает, выявил на бсп

.map(BSLParser.DoCallContext::callParamList)
.filter(Objects::nonNull)
Copy link
Member

Choose a reason for hiding this comment

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

@artbear кажется здесь это излишне

.findFirst();
}

private static Pair<String, QueryVarKind> computeQueryVarNameFromLValue(BSLParser.LValueContext lValue) {
final var identifier = lValue.IDENTIFIER();
final var acceptor = lValue.acceptor();
if (acceptor == null || identifier == null) {
return Pair.of(lValue.getText(), QueryVarKind.VAR);
}
return Optional.of(acceptor)
.map(BSLParser.AcceptorContext::accessProperty)
.map(BSLParser.AccessPropertyContext::IDENTIFIER)
.map(ParseTree::getText)
.filter(s -> QUERY_TEXT_PATTERN.matcher(s).matches())
Copy link
Member

Choose a reason for hiding this comment

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

@artbear s плохое имя для переменной

.map(s -> identifier.getText())
.map(s -> Pair.of(s, QueryVarKind.QUERY_TEXT))
.orElse(Pair.of("", QueryVarKind.EMPTY));
}

private Optional<QueryTextSetupData> getQueryTextAssignment(List<QueryTextSetupData> queryObjectTextAssignments,
BSLParser.ExpressionContext varAssign, String varName) {
return queryObjectTextAssignments.stream()
.filter(queryTextSetupData -> queryTextSetupData.getRightExpr().getStop().getLine() > varAssign.getStop().getLine())
.filter(queryTextSetupData -> queryTextSetupData.getNewQueryExpr()
.filter(callParamListContext -> callParamListContext.getText().equalsIgnoreCase(varName))
.isPresent())
.findFirst()
;
}

private void checkAssignment(CodeBlockContext codeBlock,
Map<String, ParameterContext> params,
QueryTextSetupData queryTextAssignment) {

final var callStatements = codeBlockCallStatements.computeIfAbsent(codeBlock,
MissingQueryParameterDiagnostic::getIsSetParameterCallStatements);

final var allParams = params.values();

if (!callStatements.isEmpty()) {

final var queryVarName = queryTextAssignment.getVarName();
final var usedParams = callStatements.stream()
.map(callStatementContext -> findAppropriateParamFromSetParameter(callStatementContext, queryVarName, params))
.flatMap(Optional::stream)
.collect(Collectors.toList());
allParams.removeAll(usedParams);
}

allParams.forEach(node -> diagnosticStorage.addDiagnostic(node,
info.getMessage(node.PARAMETER_IDENTIFIER().getText())));
}

private static List<BSLParser.CallStatementContext> getIsSetParameterCallStatements(CodeBlockContext codeBlock) {
return Trees.findAllRuleNodes(codeBlock, BSLParser.RULE_callStatement).stream()
.filter(BSLParser.CallStatementContext.class::isInstance)
.map(BSLParser.CallStatementContext.class::cast)
.filter(MissingQueryParameterDiagnostic::isSetParameterCall)
.collect(Collectors.toList());
}

private static boolean isSetParameterCall(BSLParser.CallStatementContext callStatement) {
return Optional.of(callStatement)
.map(BSLParser.CallStatementContext::accessCall)
.map(BSLParser.AccessCallContext::methodCall)
.map(BSLParser.MethodCallContext::methodName)
.filter(ctx -> SET_PARAMETER_PATTERN.matcher(ctx.getText()).matches())
.isPresent();
}

private static Optional<ParameterContext> findAppropriateParamFromSetParameter(BSLParser.CallStatementContext callStatementContext,
String queryVarName,
Map<String, ParameterContext> params) {
final var callCtx = Optional.of(callStatementContext);
Copy link
Member

Choose a reason for hiding this comment

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

@artbear Ctx может не стоит использовать сокращение?

return callCtx
.map(BSLParser.CallStatementContext::IDENTIFIER)
.map(ParseTree::getText)
.filter(queryVarName::equalsIgnoreCase)
.flatMap(dummy -> findAppropriateParamFromSetParameterMethod(callCtx, params));
}

private static Optional<ParameterContext> findAppropriateParamFromSetParameterMethod(Optional<BSLParser.CallStatementContext> callCtx,
Map<String, ParameterContext> params) {
return callCtx.map(BSLParser.CallStatementContext::accessCall)
Copy link
Member

Choose a reason for hiding this comment

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

@artbear длинный стрим затрудняет чтение и осознание алгоритма

.map(BSLParser.AccessCallContext::methodCall)
.map(BSLParser.MethodCallContext::doCall)
.map(BSLParser.DoCallContext::callParamList)
.map(BSLParser.CallParamListContext::callParam)
.filter(callParamContexts -> callParamContexts.size() == SET_PARAMETER_PARAMS_COUNT)
.map(callParamContexts -> callParamContexts.get(0))
.map(BSLParser.CallParamContext::expression)
.map(BSLParser.ExpressionContext::member)
.filter(memberContexts -> !memberContexts.isEmpty())
.map(memberContexts -> memberContexts.get(0))
.map(BSLParser.MemberContext::constValue)
.map(BSLParserRuleContext::getText)
.flatMap(firstValueForSetParameterMethod -> findParameterByName(params, firstValueForSetParameterMethod));
}

private static Optional<ParameterContext> findParameterByName(Map<String, ParameterContext> params,
String firstValueForSetParameterMethod) {
return params.entrySet().stream()
.filter(entry -> entry.getKey().equalsIgnoreCase(firstValueForSetParameterMethod))
.map(Map.Entry::getValue).findFirst();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
diagnosticMessage=Добавьте установку параметра запроса "%s"
diagnosticName=Все параметры запроса инициализированы
Loading