From 42e809a5e9d3cc02653075209b49a540b7e33f43 Mon Sep 17 00:00:00 2001 From: Peter Donovan Date: Thu, 15 Jun 2023 15:25:05 -0700 Subject: [PATCH 1/6] Update formatter. This runs another pass of optimization. Yes, this is totally a hacky heuristic, but it should not affect idempotency nor other correctness criteria. This can affect performance, but I modified the formatter to keep track of whether anything has changed so that it knows whether further optimization passes have the potential to give any improved results. --- .../java/org/lflang/ast/MalleableString.java | 38 +++++++++++-------- .../DistributedLogicalActionUpstreamLong.lf | 3 +- .../DistributedPhysicalActionUpstreamLong.lf | 3 +- .../src/modal_models/BanksModalStateReset.lf | 3 +- test/C/src/modal_models/ModalActions.lf | 3 +- .../src/modal_models/ModalStartupShutdown.lf | 3 +- .../src/modal_models/BanksModalStateReset.lf | 3 +- test/Python/src/modal_models/ModalActions.lf | 3 +- .../src/modal_models/ModalStartupShutdown.lf | 3 +- test/TypeScript/src/DeadlineHandledAbove.lf | 4 +- test/TypeScript/src/SimpleDeadline.lf | 4 +- 11 files changed, 33 insertions(+), 37 deletions(-) diff --git a/core/src/main/java/org/lflang/ast/MalleableString.java b/core/src/main/java/org/lflang/ast/MalleableString.java index d4dff9dae7..1081182ec8 100644 --- a/core/src/main/java/org/lflang/ast/MalleableString.java +++ b/core/src/main/java/org/lflang/ast/MalleableString.java @@ -42,8 +42,9 @@ public MalleableString indent() { * whole of this. * @param indentation The number of spaces used per level of indentation. * @param singleLineCommentPrefix The prefix that marks the start of a single-line comment. + * @return Whether the best representation changed. */ - public abstract void findBestRepresentation( + public abstract boolean findBestRepresentation( Supplier providedRender, ToLongFunction badness, int width, @@ -348,7 +349,7 @@ private List getLinesOfInterest( } @Override - public void findBestRepresentation( + public boolean findBestRepresentation( Supplier providedRender, ToLongFunction badness, int width, @@ -356,32 +357,37 @@ public void findBestRepresentation( String singleLineCommentPrefix) { this.width = width; keepCommentsOnSameLine = true; - optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); + var everChanged = false; + var changed = optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); + everChanged = changed; + if (changed) changed = optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); if (components.stream() .noneMatch( it -> it.render(indentation, singleLineCommentPrefix, false, null) .unplacedComments .findAny() - .isPresent())) return; + .isPresent())) return changed; long badnessTrue = badness.applyAsLong(providedRender.get()); keepCommentsOnSameLine = false; - optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); + changed = optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); + everChanged |= changed; long badnessFalse = badness.applyAsLong(providedRender.get()); keepCommentsOnSameLine = badnessTrue < badnessFalse; - optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); - optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); + if (changed) changed = optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); + if (changed) optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); + return everChanged; } - private void optimizeChildren( + private boolean optimizeChildren( Supplier providedRender, ToLongFunction badness, int width, int indentation, String singleLineCommentPrefix) { - components + return components .reverse() - .forEach( + .stream().anyMatch( it -> it.findBestRepresentation( providedRender, badness, width, indentation, singleLineCommentPrefix)); @@ -409,14 +415,14 @@ public MalleableString indent() { } @Override - public void findBestRepresentation( + public boolean findBestRepresentation( Supplier providedRender, ToLongFunction badness, int width, int indentation, String singleLineCommentPrefix) { this.width = width; - nested.findBestRepresentation( + return nested.findBestRepresentation( providedRender, badness, width - indentation, indentation, singleLineCommentPrefix); } @@ -462,12 +468,13 @@ public String toString() { } @Override - public void findBestRepresentation( + public boolean findBestRepresentation( Supplier providedRender, ToLongFunction badness, int width, int indentation, String singleLineCommentPrefix) { + var initialChosenPossibility = getChosenPossibility(); bestPossibility = Collections.min( getPossibilities(), @@ -479,9 +486,10 @@ public void findBestRepresentation( return Math.toIntExact(badnessA - badnessB); }); if (bestPossibility instanceof MalleableString ms) { - ms.findBestRepresentation( - providedRender, badness, width, indentation, singleLineCommentPrefix); + if (ms.findBestRepresentation( + providedRender, badness, width, indentation, singleLineCommentPrefix)) return true; } + return getChosenPossibility() != initialChosenPossibility; } /** Return the best representation of this. */ diff --git a/test/C/src/federated/DistributedLogicalActionUpstreamLong.lf b/test/C/src/federated/DistributedLogicalActionUpstreamLong.lf index 87dd86229e..3205c466c2 100644 --- a/test/C/src/federated/DistributedLogicalActionUpstreamLong.lf +++ b/test/C/src/federated/DistributedLogicalActionUpstreamLong.lf @@ -46,8 +46,7 @@ federated reactor { passThroughs7.out, passThroughs8.out, passThroughs9.out, - passThroughs10.out - -> + passThroughs10.out -> passThroughs1.in, passThroughs2.in, passThroughs3.in, diff --git a/test/C/src/federated/DistributedPhysicalActionUpstreamLong.lf b/test/C/src/federated/DistributedPhysicalActionUpstreamLong.lf index 4fe2407b54..cc9f556df0 100644 --- a/test/C/src/federated/DistributedPhysicalActionUpstreamLong.lf +++ b/test/C/src/federated/DistributedPhysicalActionUpstreamLong.lf @@ -71,8 +71,7 @@ federated reactor { passThroughs7.out, passThroughs8.out, passThroughs9.out, - passThroughs10.out - -> + passThroughs10.out -> passThroughs1.in, passThroughs2.in, passThroughs3.in, diff --git a/test/C/src/modal_models/BanksModalStateReset.lf b/test/C/src/modal_models/BanksModalStateReset.lf index 57228081c2..90d7eaf932 100644 --- a/test/C/src/modal_models/BanksModalStateReset.lf +++ b/test/C/src/modal_models/BanksModalStateReset.lf @@ -42,8 +42,7 @@ main reactor { reset2.mode_switch, reset2.count0, reset2.count1, - reset2.count2 - -> test.events + reset2.count2 -> test.events // Trigger mode change (separately because of #1278) reaction(stepper) -> reset1.next {= diff --git a/test/C/src/modal_models/ModalActions.lf b/test/C/src/modal_models/ModalActions.lf index 594273bb6f..449119d315 100644 --- a/test/C/src/modal_models/ModalActions.lf +++ b/test/C/src/modal_models/ModalActions.lf @@ -87,8 +87,7 @@ main reactor { modal.action1_sched, modal.action1_exec, modal.action2_sched, - modal.action2_exec - -> test.events + modal.action2_exec -> test.events reaction(stepper) -> modal.next {= lf_set(modal.next, true); =} // Trigger mode change } diff --git a/test/C/src/modal_models/ModalStartupShutdown.lf b/test/C/src/modal_models/ModalStartupShutdown.lf index 3a93cebfdd..3de03e3ab5 100644 --- a/test/C/src/modal_models/ModalStartupShutdown.lf +++ b/test/C/src/modal_models/ModalStartupShutdown.lf @@ -135,8 +135,7 @@ main reactor { modal.shutdown4, modal.startup5, modal.reset5, - modal.shutdown5 - -> test.events + modal.shutdown5 -> test.events reaction(stepper) -> modal.next {= lf_set(modal.next, true); =} // Trigger mode change } diff --git a/test/Python/src/modal_models/BanksModalStateReset.lf b/test/Python/src/modal_models/BanksModalStateReset.lf index d127288e0b..c81f8eea3e 100644 --- a/test/Python/src/modal_models/BanksModalStateReset.lf +++ b/test/Python/src/modal_models/BanksModalStateReset.lf @@ -42,8 +42,7 @@ main reactor { reset2.mode_switch, reset2.count0, reset2.count1, - reset2.count2 - -> test.events + reset2.count2 -> test.events # Trigger mode change (separately because of #1278) reaction(stepper) -> reset1.next {= diff --git a/test/Python/src/modal_models/ModalActions.lf b/test/Python/src/modal_models/ModalActions.lf index fd4adf8eb8..73f565c62a 100644 --- a/test/Python/src/modal_models/ModalActions.lf +++ b/test/Python/src/modal_models/ModalActions.lf @@ -87,8 +87,7 @@ main reactor { modal.action1_sched, modal.action1_exec, modal.action2_sched, - modal.action2_exec - -> test.events + modal.action2_exec -> test.events reaction(stepper) -> modal.next {= modal.next.set(True) =} # Trigger mode change } diff --git a/test/Python/src/modal_models/ModalStartupShutdown.lf b/test/Python/src/modal_models/ModalStartupShutdown.lf index f5210580d1..e464f79c42 100644 --- a/test/Python/src/modal_models/ModalStartupShutdown.lf +++ b/test/Python/src/modal_models/ModalStartupShutdown.lf @@ -135,8 +135,7 @@ main reactor { modal.shutdown4, modal.startup5, modal.reset5, - modal.shutdown5 - -> test.events + modal.shutdown5 -> test.events reaction(stepper) -> modal.next {= modal.next.set(True) =} # Trigger mode change } diff --git a/test/TypeScript/src/DeadlineHandledAbove.lf b/test/TypeScript/src/DeadlineHandledAbove.lf index 8ab2bb5617..8d4f18976a 100644 --- a/test/TypeScript/src/DeadlineHandledAbove.lf +++ b/test/TypeScript/src/DeadlineHandledAbove.lf @@ -10,9 +10,7 @@ reactor Deadline(threshold: time = 100 msec) { reaction(x) -> deadline_violation {= util.requestErrorStop("ERROR: Deadline violation was not detected!") - =} deadline( - threshold - ) {= + =} deadline(threshold) {= console.log("Deadline violation detected."); deadline_violation = true; =} diff --git a/test/TypeScript/src/SimpleDeadline.lf b/test/TypeScript/src/SimpleDeadline.lf index dde3cd07e9..56ffd3f64f 100644 --- a/test/TypeScript/src/SimpleDeadline.lf +++ b/test/TypeScript/src/SimpleDeadline.lf @@ -8,9 +8,7 @@ reactor Deadline(threshold: time = 100 msec) { reaction(x) -> deadlineViolation {= util.requestErrorStop("ERROR: Deadline violation was not detected!") - =} deadline( - threshold - ) {= + =} deadline(threshold) {= console.log("Deadline violation detected."); deadlineViolation = true; =} From 8a0cc25cf9df92568356eb9f75d0518d638d16f0 Mon Sep 17 00:00:00 2001 From: Peter Donovan Date: Thu, 15 Jun 2023 15:27:14 -0700 Subject: [PATCH 2/6] Run the formatter if there are no syntax errors. Previously any messages (even just info messages!) would stop the formatter from being run, which is an unnecessary requirement. If we can understand the AST, we can format it. --- .../java/org/lflang/formatting2/LFFormatter.java | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/lflang/formatting2/LFFormatter.java b/core/src/main/java/org/lflang/formatting2/LFFormatter.java index 4d7c511b76..e2ea020af1 100644 --- a/core/src/main/java/org/lflang/formatting2/LFFormatter.java +++ b/core/src/main/java/org/lflang/formatting2/LFFormatter.java @@ -7,6 +7,7 @@ import com.google.inject.Inject; import java.util.List; import org.eclipse.emf.ecore.EObject; +import org.eclipse.xtext.diagnostics.Severity; import org.eclipse.xtext.formatting2.FormatterRequest; import org.eclipse.xtext.formatting2.IFormatter2; import org.eclipse.xtext.formatting2.regionaccess.ITextReplacement; @@ -24,13 +25,11 @@ public class LFFormatter implements IFormatter2 { @Override public List format(FormatterRequest request) { // TODO: Use a CancelIndicator that actually cancels? - if (!request.getTextRegionAccess().getResource().getErrors().isEmpty() - || !validator - .validate( - request.getTextRegionAccess().getResource(), - CheckMode.ALL, - CancelIndicator.NullImpl) - .isEmpty()) { + if (validator + .validate( + request.getTextRegionAccess().getResource(), CheckMode.ALL, CancelIndicator.NullImpl) + .stream() + .anyMatch(it -> it.isSyntaxError() && it.getSeverity() == Severity.ERROR)) { return List.of(); } ITextSegment documentRegion = request.getTextRegionAccess().regionForDocument(); From 0484b4e7f2db2e6c11cfab6849cfb9cce7a882ff Mon Sep 17 00:00:00 2001 From: Peter Donovan Date: Thu, 15 Jun 2023 15:31:36 -0700 Subject: [PATCH 3/6] Format Java. --- .../java/org/lflang/ast/MalleableString.java | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/org/lflang/ast/MalleableString.java b/core/src/main/java/org/lflang/ast/MalleableString.java index 1081182ec8..7a9c42b231 100644 --- a/core/src/main/java/org/lflang/ast/MalleableString.java +++ b/core/src/main/java/org/lflang/ast/MalleableString.java @@ -358,9 +358,12 @@ public boolean findBestRepresentation( this.width = width; keepCommentsOnSameLine = true; var everChanged = false; - var changed = optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); + var changed = + optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); everChanged = changed; - if (changed) changed = optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); + if (changed) + changed = + optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); if (components.stream() .noneMatch( it -> @@ -370,12 +373,16 @@ public boolean findBestRepresentation( .isPresent())) return changed; long badnessTrue = badness.applyAsLong(providedRender.get()); keepCommentsOnSameLine = false; - changed = optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); + changed = + optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); everChanged |= changed; long badnessFalse = badness.applyAsLong(providedRender.get()); keepCommentsOnSameLine = badnessTrue < badnessFalse; - if (changed) changed = optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); - if (changed) optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); + if (changed) + changed = + optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); + if (changed) + optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix); return everChanged; } @@ -385,9 +392,8 @@ private boolean optimizeChildren( int width, int indentation, String singleLineCommentPrefix) { - return components - .reverse() - .stream().anyMatch( + return components.reverse().stream() + .anyMatch( it -> it.findBestRepresentation( providedRender, badness, width, indentation, singleLineCommentPrefix)); From f67d77fab5847bdc0567dd681835e29e3fbe4ec3 Mon Sep 17 00:00:00 2001 From: Peter Donovan Date: Thu, 15 Jun 2023 15:37:16 -0700 Subject: [PATCH 4/6] Add test case. --- .../test/java/org/lflang/cli/LffCliTest.java | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/cli/lff/src/test/java/org/lflang/cli/LffCliTest.java b/cli/lff/src/test/java/org/lflang/cli/LffCliTest.java index 43c045919b..89e09674ba 100644 --- a/cli/lff/src/test/java/org/lflang/cli/LffCliTest.java +++ b/cli/lff/src/test/java/org/lflang/cli/LffCliTest.java @@ -141,7 +141,37 @@ reactor Filter(period: int = 0, b: double[] = {0, 0}) { // baz state grid2: SnakeGrid = {= SnakeGrid::new(grid_side, &snake) =} } - """)); + """), + List.of( + """ + target Cpp + + reactor ContextManager { + + + \s + } + + reactor MACService { + mul_cm = new ContextManager() + } + + """, + """ + target Cpp + + reactor ContextManager { + } + + reactor MACService { + mul_cm = new ContextManager< + loooooooooooooooooooooooooooooong, + looooooooooooooong, + loooooooooooooong + >() + } + """ + )); LffTestFixture lffTester = new LffTestFixture(); From 1b785a932a9420b32099ef6d73b9b5a079a02fa2 Mon Sep 17 00:00:00 2001 From: Peter Donovan Date: Thu, 15 Jun 2023 23:01:14 -0700 Subject: [PATCH 5/6] Format Java. --- .../test/java/org/lflang/cli/LffCliTest.java | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/cli/lff/src/test/java/org/lflang/cli/LffCliTest.java b/cli/lff/src/test/java/org/lflang/cli/LffCliTest.java index 89e09674ba..27c43c80cc 100644 --- a/cli/lff/src/test/java/org/lflang/cli/LffCliTest.java +++ b/cli/lff/src/test/java/org/lflang/cli/LffCliTest.java @@ -145,24 +145,24 @@ reactor Filter(period: int = 0, b: double[] = {0, 0}) { List.of( """ target Cpp - + reactor ContextManager { - - + + \s } - + reactor MACService { mul_cm = new ContextManager() } - + """, """ target Cpp - + reactor ContextManager { } - + reactor MACService { mul_cm = new ContextManager< loooooooooooooooooooooooooooooong, @@ -170,8 +170,7 @@ reactor Filter(period: int = 0, b: double[] = {0, 0}) { loooooooooooooong >() } - """ - )); + """)); LffTestFixture lffTester = new LffTestFixture(); From c8b9f1ad454a5b45834a7d8348563da236ec227e Mon Sep 17 00:00:00 2001 From: Peter Donovan Date: Fri, 16 Jun 2023 00:00:39 -0700 Subject: [PATCH 6/6] Add comment. --- core/src/main/java/org/lflang/ast/MalleableString.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/core/src/main/java/org/lflang/ast/MalleableString.java b/core/src/main/java/org/lflang/ast/MalleableString.java index 7a9c42b231..ac4014748d 100644 --- a/core/src/main/java/org/lflang/ast/MalleableString.java +++ b/core/src/main/java/org/lflang/ast/MalleableString.java @@ -357,6 +357,14 @@ public boolean findBestRepresentation( String singleLineCommentPrefix) { this.width = width; keepCommentsOnSameLine = true; + // Multiple calls to optimizeChildren may be required because as parts of the textual + // representation are updated, the optimal representation of other parts may change. + // For example, if the text is wider than 100 characters, the line may only need to be + // broken in one place, but it will be broken in multiple places a second optimization pass + // is not made. This is a heuristic in the sense that two passes are not guaranteed to result + // in a fixed point, but since a subsequent call to the formatter will get the same AST and + // therefore have the same starting point, the formatter as a whole should still be + // idempotent. var everChanged = false; var changed = optimizeChildren(providedRender, badness, width, indentation, singleLineCommentPrefix);