From b52a4b0b330c4f4903fb4aa41a63980774b43c85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krist=C3=B3f=20Marussy?= Date: Fri, 28 Jun 2024 21:21:53 +0200 Subject: [PATCH] refactor: improve propagation rule diagnostics --- .../generator/ModelSemanticsFactory.java | 3 +- .../language/semantics/ModelInitializer.java | 1 + .../language/semantics/ProblemTrace.java | 4 ++ .../language/semantics/ProblemTraceImpl.java | 24 +++++-- .../web/semantics/SemanticsWorker.java | 67 ++++++++++++------- .../dse/propagation/PropagationBuilder.java | 2 + .../PropagationRejectedResult.java | 8 ++- .../impl/PropagationAdapterImpl.java | 10 +-- .../impl/PropagationBuilderImpl.java | 9 ++- .../impl/PropagationStoreAdapterImpl.java | 10 ++- .../impl/rule/BoundPropagationRule.java | 3 +- 11 files changed, 104 insertions(+), 37 deletions(-) diff --git a/subprojects/generator/src/main/java/tools/refinery/generator/ModelSemanticsFactory.java b/subprojects/generator/src/main/java/tools/refinery/generator/ModelSemanticsFactory.java index 77f05a9d2..3c3488dfc 100644 --- a/subprojects/generator/src/main/java/tools/refinery/generator/ModelSemanticsFactory.java +++ b/subprojects/generator/src/main/java/tools/refinery/generator/ModelSemanticsFactory.java @@ -41,7 +41,8 @@ public ModelSemantics tryCreateSemantics(Problem problem) { var storeBuilder = ModelStore.builder() .cancellationToken(cancellationToken) .with(QueryInterpreterAdapter.builder()) - .with(PropagationAdapter.builder()) + .with(PropagationAdapter.builder() + .throwOnFatalRejection(false)) .with(ReasoningAdapter.builder() .requiredInterpretations(Set.of(Concreteness.PARTIAL))); initializer.configureStoreBuilder(storeBuilder); diff --git a/subprojects/language-semantics/src/main/java/tools/refinery/language/semantics/ModelInitializer.java b/subprojects/language-semantics/src/main/java/tools/refinery/language/semantics/ModelInitializer.java index c2243f085..8bfc8acdf 100644 --- a/subprojects/language-semantics/src/main/java/tools/refinery/language/semantics/ModelInitializer.java +++ b/subprojects/language-semantics/src/main/java/tools/refinery/language/semantics/ModelInitializer.java @@ -967,6 +967,7 @@ private void collectRule(RuleDefinition ruleDefinition, ModelStoreBuilder storeB .orElseGet(() -> "::rule" + ruleCount); ruleCount++; var rule = toRule(name, ruleDefinition); + problemTrace.putRuleDefinition(ruleDefinition, rule); switch (ruleDefinition.getKind()) { case DECISION -> storeBuilder.tryGetAdapter(DesignSpaceExplorationBuilder.class) .ifPresent(dseBuilder -> dseBuilder.transformation(rule)); diff --git a/subprojects/language-semantics/src/main/java/tools/refinery/language/semantics/ProblemTrace.java b/subprojects/language-semantics/src/main/java/tools/refinery/language/semantics/ProblemTrace.java index b8d0b8045..9122cf1fa 100644 --- a/subprojects/language-semantics/src/main/java/tools/refinery/language/semantics/ProblemTrace.java +++ b/subprojects/language-semantics/src/main/java/tools/refinery/language/semantics/ProblemTrace.java @@ -10,6 +10,8 @@ import tools.refinery.language.model.problem.Node; import tools.refinery.language.model.problem.Problem; import tools.refinery.language.model.problem.Relation; +import tools.refinery.language.model.problem.RuleDefinition; +import tools.refinery.store.dse.transition.Rule; import tools.refinery.store.reasoning.representation.AnyPartialSymbol; import tools.refinery.store.reasoning.representation.PartialRelation; import tools.refinery.store.reasoning.translator.TranslationException; @@ -36,6 +38,8 @@ public interface ProblemTrace { Relation getRelation(AnyPartialSymbol partialSymbol); + RuleDefinition getRuleDefinition(Rule rule); + RuntimeException wrapException(TranslationException translationException); PartialRelation getPartialRelation(Relation relation); diff --git a/subprojects/language-semantics/src/main/java/tools/refinery/language/semantics/ProblemTraceImpl.java b/subprojects/language-semantics/src/main/java/tools/refinery/language/semantics/ProblemTraceImpl.java index 4b44339cb..5584af100 100644 --- a/subprojects/language-semantics/src/main/java/tools/refinery/language/semantics/ProblemTraceImpl.java +++ b/subprojects/language-semantics/src/main/java/tools/refinery/language/semantics/ProblemTraceImpl.java @@ -13,10 +13,8 @@ import org.eclipse.xtext.naming.QualifiedName; import org.eclipse.xtext.scoping.IScope; import org.eclipse.xtext.scoping.IScopeProvider; -import tools.refinery.language.model.problem.Node; -import tools.refinery.language.model.problem.Problem; -import tools.refinery.language.model.problem.ProblemPackage; -import tools.refinery.language.model.problem.Relation; +import tools.refinery.language.model.problem.*; +import tools.refinery.store.dse.transition.Rule; import tools.refinery.store.reasoning.representation.AnyPartialSymbol; import tools.refinery.store.reasoning.representation.PartialRelation; import tools.refinery.store.reasoning.translator.TranslationException; @@ -46,6 +44,7 @@ class ProblemTraceImpl implements ProblemTrace { Collections.unmodifiableMap(mutableRelationTrace); private final Map mutableInverseTrace = new HashMap<>(); private final Map inverseTrace = Collections.unmodifiableMap(mutableInverseTrace); + private final Map mutableInverseRuleDefinitionTrace = new LinkedHashMap<>(); @Override public Problem getProblem() { @@ -129,6 +128,23 @@ public Relation getRelation(AnyPartialSymbol partialSymbol) { return relation; } + void putRuleDefinition(RuleDefinition ruleDefinition, Rule rule) { + var oldRuleDefinition = mutableInverseRuleDefinitionTrace.put(rule, ruleDefinition); + if (oldRuleDefinition != null) { + throw new TracedException(oldRuleDefinition, "Rule definition %s was already mapped to rule" + .formatted(rule.getName())); + } + } + + @Override + public RuleDefinition getRuleDefinition(Rule rule) { + var ruleDefinition = mutableInverseRuleDefinitionTrace.get(rule); + if (ruleDefinition == null) { + throw new IllegalArgumentException("No definition for rule: " + rule.getName()); + } + return ruleDefinition; + } + @Override public RuntimeException wrapException(TranslationException translationException) { var partialSymbol = translationException.getPartialSymbol(); diff --git a/subprojects/language-web/src/main/java/tools/refinery/language/web/semantics/SemanticsWorker.java b/subprojects/language-web/src/main/java/tools/refinery/language/web/semantics/SemanticsWorker.java index 4ec11bb9b..c64853099 100644 --- a/subprojects/language-web/src/main/java/tools/refinery/language/web/semantics/SemanticsWorker.java +++ b/subprojects/language-web/src/main/java/tools/refinery/language/web/semantics/SemanticsWorker.java @@ -8,6 +8,7 @@ import com.google.inject.Inject; import org.eclipse.emf.common.util.Diagnostic; import org.eclipse.emf.ecore.EObject; +import org.eclipse.emf.ecore.EStructuralFeature; import org.eclipse.xtext.service.OperationCanceledManager; import org.eclipse.xtext.util.CancelIndicator; import org.eclipse.xtext.validation.CheckType; @@ -18,11 +19,14 @@ import tools.refinery.generator.ModelSemantics; import tools.refinery.generator.ModelSemanticsFactory; import tools.refinery.language.model.problem.Problem; +import tools.refinery.language.model.problem.ProblemPackage; import tools.refinery.language.model.problem.ScopeDeclaration; +import tools.refinery.language.semantics.ProblemTrace; import tools.refinery.language.semantics.TracedException; import tools.refinery.language.web.semantics.metadata.MetadataCreator; import tools.refinery.store.dse.propagation.PropagationRejectedResult; import tools.refinery.store.dse.propagation.PropagationResult; +import tools.refinery.store.dse.transition.Rule; import tools.refinery.store.reasoning.literal.Concreteness; import tools.refinery.store.reasoning.scope.ScopePropagator; import tools.refinery.store.reasoning.translator.TranslationException; @@ -79,18 +83,31 @@ public SemanticsResult call() { } cancellationToken.checkCancelled(); var modelResult = createSemanticsModelResult(semantics); - return createSemanticsResult(modelResult, semantics.getPropagationResult()); + return createSemanticsResult(modelResult, semantics.getProblemTrace(), semantics.getPropagationResult()); } - private SemanticsResult getTracedErrorResult(EObject sourceElement, String message) { + var diagnostics = getTracedDiagnostics(sourceElement, null, message); + return getSemanticsResultWithDiagnostics(null, message, diagnostics); + } + + private List getTracedDiagnostics( + EObject sourceElement, EStructuralFeature feature, String message) { if (sourceElement == null || !problem.eResource().equals(sourceElement.eResource())) { - return new SemanticsResult(message); + return List.of(); } - var diagnostic = new FeatureBasedDiagnostic(Diagnostic.ERROR, message, sourceElement, null, 0, + var diagnostic = new FeatureBasedDiagnostic(Diagnostic.ERROR, message, sourceElement, feature, 0, CheckType.EXPENSIVE, DIAGNOSTIC_ID); - var issues = convertIssues(List.of(diagnostic)); - return new SemanticsResult(issues); + return List.of(diagnostic); + } + + private SemanticsResult getSemanticsResultWithDiagnostics( + SemanticsModelResult modelResult, String message, List diagnostics) { + if (diagnostics.isEmpty()) { + return new SemanticsResult(modelResult, message); + } + var issues = convertIssues(diagnostics); + return new SemanticsResult(modelResult, issues); } private List convertIssues(List diagnostics) { @@ -114,26 +131,30 @@ private SemanticsModelResult createSemanticsModelResult(ModelSemantics semantics return new SemanticsModelResult(nodesMetadata, relationsMetadata, partialInterpretation); } - private SemanticsResult createSemanticsResult(SemanticsModelResult modelResult, - PropagationResult propagationResult) { + private SemanticsResult createSemanticsResult( + SemanticsModelResult modelResult, ProblemTrace trace, PropagationResult propagationResult) { if (!(propagationResult instanceof PropagationRejectedResult rejectedResult)) { return new SemanticsResult(modelResult); } var message = rejectedResult.formatMessage(); - if (!(rejectedResult.reason() instanceof ScopePropagator)) { - return new SemanticsResult(modelResult, message); - } - var diagnostics = new ArrayList(); - for (var statement : problem.getStatements()) { - if (statement instanceof ScopeDeclaration scopeDeclaration) { - diagnostics.add(new FeatureBasedDiagnostic(Diagnostic.ERROR, message, scopeDeclaration, null, 0, - CheckType.EXPENSIVE, DIAGNOSTIC_ID)); - } - } - if (diagnostics.isEmpty()) { - return new SemanticsResult(modelResult, message); - } - var issues = convertIssues(diagnostics); - return new SemanticsResult(modelResult, issues); + List diagnostics = switch (rejectedResult.reason()) { + case ScopePropagator ignored -> getScopePropagatorDiagnostics(message); + case Rule rule -> getRuleDiagnostics(rule, trace, message); + default -> List.of(); + }; + return getSemanticsResultWithDiagnostics(modelResult, message, diagnostics); + } + + private List getScopePropagatorDiagnostics(String message) { + return problem.getStatements().stream() + .filter(ScopeDeclaration.class::isInstance) + .map(eObject -> new FeatureBasedDiagnostic(Diagnostic.ERROR, message, eObject, null, 0, + CheckType.EXPENSIVE, DIAGNOSTIC_ID)) + .toList(); + } + + private List getRuleDiagnostics(Rule rule, ProblemTrace trace, String message) { + var ruleDefinition = trace.getRuleDefinition(rule); + return getTracedDiagnostics(ruleDefinition, ProblemPackage.Literals.NAMED_ELEMENT__NAME, message); } } diff --git a/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/PropagationBuilder.java b/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/PropagationBuilder.java index f8a89b301..2a22f00db 100644 --- a/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/PropagationBuilder.java +++ b/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/PropagationBuilder.java @@ -27,6 +27,8 @@ default PropagationBuilder rules(Collection propagationRules) { PropagationBuilder propagator(Propagator propagator); + PropagationBuilder throwOnFatalRejection(boolean throwOnFatalRejection); + @Override PropagationStoreAdapter build(ModelStore store); } diff --git a/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/PropagationRejectedResult.java b/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/PropagationRejectedResult.java index 6d696de72..0ef1dc389 100644 --- a/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/PropagationRejectedResult.java +++ b/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/PropagationRejectedResult.java @@ -5,7 +5,11 @@ */ package tools.refinery.store.dse.propagation; -public record PropagationRejectedResult(Object reason, String message) implements PropagationResult { +public record PropagationRejectedResult(Object reason, String message, boolean fatal) implements PropagationResult { + public PropagationRejectedResult(Object reason, String message) { + this(reason, message, false); + } + @Override public PropagationResult andThen(PropagationResult next) { return this; @@ -18,7 +22,7 @@ public boolean isRejected() { @Override public void throwIfRejected() { - throw new IllegalArgumentException(formatMessage()); + throw new IllegalStateException(formatMessage()); } public String formatMessage() { diff --git a/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/PropagationAdapterImpl.java b/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/PropagationAdapterImpl.java index fdd19217a..7158a7abb 100644 --- a/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/PropagationAdapterImpl.java +++ b/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/PropagationAdapterImpl.java @@ -5,10 +5,7 @@ */ package tools.refinery.store.dse.propagation.impl; -import tools.refinery.store.dse.propagation.BoundPropagator; -import tools.refinery.store.dse.propagation.PropagationAdapter; -import tools.refinery.store.dse.propagation.PropagationResult; -import tools.refinery.store.dse.propagation.PropagationStoreAdapter; +import tools.refinery.store.dse.propagation.*; import tools.refinery.store.model.Model; class PropagationAdapterImpl implements PropagationAdapter { @@ -35,6 +32,11 @@ public PropagationResult propagate() { lastResult = propagateOne(); result = result.andThen(lastResult); } while (lastResult.isChanged()); + if (lastResult instanceof PropagationRejectedResult rejectedResult && + rejectedResult.fatal() && + storeAdapter.isThrowOnFatalRejection()) { + rejectedResult.throwIfRejected(); + } return result; } diff --git a/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/PropagationBuilderImpl.java b/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/PropagationBuilderImpl.java index c844a89f1..da32fe443 100644 --- a/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/PropagationBuilderImpl.java +++ b/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/PropagationBuilderImpl.java @@ -20,6 +20,7 @@ public class PropagationBuilderImpl extends AbstractModelAdapterBuilder propagationRules = new LinkedHashSet<>(); private final Deque propagators = new ArrayDeque<>(); + private boolean throwOnFatalRejection = true; @Override public PropagationBuilder rule(Rule propagationRule) { @@ -35,6 +36,12 @@ public PropagationBuilder propagator(Propagator propagator) { return this; } + @Override + public PropagationBuilder throwOnFatalRejection(boolean throwOnFatalRejection) { + this.throwOnFatalRejection = throwOnFatalRejection; + return this; + } + @Override protected void doConfigure(ModelStoreBuilder storeBuilder) { super.doConfigure(storeBuilder); @@ -48,6 +55,6 @@ protected void doConfigure(ModelStoreBuilder storeBuilder) { @Override protected PropagationStoreAdapter doBuild(ModelStore store) { - return new PropagationStoreAdapterImpl(store, List.copyOf(propagators)); + return new PropagationStoreAdapterImpl(store, List.copyOf(propagators), throwOnFatalRejection); } } diff --git a/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/PropagationStoreAdapterImpl.java b/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/PropagationStoreAdapterImpl.java index a223caed7..4f7b86220 100644 --- a/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/PropagationStoreAdapterImpl.java +++ b/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/PropagationStoreAdapterImpl.java @@ -13,13 +13,17 @@ import java.util.List; +// This should not be a record, because we don't want auto-generated {@code equals} and {@code hashCode} methods. +@SuppressWarnings("ClassCanBeRecord") class PropagationStoreAdapterImpl implements PropagationStoreAdapter { private final ModelStore store; private final List propagators; + private final boolean throwOnFatalRejection; - PropagationStoreAdapterImpl(ModelStore store, List propagators) { + PropagationStoreAdapterImpl(ModelStore store, List propagators, boolean throwOnFatalRejection) { this.store = store; this.propagators = propagators; + this.throwOnFatalRejection = throwOnFatalRejection; } @Override @@ -35,4 +39,8 @@ public PropagationAdapter createModelAdapter(Model model) { List getPropagators() { return propagators; } + + boolean isThrowOnFatalRejection() { + return throwOnFatalRejection; + } } diff --git a/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/rule/BoundPropagationRule.java b/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/rule/BoundPropagationRule.java index f91882be1..cc1e0486c 100644 --- a/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/rule/BoundPropagationRule.java +++ b/subprojects/store-dse/src/main/java/tools/refinery/store/dse/propagation/impl/rule/BoundPropagationRule.java @@ -44,7 +44,8 @@ public void afterRestore() { public PropagationResult fireAll() { if (!firedActivations.isEmpty()) { - throw new IllegalStateException("Stuck propagation rule '%s'.".formatted(rule.getName())); + return new PropagationRejectedResult(rule, "Propagation rule '%s' got stuck.".formatted(rule.getName()), + true); } if (resultSet.size() == 0) { return PropagationResult.UNCHANGED;