From d53bda3208d0ba709b3060af59f468aa7aae1d7f Mon Sep 17 00:00:00 2001 From: Lars Reimann Date: Fri, 6 Oct 2023 16:35:22 +0200 Subject: [PATCH] feat: port remaining validation infos that don't need partial evaluation (#607) Closes partially #543. ### Summary of Changes Show an info if * an argument list can be removed, * a type argument list can be removed, * a safe access can be removed. --------- Co-authored-by: megalinter-bot <129584137+megalinter-bot@users.noreply.github.com> --- src/language/helpers/nodeProperties.ts | 4 + src/language/validation/safe-ds-validator.ts | 8 ++ src/language/validation/style.ts | 109 +++++++++++++++++- .../main.sdstest | 35 ++++++ .../main.sdstest | 65 +++++++++++ .../unnecessary safe access/main.sdstest | 10 ++ .../main.sdstest | 49 ++++++++ 7 files changed, 277 insertions(+), 3 deletions(-) create mode 100644 tests/resources/validation/style/unnecessary argument list in annotation call/main.sdstest create mode 100644 tests/resources/validation/style/unnecessary argument list in call/main.sdstest create mode 100644 tests/resources/validation/style/unnecessary safe access/main.sdstest create mode 100644 tests/resources/validation/style/unnecessary type argument list/main.sdstest diff --git a/src/language/helpers/nodeProperties.ts b/src/language/helpers/nodeProperties.ts index 3490fa941..9770f2ca7 100644 --- a/src/language/helpers/nodeProperties.ts +++ b/src/language/helpers/nodeProperties.ts @@ -63,6 +63,10 @@ export const isNamedTypeArgument = (node: SdsTypeArgument): boolean => { return Boolean(node.typeParameter); }; +export const isRequiredParameter = (node: SdsParameter): boolean => { + return !node.defaultValue && !node.isVariadic; +}; + export const isStatic = (node: SdsClassMember): boolean => { if (isSdsClass(node) || isSdsEnum(node)) { return true; diff --git a/src/language/validation/safe-ds-validator.ts b/src/language/validation/safe-ds-validator.ts index 9b115dc2c..05420011d 100644 --- a/src/language/validation/safe-ds-validator.ts +++ b/src/language/validation/safe-ds-validator.ts @@ -16,13 +16,17 @@ import { segmentMustContainUniqueNames, } from './names.js'; import { + annotationCallArgumentListShouldBeNeeded, annotationParameterListShouldNotBeEmpty, assignmentShouldHaveMoreThanWildcardsAsAssignees, + callArgumentListShouldBeNeeded, classBodyShouldNotBeEmpty, constraintListShouldNotBeEmpty, enumBodyShouldNotBeEmpty, enumVariantParameterListShouldNotBeEmpty, functionResultListShouldNotBeEmpty, + memberAccessNullSafetyShouldBeNeeded, + namedTypeTypeArgumentListShouldBeNeeded, segmentResultListShouldNotBeEmpty, typeParameterListShouldNotBeEmpty, unionTypeShouldNotHaveASingularTypeArgument, @@ -52,9 +56,11 @@ export const registerValidationChecks = function (services: SafeDsServices) { const checks: ValidationChecks = { SdsAssignment: [assignmentShouldHaveMoreThanWildcardsAsAssignees], SdsAnnotation: [annotationMustContainUniqueNames, annotationParameterListShouldNotBeEmpty], + SdsAnnotationCall: [annotationCallArgumentListShouldBeNeeded], SdsArgumentList: [argumentListMustNotHavePositionalArgumentsAfterNamedArguments], SdsAttribute: [attributeMustHaveTypeHint], SdsBlockLambda: [blockLambdaMustContainUniqueNames], + SdsCall: [callArgumentListShouldBeNeeded(services)], SdsCallableType: [callableTypeMustContainUniqueNames, callableTypeMustNotHaveOptionalParameters], SdsClass: [classMustContainUniqueNames], SdsClassBody: [classBodyShouldNotBeEmpty], @@ -65,7 +71,9 @@ export const registerValidationChecks = function (services: SafeDsServices) { SdsEnumVariant: [enumVariantMustContainUniqueNames, enumVariantParameterListShouldNotBeEmpty], SdsExpressionLambda: [expressionLambdaMustContainUniqueNames], SdsFunction: [functionMustContainUniqueNames, functionResultListShouldNotBeEmpty], + SdsMemberAccess: [memberAccessNullSafetyShouldBeNeeded(services)], SdsModule: [moduleDeclarationsMustMatchFileKind, moduleWithDeclarationsMustStatePackage], + SdsNamedType: [namedTypeTypeArgumentListShouldBeNeeded], SdsParameter: [parameterMustHaveTypeHint, parameterMustNotBeVariadicAndOptional], SdsParameterList: [ parameterListMustNotHaveOptionalAndVariadicParameters, diff --git a/src/language/validation/style.ts b/src/language/validation/style.ts index 2df07a734..1e0b5c414 100644 --- a/src/language/validation/style.ts +++ b/src/language/validation/style.ts @@ -1,33 +1,88 @@ import { + isSdsEnumVariant, isSdsWildcard, SdsAnnotation, + SdsAnnotationCall, SdsAssignment, + SdsCall, SdsClassBody, SdsConstraintList, SdsEnumBody, SdsEnumVariant, SdsFunction, + SdsMemberAccess, + SdsNamedType, SdsSegment, SdsTypeParameterList, SdsUnionType, } from '../generated/ast.js'; import { ValidationAcceptor } from 'langium'; import { isEmpty } from 'radash'; +import { isRequiredParameter, parametersOrEmpty, typeParametersOrEmpty } from '../helpers/nodeProperties.js'; +import { SafeDsServices } from '../safe-ds-module.js'; +import { UnknownType } from '../typing/model.js'; export const CODE_STYLE_UNNECESSARY_ASSIGNMENT = 'style/unnecessary-assignment'; export const CODE_STYLE_UNNECESSARY_ARGUMENT_LIST = 'style/unnecessary-argument-list'; export const CODE_STYLE_UNNECESSARY_BODY = 'style/unnecessary-body'; export const CODE_STYLE_UNNECESSARY_CONSTRAINT_LIST = 'style/unnecessary-constraint-list'; export const CODE_STYLE_UNNECESSARY_ELVIS_OPERATOR = 'style/unnecessary-elvis-operator'; -export const CODE_STYLE_UNNECESSARY_SAFE_ACCESS = 'style/unnecessary-safe-access'; export const CODE_STYLE_UNNECESSARY_PARAMETER_LIST = 'style/unnecessary-parameter-list'; export const CODE_STYLE_UNNECESSARY_RESULT_LIST = 'style/unnecessary-result-list'; +export const CODE_STYLE_UNNECESSARY_SAFE_ACCESS = 'style/unnecessary-safe-access'; export const CODE_STYLE_UNNECESSARY_TYPE_ARGUMENT_LIST = 'style/unnecessary-type-argument-list'; export const CODE_STYLE_UNNECESSARY_TYPE_PARAMETER_LIST = 'style/unnecessary-type-parameter-list'; export const CODE_STYLE_UNNECESSARY_UNION_TYPE = 'style/unnecessary-union-type'; // ----------------------------------------------------------------------------- -// Unnecessary assignment +// Unnecessary argument lists +// ----------------------------------------------------------------------------- + +export const annotationCallArgumentListShouldBeNeeded = (node: SdsAnnotationCall, accept: ValidationAcceptor): void => { + const argumentList = node.argumentList; + if (!argumentList || !isEmpty(argumentList.arguments)) { + // If there are arguments, they are either needed or erroneous (i.e. we already show an error) + return; + } + + const annotation = node.annotation?.ref; + if (!annotation) { + return; + } + + const hasRequiredParameters = parametersOrEmpty(annotation).some(isRequiredParameter); + if (!hasRequiredParameters) { + accept('info', 'This argument list can be removed.', { + node: argumentList, + code: CODE_STYLE_UNNECESSARY_ARGUMENT_LIST, + }); + } +}; + +export const callArgumentListShouldBeNeeded = + (services: SafeDsServices) => + (node: SdsCall, accept: ValidationAcceptor): void => { + const argumentList = node.argumentList; + if (!argumentList || !isEmpty(argumentList.arguments)) { + // If there are arguments, they are either needed or erroneous (i.e. we already show an error) + return; + } + + const callable = services.helpers.NodeMapper.callToCallableOrUndefined(node); + if (!isSdsEnumVariant(callable)) { + return; + } + + if (isEmpty(parametersOrEmpty(callable))) { + accept('info', 'This argument list can be removed.', { + node: argumentList, + code: CODE_STYLE_UNNECESSARY_ARGUMENT_LIST, + }); + } + }; + +// ----------------------------------------------------------------------------- +// Unnecessary assignments // ----------------------------------------------------------------------------- export const assignmentShouldHaveMoreThanWildcardsAsAssignees = ( @@ -66,7 +121,7 @@ export const enumBodyShouldNotBeEmpty = (node: SdsEnumBody, accept: ValidationAc }; // ----------------------------------------------------------------------------- -// Unnecessary constraint list +// Unnecessary constraint lists // ----------------------------------------------------------------------------- export const constraintListShouldNotBeEmpty = (node: SdsConstraintList, accept: ValidationAcceptor) => { @@ -126,6 +181,54 @@ export const segmentResultListShouldNotBeEmpty = (node: SdsSegment, accept: Vali } }; +// ----------------------------------------------------------------------------- +// Unnecessary safe access +// ----------------------------------------------------------------------------- + +export const memberAccessNullSafetyShouldBeNeeded = + (services: SafeDsServices) => + (node: SdsMemberAccess, accept: ValidationAcceptor): void => { + if (!node.isNullSafe) { + return; + } + + const receiverType = services.types.TypeComputer.computeType(node.receiver); + if (receiverType === UnknownType) { + return; + } + + if (!receiverType.isNullable) { + accept('info', 'The receiver is never null, so the safe access is unnecessary.', { + node, + code: CODE_STYLE_UNNECESSARY_SAFE_ACCESS, + }); + } + }; + +// ----------------------------------------------------------------------------- +// Unnecessary type argument lists +// ----------------------------------------------------------------------------- + +export const namedTypeTypeArgumentListShouldBeNeeded = (node: SdsNamedType, accept: ValidationAcceptor): void => { + const typeArgumentList = node.typeArgumentList; + if (!typeArgumentList || !isEmpty(typeArgumentList.typeArguments)) { + // If there are type arguments, they are either needed or erroneous (i.e. we already show an error) + return; + } + + const namedTypeDeclaration = node.declaration?.ref; + if (!namedTypeDeclaration) { + return; + } + + if (isEmpty(typeParametersOrEmpty(namedTypeDeclaration))) { + accept('info', 'This type argument list can be removed.', { + node: typeArgumentList, + code: CODE_STYLE_UNNECESSARY_ARGUMENT_LIST, + }); + } +}; + // ----------------------------------------------------------------------------- // Unnecessary type parameter lists // ----------------------------------------------------------------------------- diff --git a/tests/resources/validation/style/unnecessary argument list in annotation call/main.sdstest b/tests/resources/validation/style/unnecessary argument list in annotation call/main.sdstest new file mode 100644 index 000000000..8ec279c73 --- /dev/null +++ b/tests/resources/validation/style/unnecessary argument list in annotation call/main.sdstest @@ -0,0 +1,35 @@ +package tests.validation.style.unnecessaryArgumentListInAnnotationCall + +@Repeatable +annotation AnnotationWithoutParameterList + +@Repeatable +annotation AnnotationWithEmptyParameterList() + +@Repeatable +annotation AnnotationWithoutRequiredParameters(a: Int = 0, vararg b: Int) + +@Repeatable +annotation AnnotationWithRequiredParameters(a: Int) + +// $TEST$ info "This argument list can be removed." +@AnnotationWithoutParameterList»()« +// $TEST$ no info "This argument list can be removed." +@AnnotationWithoutParameterList»(1)« +// $TEST$ info "This argument list can be removed." +@AnnotationWithEmptyParameterList»()« +// $TEST$ no info "This argument list can be removed." +@AnnotationWithEmptyParameterList»(1)« +// $TEST$ info "This argument list can be removed." +@AnnotationWithoutRequiredParameters»()« +// $TEST$ no info "This argument list can be removed." +@AnnotationWithoutRequiredParameters»(1)« +// $TEST$ no info "This argument list can be removed." +@AnnotationWithRequiredParameters»()« +// $TEST$ no info "This argument list can be removed." +@AnnotationWithRequiredParameters»(1)« +// $TEST$ no info "This argument list can be removed." +@Unresolved»()« +// $TEST$ no info "This argument list can be removed." +@Unresolved»(1)« +class MyClass diff --git a/tests/resources/validation/style/unnecessary argument list in call/main.sdstest b/tests/resources/validation/style/unnecessary argument list in call/main.sdstest new file mode 100644 index 000000000..3c3eb3615 --- /dev/null +++ b/tests/resources/validation/style/unnecessary argument list in call/main.sdstest @@ -0,0 +1,65 @@ +package tests.validation.style.unnecessaryArgumentListInCall + +annotation MyAnnotation + +class MyClass() + +enum MyEnum { + EnumVariantWithoutParameterList + EnumVariantWithEmptyParameterList() + EnumVariantWithoutRequiredParameters(a: Int = 0, vararg b: Int) + EnumVariantWithRequiredParameters(a: Int) +} + +fun myFunction() + +segment mySegment1() {} + +segment mySegment2( + callableType: () -> (), +) { + val blockLambda = () {}; + val expressionLambda = () -> 0; + + // $TEST$ no info "This argument list can be removed." + MyAnnotation»()«; + + // $TEST$ no info "This argument list can be removed." + MyClass»()«; + + // $TEST$ info "This argument list can be removed." + MyEnum.EnumVariantWithoutParameterList»()«; + // $TEST$ no info "This argument list can be removed." + MyEnum.EnumVariantWithoutParameterList»(1)«; + // $TEST$ info "This argument list can be removed." + MyEnum.EnumVariantWithEmptyParameterList»()«; + // $TEST$ no info "This argument list can be removed." + MyEnum.EnumVariantWithEmptyParameterList»(1)«; + // $TEST$ no info "This argument list can be removed." + MyEnum.EnumVariantWithoutRequiredParameters»()«; + // $TEST$ no info "This argument list can be removed." + MyEnum.EnumVariantWithoutRequiredParameters»(1)«; + // $TEST$ no info "This argument list can be removed." + MyEnum.EnumVariantWithRequiredParameters»()«; + // $TEST$ no info "This argument list can be removed." + MyEnum.EnumVariantWithRequiredParameters»(1)«; + // $TEST$ no info "This argument list can be removed." + MyEnum.Unresolved»()«; + // $TEST$ no info "This argument list can be removed." + MyEnum.Unresolved»(1)«; + + // $TEST$ no info "This argument list can be removed." + myFunction»()«; + + // $TEST$ no info "This argument list can be removed." + mySegment1»()«; + + // $TEST$ no info "This argument list can be removed." + callableType»()«; + + // $TEST$ no info "This argument list can be removed." + blockLambda»()«; + + // $TEST$ no info "This argument list can be removed." + expressionLambda»()«; +} diff --git a/tests/resources/validation/style/unnecessary safe access/main.sdstest b/tests/resources/validation/style/unnecessary safe access/main.sdstest new file mode 100644 index 000000000..c1ad2d81c --- /dev/null +++ b/tests/resources/validation/style/unnecessary safe access/main.sdstest @@ -0,0 +1,10 @@ +package tests.validation.style.unnecessarySafeAccess + +pipeline test { + // $TEST$ info "The receiver is never null, so the safe access is unnecessary." + »1?.toString«(); + // $TEST$ no info "The receiver is never null, so the safe access is unnecessary." + »null?.toString«(); + // $TEST$ no info "The receiver is never null, so the safe access is unnecessary." + »unresolved?.toString«(); +} diff --git a/tests/resources/validation/style/unnecessary type argument list/main.sdstest b/tests/resources/validation/style/unnecessary type argument list/main.sdstest new file mode 100644 index 000000000..a2b6671b6 --- /dev/null +++ b/tests/resources/validation/style/unnecessary type argument list/main.sdstest @@ -0,0 +1,49 @@ +package tests.validation.style.unnecessaryTypeArgumentList + +class ClassWithoutTypeParameterList +class ClassWithEmptyTypeParameterList<> +class ClassWithTypeParameters + +enum Enum { + EnumVariantWithoutTypeParameterList + EnumVariantWithEmptyTypeParameterList<> + VariantWithTypeParameters +} + +fun myFunction( + // $TEST$ info "This type argument list can be removed." + a1: ClassWithoutTypeParameterList»<>«, + // $TEST$ no info "This type argument list can be removed." + a2: ClassWithoutTypeParameterList»«, + // $TEST$ info "This type argument list can be removed." + a3: ClassWithEmptyTypeParameterList»<>«, + // $TEST$ no info "This type argument list can be removed." + a4: ClassWithEmptyTypeParameterList»«, + // $TEST$ no info "This type argument list can be removed." + a5: ClassWithTypeParameters»<>«, + // $TEST$ no info "This type argument list can be removed." + a6: ClassWithTypeParameters»«, + + // $TEST$ info "This type argument list can be removed." + b1: Enum.EnumVariantWithoutTypeParameterList»<>«, + // $TEST$ no info "This type argument list can be removed." + b2: Enum.EnumVariantWithoutTypeParameterList»«, + // $TEST$ info "This type argument list can be removed." + b3: Enum.EnumVariantWithEmptyTypeParameterList»<>«, + // $TEST$ no info "This type argument list can be removed." + b4: Enum.EnumVariantWithEmptyTypeParameterList»«, + // $TEST$ no info "This type argument list can be removed." + b5: Enum.VariantWithTypeParameters»<>«, + // $TEST$ no info "This type argument list can be removed." + b6: Enum.VariantWithTypeParameters»«, + + // $TEST$ info "This type argument list can be removed." + c1: Enum»<>«, + // $TEST$ no info "This type argument list can be removed." + c2: Enum»«, + + // $TEST$ no info "This type argument list can be removed." + d1: Unresolved»<>«, + // $TEST$ no info "This type argument list can be removed." + d2: Unresolved»«, +)