Skip to content

Commit

Permalink
Merge pull request gradle#27184 Refactor Problems API
Browse files Browse the repository at this point in the history
This PR contains the following changes:

- Remove stepwise builder
- Remove ProblemBuilder.undocumented() and ProblemBuilder.noLocation()
- Allow Problems API to define namespace
- Separate private and public Problems API interface
- Make transformers internal
- Add ProblemBuilder.documentedAt(String)
- Make problem instances not reference the Problems service
- Make public API expose minimal set of features
- Replace ProblemBuilder with Action<ProblemSpec>
- ProblemBuilder extends ProblemSpec
- Require exception in ProbemReporter.throwing(Action)
- Move and cleanup ProblemLocation and its subclasses
- Clean up task path location and rename identityPath to buildTreePath
- Represent buildTreePath with String

## Reviewing cheatsheet

### Before merging the PR, comments starting with

    ❌ ❓must be fixed
    🤔 💅 should be fixed
    💭 may be fixed
    🎉 celebrate happy things

Co-authored-by: Donát Csikós <donat@gradle.com>
  • Loading branch information
bot-gradle and donat committed Dec 7, 2023
2 parents aeb974b + 2d1fec9 commit 3430dee
Show file tree
Hide file tree
Showing 127 changed files with 1,438 additions and 1,979 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ import org.gradle.api.internal.tasks.AbstractTaskDependencyResolveContext
import org.gradle.api.internal.tasks.properties.InspectionSchemeFactory
import org.gradle.api.problems.Problem
import org.gradle.api.problems.Problems
import org.gradle.api.problems.ReportableProblem
import org.gradle.api.problems.Severity
import org.gradle.api.problems.internal.InternalProblems
import org.gradle.api.services.BuildService
import org.gradle.api.services.ServiceReference
import org.gradle.api.tasks.Input
Expand Down Expand Up @@ -59,7 +59,7 @@ class FlowParametersInstantiator(
inspection.propertyWalker.visitProperties(
parameters,
object : ProblemRecordingTypeValidationContext(type, { Optional.empty() }) {
override fun recordProblem(problem: ReportableProblem) {
override fun recordProblem(problem: Problem) {
problems.add(problem)
}
},
Expand All @@ -75,11 +75,8 @@ class FlowParametersInstantiator(
object : AbstractTaskDependencyResolveContext() {
override fun add(dependency: Any) {
problems.add(
problemsService.create { builder ->
builder
.label("Property '$propertyName' cannot carry a dependency on $dependency as these are not yet supported.")
.undocumented()
.noLocation()
(problemsService as InternalProblems).internalReporter.create {
label("Property '$propertyName' cannot carry a dependency on $dependency as these are not yet supported.")
.category("validation", "property", "invalid-dependency")
.severity(Severity.ERROR)
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@
package org.gradle.internal.properties.annotations;

import org.gradle.api.problems.Severity;
import org.gradle.api.problems.internal.DefaultProblemCategory;
import org.gradle.internal.deprecation.Documentation;
import org.gradle.internal.reflect.validation.TypeValidationContext;

import java.lang.annotation.Annotation;
import java.util.Arrays;

import static java.util.stream.Collectors.joining;
import static org.gradle.api.problems.internal.DefaultProblemCategory.VALIDATION;

public abstract class AbstractTypeAnnotationHandler implements TypeAnnotationHandler {

Expand All @@ -49,8 +49,7 @@ protected static void reportInvalidUseOfTypeAnnotation(
problem.withAnnotationType(classWithAnnotationAttached)
.label("is incorrectly annotated with @" + annotationType.getSimpleName())
.documentedAt(Documentation.userManual("validation_problems", "invalid_use_of_cacheable_annotation"))
.noLocation()
.category(VALIDATION, "type", "invalid-use-of-type-annotation")
.category(DefaultProblemCategory.VALIDATION, "type", "invalid-use-of-type-annotation")
.severity(Severity.ERROR)
.details(String.format("This annotation only makes sense on %s types", Arrays.stream(appliesOnlyTo)
.map(Class::getSimpleName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.google.common.collect.Maps;
import com.google.common.reflect.TypeToken;
import org.gradle.api.internal.GeneratedSubclasses;
import org.gradle.api.problems.internal.DefaultProblemCategory;
import org.gradle.cache.internal.CrossBuildInMemoryCache;
import org.gradle.cache.internal.CrossBuildInMemoryCacheFactory;
import org.gradle.internal.reflect.annotations.AnnotationCategory;
Expand All @@ -41,7 +42,6 @@
import java.util.stream.Collector;
import java.util.stream.Collectors;

import static org.gradle.api.problems.internal.DefaultProblemCategory.VALIDATION;
import static org.gradle.api.problems.Severity.ERROR;
import static org.gradle.internal.deprecation.Documentation.userManual;
import static org.gradle.internal.reflect.annotations.AnnotationCategory.TYPE;
Expand Down Expand Up @@ -112,8 +112,7 @@ private <T> TypeMetadata createTypeMetadata(Class<T> type) {
.forProperty(propertyAnnotationMetadata.getPropertyName())
.label("is missing " + displayName)
.documentedAt(userManual("validation_problems", MISSING_ANNOTATION.toLowerCase()))
.noLocation()
.category(VALIDATION, "property", TextUtil.screamingSnakeToKebabCase(MISSING_ANNOTATION))
.category(DefaultProblemCategory.VALIDATION, "property", TextUtil.screamingSnakeToKebabCase(MISSING_ANNOTATION))
.severity(ERROR)
.details("A property without annotation isn't considered during up-to-date checking")
.solution("Add " + displayName)
Expand All @@ -129,8 +128,7 @@ private <T> TypeMetadata createTypeMetadata(Class<T> type) {
.forProperty(propertyAnnotationMetadata.getPropertyName())
.label("is annotated with invalid property type @%s", propertyType.getSimpleName())
.documentedAt(userManual("validation_problems", ANNOTATION_INVALID_IN_CONTEXT.toLowerCase()))
.noLocation()
.category(VALIDATION, "property", TextUtil.screamingSnakeToKebabCase(ANNOTATION_INVALID_IN_CONTEXT))
.category(DefaultProblemCategory.VALIDATION, "property", TextUtil.screamingSnakeToKebabCase(ANNOTATION_INVALID_IN_CONTEXT))
.severity(ERROR)
.details("The '@" + propertyType.getSimpleName() + "' annotation cannot be used in this context")
.solution("Remove the property")
Expand All @@ -152,8 +150,7 @@ private <T> TypeMetadata createTypeMetadata(Class<T> type) {
.forProperty(propertyAnnotationMetadata.getPropertyName())
.label("is annotated with @" + annotationType.getSimpleName() + " but that is not allowed for '" + propertyType.getSimpleName() + "' properties")
.documentedAt(userManual("validation_problems", INCOMPATIBLE_ANNOTATIONS.toLowerCase()))
.noLocation()
.category(VALIDATION, "property", TextUtil.screamingSnakeToKebabCase(INCOMPATIBLE_ANNOTATIONS))
.category(DefaultProblemCategory.VALIDATION, "property", TextUtil.screamingSnakeToKebabCase(INCOMPATIBLE_ANNOTATIONS))
.severity(ERROR)
.details("This modifier is used in conjunction with a property of type '" + propertyType.getSimpleName() + "' but this doesn't have semantics")
.solution("Remove the '@" + annotationType.getSimpleName() + "' annotation"));
Expand All @@ -163,8 +160,7 @@ private <T> TypeMetadata createTypeMetadata(Class<T> type) {
.forProperty(propertyAnnotationMetadata.getPropertyName())
.label("is annotated with invalid modifier @%s", annotationType.getSimpleName())
.documentedAt(userManual("validation_problems", ANNOTATION_INVALID_IN_CONTEXT.toLowerCase()))
.noLocation()
.category(VALIDATION, "property", TextUtil.screamingSnakeToKebabCase(ANNOTATION_INVALID_IN_CONTEXT))
.category(DefaultProblemCategory.VALIDATION, "property", TextUtil.screamingSnakeToKebabCase(ANNOTATION_INVALID_IN_CONTEXT))
.severity(ERROR)
.details("The '@" + annotationType.getSimpleName() + "' annotation cannot be used in this context")
.solution("Remove the annotation")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@

import com.google.common.collect.ImmutableSet;
import org.gradle.api.NonNullApi;
import org.gradle.api.problems.internal.DefaultProblemCategory;
import org.gradle.internal.reflect.validation.TypeValidationContext;

import java.util.Optional;
import java.util.stream.Collectors;

import static org.gradle.api.problems.internal.DefaultProblemCategory.VALIDATION;
import static org.gradle.api.problems.Severity.WARNING;
import static org.gradle.internal.deprecation.Documentation.userManual;

Expand Down Expand Up @@ -60,8 +60,7 @@ public static void validateBeanType(
.forProperty(propertyName)
.label("with nested type '" + beanType.getName() + "' is not supported")
.documentedAt(userManual("validation_problems", "unsupported_nested_type"))
.noLocation()
.category(VALIDATION, "property", "nested-type-unsupported")
.category(DefaultProblemCategory.VALIDATION, "property", "nested-type-unsupported")
.severity(WARNING)
.details(reason)
.solution("Use a different input annotation if type is not a bean")
Expand Down Expand Up @@ -102,8 +101,7 @@ public static void validateKeyType(
.forProperty(propertyName)
.label("where key of nested map is of type '" + keyType.getName() + "'")
.documentedAt(userManual("validation_problems", "unsupported_key_type_of_nested_map"))
.noLocation()
.category(VALIDATION, "property", "nested-map-unsupported-key-type")
.category(DefaultProblemCategory.VALIDATION, "property", "nested-map-unsupported-key-type")
.severity(WARNING)
.details("Key of nested map must be one of the following types: " + getSupportedKeyTypes())
.solution("Change type of key to one of the following types: " + getSupportedKeyTypes())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import org.gradle.api.InvalidUserDataException;
import org.gradle.api.problems.Problem;
import org.gradle.api.problems.ProblemCategory;
import org.gradle.api.problems.ReportableProblem;
import org.gradle.api.problems.internal.DefaultProblemCategory;
import org.gradle.internal.exceptions.DefaultMultiCauseException;
import org.gradle.internal.reflect.validation.TypeValidationProblemRenderer;
Expand All @@ -30,8 +29,6 @@
import java.util.Optional;

import static java.util.stream.Collectors.toList;
import static org.gradle.api.problems.internal.DefaultProblemCategory.SEPARATOR;
import static org.gradle.api.problems.internal.DefaultProblemCategory.VALIDATION;

public class DefaultTypeValidationContext extends ProblemRecordingTypeValidationContext {

Expand All @@ -52,15 +49,19 @@ private DefaultTypeValidationContext(@Nullable Class<?> rootType, boolean report
this.reportCacheabilityProblems = reportCacheabilityProblems;
}

public static final String MISSING_NORMALIZATION_CATEGORY_DETAILS = "property" + SEPARATOR + "missing-normalization-annotation";
public static final DefaultProblemCategory MISSING_NORMALIZATION_CATEGORY = new DefaultProblemCategory(VALIDATION + SEPARATOR + MISSING_NORMALIZATION_CATEGORY_DETAILS);
public static final String[] MISSING_NORMALIZATION_CATEGORY_DETAILS = new String[]
{"property", "missing-normalization-annotation"};
public static final DefaultProblemCategory MISSING_NORMALIZATION_CATEGORY = DefaultProblemCategory.create(DefaultProblemCategory.GRADLE_CORE_NAMESPACE, DefaultProblemCategory.VALIDATION, MISSING_NORMALIZATION_CATEGORY_DETAILS);

public static boolean onlyAffectsCacheableWork(ProblemCategory problemCategory) {
return MISSING_NORMALIZATION_CATEGORY.equals(problemCategory);
}




@Override
protected void recordProblem(ReportableProblem problem) {
protected void recordProblem(Problem problem) {
if (onlyAffectsCacheableWork(problem.getProblemCategory()) && !reportCacheabilityProblems) { // TODO (donat) is is already fixed on master
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
package org.gradle.internal.reflect;

import org.gradle.api.Action;
import org.gradle.api.problems.ReportableProblem;
import org.gradle.api.problems.Problem;
import org.gradle.api.problems.internal.DefaultProblemReporter;
import org.gradle.api.problems.internal.InternalProblemReporter;
import org.gradle.api.problems.internal.InternalProblems;
import org.gradle.api.problems.internal.ProblemsProgressEventEmitterHolder;
import org.gradle.internal.reflect.validation.DefaultTypeAwareProblemBuilder;
Expand Down Expand Up @@ -46,7 +48,8 @@ public ProblemRecordingTypeValidationContext(
@Override
public void visitTypeProblem(Action<? super TypeAwareProblemBuilder> problemSpec) {
InternalProblems problems = (InternalProblems) ProblemsProgressEventEmitterHolder.get();
DefaultTypeAwareProblemBuilder problemBuilder = new DefaultTypeAwareProblemBuilder(problems.createProblemBuilder());
InternalProblemReporter reporter = problems.getInternalReporter();
DefaultTypeAwareProblemBuilder problemBuilder = new DefaultTypeAwareProblemBuilder(((DefaultProblemReporter) reporter).createProblemBuilder());
problemSpec.execute(problemBuilder);
recordProblem(problemBuilder.build());
}
Expand All @@ -58,8 +61,9 @@ private Optional<PluginId> pluginId() {

@Override
public void visitPropertyProblem(Action<? super TypeAwareProblemBuilder> problemSpec) {
InternalProblems problems = (InternalProblems) ProblemsProgressEventEmitterHolder.get();
DefaultTypeAwareProblemBuilder problemBuilder = new DefaultTypeAwareProblemBuilder(problems.createProblemBuilder());
InternalProblems internalProblems = (InternalProblems) ProblemsProgressEventEmitterHolder.get();
InternalProblemReporter reporter = internalProblems.getInternalReporter();
DefaultTypeAwareProblemBuilder problemBuilder = new DefaultTypeAwareProblemBuilder(((DefaultProblemReporter) reporter).createProblemBuilder());
problemSpec.execute(problemBuilder);
problemBuilder.withAnnotationType(rootType);
pluginId()
Expand All @@ -68,5 +72,5 @@ public void visitPropertyProblem(Action<? super TypeAwareProblemBuilder> problem
recordProblem(problemBuilder.build());
}

abstract protected void recordProblem(ReportableProblem problem);
abstract protected void recordProblem(Problem problem);
}
Loading

0 comments on commit 3430dee

Please sign in to comment.