From de744aef5416594b915017bfa95b978c8f0f8cf2 Mon Sep 17 00:00:00 2001 From: Joseph Humfrey Date: Thu, 19 Apr 2018 11:19:35 +0100 Subject: [PATCH] Re-wrote loose end detection code to be more robust (e.g. detect unterminated choices) --- compiler/InkParser/InkParser.cs | 2 +- compiler/ParsedHierarchy/FlowBase.cs | 162 ++++++--------------------- compiler/ParsedHierarchy/Object.cs | 10 +- compiler/ParsedHierarchy/Story.cs | 2 +- compiler/ParsedHierarchy/Weave.cs | 146 ++++++++++++++++++++++-- tests/Tests.cs | 2 + 6 files changed, 185 insertions(+), 139 deletions(-) diff --git a/compiler/InkParser/InkParser.cs b/compiler/InkParser/InkParser.cs index e75856a5..1cbf2c58 100644 --- a/compiler/InkParser/InkParser.cs +++ b/compiler/InkParser/InkParser.cs @@ -44,7 +44,7 @@ public Parsed.Story Parse() // continue with errors. Returning an empty include files meant that anything // that *did* compile successfully would otherwise be ignored, generating way // more errors than necessary. - return new Parsed.Story (topLevelContent); + return new Parsed.Story (topLevelContent, isInclude:_rootParser != this); } protected List SeparatedList (SpecificParseRule mainRule, ParseRule separatorRule) where T : class diff --git a/compiler/ParsedHierarchy/FlowBase.cs b/compiler/ParsedHierarchy/FlowBase.cs index 501ec01c..4c2163bd 100644 --- a/compiler/ParsedHierarchy/FlowBase.cs +++ b/compiler/ParsedHierarchy/FlowBase.cs @@ -20,7 +20,7 @@ internal class Argument public abstract FlowLevel flowLevel { get; } public bool isFunction { get; protected set; } - public FlowBase (string name = null, List topLevelObjects = null, List arguments = null, bool isFunction = false) + public FlowBase (string name = null, List topLevelObjects = null, List arguments = null, bool isFunction = false, bool isIncludedStory = false) { this.name = name; @@ -31,7 +31,7 @@ public FlowBase (string name = null, List topLevelObjects = null, // Used by story to add includes PreProcessTopLevelObjects (topLevelObjects); - topLevelObjects = SplitWeaveAndSubFlowContent (topLevelObjects); + topLevelObjects = SplitWeaveAndSubFlowContent (topLevelObjects, isRootStory:this is Story && !isIncludedStory); AddContent(topLevelObjects); @@ -40,7 +40,7 @@ public FlowBase (string name = null, List topLevelObjects = null, this.variableDeclarations = new Dictionary (); } - List SplitWeaveAndSubFlowContent(List contentObjs) + List SplitWeaveAndSubFlowContent(List contentObjs, bool isRootStory) { var weaveObjs = new List (); var subFlowObjs = new List (); @@ -61,11 +61,19 @@ public FlowBase (string name = null, List topLevelObjects = null, } } + // Implicit final gather in top level story for ending without warning that you run out of content + if (isRootStory) { + weaveObjs.Add (new Gather (null, 1)); + weaveObjs.Add (new Divert (new Path ("END"))); + } + var finalContent = new List (); + if (weaveObjs.Count > 0) { _rootWeave = new Weave (weaveObjs, 0); finalContent.Add (_rootWeave); } + if (subFlowObjs.Count > 0) { finalContent.AddRange (subFlowObjs); } @@ -161,13 +169,17 @@ public void ResolveWeavePointNaming () public override Runtime.Object GenerateRuntimeObject () { - // Check whether flow has a loose end: - // - Most flows should end in a choice or a divert (otherwise issue a warning) - // - Functions need a return, otherwise an implicit one is added - ValidateTermination(); - + Return foundReturn = null; if (isFunction) { CheckForDisallowedFunctionFlowControl (); + } + + // Non-functon: Make sure knots and stitches don't attempt to use Return statement + else if( flowLevel == FlowLevel.Knot || flowLevel == FlowLevel.Stitch ) { + foundReturn = Find (); + if (foundReturn != null) { + Error ("Return statements can only be used in knots that are declared as functions: == function " + this.name + " ==", foundReturn); + } } var container = new Runtime.Container (); @@ -234,23 +246,15 @@ public override Runtime.Object GenerateRuntimeObject () contentIdx++; } - // Tie up final loose ends to the very end - if (_rootWeave && _rootWeave.looseEnds != null && _rootWeave.looseEnds.Count > 0) { - - foreach (var looseEnd in _rootWeave.looseEnds) { - - Divert looseEndDivert = looseEnd as Divert; - - if (looseEndDivert == null) continue; - - if (_finalLooseEnds == null) { - _finalLooseEnds = new List (); - _finalLooseEndTarget = Runtime.ControlCommand.NoOp (); - container.AddContent (_finalLooseEndTarget); - } - - _finalLooseEnds.Add ((Runtime.Divert)looseEndDivert.runtimeObject); - } + // CHECK FOR FINAL LOOSE ENDS! + // Notes: + // - Functions don't need to terminate - they just implicitly return + // - If return statement was found, don't continue finding warnings for missing control flow, + // since it's likely that a return statement has been used instead of a ->-> or something, + // or the writer failed to mark the knot as a function. + // - _rootWeave may be null if it's a knot that only has stitches + if (flowLevel != FlowLevel.Story && !this.isFunction && _rootWeave != null && foundReturn == null) { + _rootWeave.ValidateTermination (WarningInTermination); } return container; @@ -331,13 +335,6 @@ Parsed.Object DeepSearchForAnyLevelContent(string name) public override void ResolveReferences (Story context) { - if (_finalLooseEndTarget) { - var flowEndPath = _finalLooseEndTarget.path; - foreach (var finalLooseEndDivert in _finalLooseEnds) { - finalLooseEndDivert.targetPath = flowEndPath; - } - } - if (_startingSubFlowDivert) { _startingSubFlowDivert.targetPath = _startingSubFlowRuntime.path; } @@ -387,102 +384,19 @@ void CheckForDisallowedFunctionFlowControl() } } - void ValidateTermination() - { - // Stories don't have to explicitly terminate - // Functions don't have to terimate - they simply drop out automatically - if (this is Story || this.isFunction) - return; - - // Nothing in the main weave - probably a knot with stitches - if (_rootWeave == null) { - return; - } - - if (_rootWeave.looseEnds != null && _rootWeave.looseEnds.Count > 0) { - foreach (var looseEndObj in _rootWeave.looseEnds) { - Error ("Found loose end from weave structure", looseEndObj); - } - return; - } - - var foundReturn = Find (); - if (foundReturn != null) { - Error ("Return statements can only be used in knots that are declared as functions: == function " + this.name + " ==", foundReturn); - - // Don't continue finding warnings for missing control flow, since it's likely that a return - // statement has been used instead of a ->-> or something, or the writer failed to mark the knot as a function. - return; - } - - // Knots/stitches have to terminate in a choice, a divert, - // a conditional that contains a choice or divert. - var lastObjectInFlow = _rootWeave.lastParsedSignificantObject; - - - var terminatingDivert = lastObjectInFlow as Divert; - if (terminatingDivert) { - ValidateTerminatingDivert (terminatingDivert); - return; - } - - if (lastObjectInFlow is TunnelOnwards) { - return; - } - - if (lastObjectInFlow is Choice) { - return; - } - - // Author has left a note to self here - clearly we don't need - // to leave them with another warning since they know what they're doing. - if (lastObjectInFlow is AuthorWarning) { - return; - } - - var innerDiverts = lastObjectInFlow.FindAll (); - if (innerDiverts.Count > 0) { - var finalDivert = innerDiverts [innerDiverts.Count - 1]; - ValidateTerminatingDivert (finalDivert); - return; - } - - var innerChoices = lastObjectInFlow.FindAll (); - if (innerChoices.Count > 0) { - return; - } - - var innerTunnelOnwards = lastObjectInFlow.FindAll (); - if (innerTunnelOnwards.Count > 0) { - return; - } - - WarningInTermination (lastObjectInFlow); - } - - void ValidateTerminatingDivert(Divert terminatingDivert) - { - if (terminatingDivert.isFunctionCall) { - WarningInTermination (terminatingDivert); - return; - } - - if (terminatingDivert.isTunnel) { - WarningInTermination (terminatingDivert, "When final tunnel to '"+terminatingDivert.target+" ->' returns it won't have anywhere to go."); - } - } - - void WarningInTermination(Parsed.Object terminatingObject, string additionalExplanation = null) + void WarningInTermination(Parsed.Object terminatingObject) { string message = "Apparent loose end exists where the flow runs out. Do you need a '-> DONE' statement, choice or divert?"; - if (additionalExplanation != null) { - message = message + " " + additionalExplanation; - } - if (_firstChildFlow) { + if (terminatingObject.parent == _rootWeave && _firstChildFlow) { message = message + " Note that if you intend to enter '"+_firstChildFlow.name+"' next, you need to divert to it explicitly."; } - Warning (additionalExplanation == null ? message : message + " " + additionalExplanation, terminatingObject); + var terminatingDivert = terminatingObject as Divert; + if (terminatingDivert && terminatingDivert.isTunnel) { + message = message + " When final tunnel to '"+terminatingDivert.target+" ->' returns it won't have anywhere to go."; + } + + Warning (message, terminatingObject); } protected Dictionary subFlowsByName { @@ -505,10 +419,8 @@ public override string ToString () Weave _rootWeave; Dictionary _subFlowsByName; - List _finalLooseEnds; Runtime.Divert _startingSubFlowDivert; Runtime.Object _startingSubFlowRuntime; - Runtime.Object _finalLooseEndTarget; FlowBase _firstChildFlow; } diff --git a/compiler/ParsedHierarchy/Object.cs b/compiler/ParsedHierarchy/Object.cs index 848215e1..1087eb3e 100644 --- a/compiler/ParsedHierarchy/Object.cs +++ b/compiler/ParsedHierarchy/Object.cs @@ -223,15 +223,15 @@ public T InsertContent(int index, T subContent) where T : Parsed.Object public delegate bool FindQueryFunc(T obj); public T Find(FindQueryFunc queryFunc = null) where T : class { + var tObj = this as T; + if (tObj != null && (queryFunc == null || queryFunc (tObj) == true)) { + return tObj; + } + if (content == null) return null; foreach (var obj in content) { - var tObj = obj as T; - if (tObj != null && (queryFunc == null || queryFunc (tObj) == true)) { - return tObj; - } - var nestedResult = obj.Find (queryFunc); if (nestedResult != null) return nestedResult; diff --git a/compiler/ParsedHierarchy/Story.cs b/compiler/ParsedHierarchy/Story.cs index 1ce99de7..9aafb605 100644 --- a/compiler/ParsedHierarchy/Story.cs +++ b/compiler/ParsedHierarchy/Story.cs @@ -26,7 +26,7 @@ internal class Story : FlowBase // larger safe file, with a lot of potentially redundant counts. public bool countAllVisits = false; - public Story (List toplevelObjects) : base(null, toplevelObjects) + public Story (List toplevelObjects, bool isInclude = false) : base(null, toplevelObjects, isIncludedStory:isInclude) { // Don't do anything much on construction, leave it lightweight until // the ExportRuntime method is called. diff --git a/compiler/ParsedHierarchy/Weave.cs b/compiler/ParsedHierarchy/Weave.cs index 3929033b..a48274fd 100644 --- a/compiler/ParsedHierarchy/Weave.cs +++ b/compiler/ParsedHierarchy/Weave.cs @@ -26,7 +26,7 @@ public Runtime.Container rootContainer { // Loose ends are: // - Choices or Gathers that need to be joined up // - Explicit Divert to gather points (i.e. "->" without a target) - public List looseEnds; + public List looseEnds; public List gatherPointsToResolve; internal class GatherPointToResolve @@ -175,7 +175,7 @@ public int DetermineBaseIndentationFromContent(List contentList) public override Runtime.Object GenerateRuntimeObject () { _rootContainer = currentContainer = new Runtime.Container(); - looseEnds = new List (); + looseEnds = new List (); gatherPointsToResolve = new List (); @@ -255,7 +255,9 @@ void AddRuntimeForGather(Gather gather) } // Consume loose ends: divert them to this gather - foreach (Parsed.Object looseEnd in looseEnds) { + foreach (IWeavePoint looseEndWeavePoint in looseEnds) { + + var looseEnd = (Parsed.Object)looseEndWeavePoint; // Skip gather loose ends that are at the same level // since they'll be handled by the auto-enter code below @@ -301,7 +303,7 @@ void AddRuntimeForWeavePoint(IWeavePoint weavePoint) // Gathers that contain choices are no longer loose ends // (same as when weave points get nested content) if (previousWeavePoint is Gather) { - looseEnds.Remove ((Parsed.Object)previousWeavePoint); + looseEnds.Remove (previousWeavePoint); } // Add choice point content @@ -319,7 +321,7 @@ void AddRuntimeForWeavePoint(IWeavePoint weavePoint) // Keep track of loose ends addContentToPreviousWeavePoint = false; // default if (WeavePointHasLooseEnd (weavePoint)) { - looseEnds.Add ((Parsed.Object)weavePoint); + looseEnds.Add (weavePoint); var looseChoice = weavePoint as Choice; @@ -340,7 +342,7 @@ public void AddRuntimeForNestedWeave(Weave nestedResult) // Now there's a deeper indentation level, the previous weave point doesn't // count as a loose end (since it will have content to go to) if (previousWeavePoint != null) { - looseEnds.Remove ((Parsed.Object)previousWeavePoint); + looseEnds.Remove (previousWeavePoint); addContentToPreviousWeavePoint = false; } } @@ -373,7 +375,7 @@ void PassLooseEndsToAncestors() } } - public void ReceiveLooseEnds(List childWeaveLooseEnds) + public void ReceiveLooseEnds(List childWeaveLooseEnds) { looseEnds.AddRange (childWeaveLooseEnds); } @@ -401,6 +403,136 @@ public IWeavePoint WeavePointNamed(string name) return null; } + // Global VARs and CONSTs are treated as "outside of the flow" + // when iterating over content that follows loose ends + bool IsGlobalDeclaration (Parsed.Object obj) + { + + var varAss = obj as VariableAssignment; + if (varAss && varAss.isGlobalDeclaration && varAss.isDeclaration) + return true; + + var constDecl = obj as ConstantDeclaration; + if (constDecl) + return true; + + return false; + } + + // While analysing final loose ends, we look to see whether there + // are any diverts etc which choices etc divert from + IEnumerable ContentThatFollowsWeavePoint (IWeavePoint weavePoint) + { + var obj = (Parsed.Object)weavePoint; + + // Inner content first (e.g. for a choice) + if (obj.content != null) { + foreach (var contentObj in obj.content) { + + // Global VARs and CONSTs are treated as "outside of the flow" + if (IsGlobalDeclaration (contentObj)) continue; + + yield return contentObj; + } + } + + + var parentWeave = obj.parent as Weave; + if (parentWeave == null) { + throw new System.Exception ("Expected weave point parent to be weave?"); + } + + var weavePointIdx = parentWeave.content.IndexOf (obj); + + for (int i = weavePointIdx+1; i < parentWeave.content.Count; i++) { + var laterObj = parentWeave.content [i]; + + // Global VARs and CONSTs are treated as "outside of the flow" + if (IsGlobalDeclaration (laterObj)) continue; + + // End of the current flow + if (laterObj is IWeavePoint) + break; + + // Other weaves will be have their own loose ends + if (laterObj is Weave) + break; + + yield return laterObj; + } + } + + public delegate void BadTerminationHandler (Parsed.Object terminatingObj); + public void ValidateTermination (BadTerminationHandler badTerminationHandler) + { + // By now, any sub-weaves will have passed loose ends up to the root weave (this). + // So there are 2 possible situations: + // - There are loose ends from somewhere in the flow. + // These aren't necessarily "real" loose ends - they're weave points + // that don't connect to any lower weave points, so we just + // have to check that they terminate properly. + // - This weave is just a list of content with no actual weave points, + // so we just need to check that the list of content terminates. + + bool hasLooseEnds = looseEnds != null && looseEnds.Count > 0; + + if (hasLooseEnds) { + foreach (var looseEnd in looseEnds) { + var looseEndFlow = ContentThatFollowsWeavePoint (looseEnd); + ValidateFlowOfObjectsTerminates (looseEndFlow, (Parsed.Object)looseEnd, badTerminationHandler); + } + } + + // No loose ends... is there any inner weaving at all? + // If not, make sure the single content stream is terminated correctly + else { + + // If there's any actual weaving, assume that content is + // terminated correctly since we would've had a loose end otherwise + foreach (var obj in content) { + if (obj is IWeavePoint) return; + } + + // Straight linear flow? Check it terminates + ValidateFlowOfObjectsTerminates (content, this, badTerminationHandler); + } + } + + void ValidateFlowOfObjectsTerminates (IEnumerable objFlow, Parsed.Object defaultObj, BadTerminationHandler badTerminationHandler) + { + bool terminated = false; + Parsed.Object terminatingObj = defaultObj; + foreach (var flowObj in objFlow) { + + var divert = flowObj.Find (); + if (divert != null) { + bool divertComesBackHere = divert.isThread || divert.isTunnel || divert.isFunctionCall; + if (!divertComesBackHere) { + terminated = true; + } + } + + if (flowObj.Find () != null) { + terminated = true; + break; + } + + terminatingObj = flowObj; + } + + + if (!terminated) { + + // Author has left a note to self here - clearly we don't need + // to leave them with another warning since they know what they're doing. + if (terminatingObj is AuthorWarning) { + return; + } + + badTerminationHandler (terminatingObj); + } + } + Weave closestWeaveAncestor { get { var ancestor = this.parent; diff --git a/tests/Tests.cs b/tests/Tests.cs index ea1af6b3..442ed9fe 100644 --- a/tests/Tests.cs +++ b/tests/Tests.cs @@ -1567,6 +1567,7 @@ public void TestReadCountAcrossThreads() = aside * {false} DONE + - -> DONE "); Assert.AreEqual("1\n1\n", story.ContinueMaximally()); } @@ -1879,6 +1880,7 @@ Starting thread. == thread_with_options == * C * D +- -> DONE "); Assert.IsFalse(story.ContinueMaximally().Contains("Finished tunnel"));