Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

keep track of the longest failed match to improve error messages #269

Merged
merged 6 commits into from
Feb 26, 2020
Merged

keep track of the longest failed match to improve error messages #269

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d4ce46b
keep track of the longest failed match to improve error messages
skoppe Jan 19, 2020
f8163be
Fix wording, remove commented out code
skoppe Jan 21, 2020
014394f
Add unittests for failedChild
skoppe Jan 21, 2020
37469f6
Fix several bugs/enchancements
skoppe Feb 3, 2020
9add4b5
Fix unicode bug and fix decimateTree
skoppe Feb 5, 2020
c6d1a03
fix expected string length bug
skoppe Feb 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 61 additions & 8 deletions pegged/peg.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Writing tests the long way is preferred here, as it will avoid the circular
dependency.
*/

import std.algorithm: equal, map, startsWith;
import std.algorithm: equal, map, startsWith, max, countUntil;
import std.uni : isAlpha, icmp;
import std.array;
import std.conv;
Expand Down Expand Up @@ -248,6 +248,9 @@ struct ParseTree
ParseTree[] children; /// The sub-trees created by sub-rules parsing.
size_t failEnd; // The furthest this tree could match the input (including !successful rules).
ParseTree[] failedChild; /// The !successful child that could still be partially parsed.
/**
Basic toString for easy pretty-printing.
*/
Expand Down Expand Up @@ -343,6 +346,8 @@ struct ParseTree
result.input = input;
result.begin = begin;
result.end = end;
result.failEnd = failEnd;
result.failedChild = map!(p => p.dup)(failedChild).array();
result.children = map!(p => p.dup)(children).array();
return result;
}
Expand Down Expand Up @@ -719,7 +724,7 @@ template literal(string s)
if (p.end+s.length <= p.input.length && p.input[p.end..p.end+s.length] == s)
return ParseTree(name, true, [s], p.input, p.end, p.end+s.length);
else
return ParseTree(name, false, [lit], p.input, p.end, p.end);
return ParseTree(name, false, [lit], p.input, p.end, p.end, null, p.end);
}
ParseTree literal(string input)
Expand Down Expand Up @@ -1262,7 +1267,8 @@ template and(rules...) if (rules.length > 0)
//&& !node.name.startsWith("drop!(")
&& node.matches !is null
//&& node.begin != node.end
);
)
|| (node.failEnd > node.end);
}
version (tracer)
Expand All @@ -1281,6 +1287,7 @@ template and(rules...) if (rules.length > 0)
}
ParseTree temp = r(result);
result.end = temp.end;
result.failEnd = max(result.failEnd, temp.failEnd);
if (temp.successful)
{
if (keepNode(temp))
Expand All @@ -1296,9 +1303,17 @@ template and(rules...) if (rules.length > 0)
}
else
{
result.children ~= temp;// add the failed node, to indicate which failed
if (temp.matches.length > 0)
result.matches ~= temp.matches[$-1];
auto firstLongestFailedMatch = result.children.countUntil!(c => c.failEnd > temp.end);
if (firstLongestFailedMatch == -1) {
result.children ~= temp;// add the failed node, to indicate which failed
// result.end = temp.end;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you don't mean to commit this line that is commented out.

if (temp.matches.length > 0)
result.matches ~= temp.matches[$-1];
} else {
// don't add the failed node because a previous one already failed further back
result.children = result.children[0 .. firstLongestFailedMatch+1]; // discard any intermediate correct nodes
failedChildFixup(result.children[firstLongestFailedMatch]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe explain what needs fixing up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope this is clearer now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

}
version (tracer)
{
if (shouldTrace(getName!(r)(), p))
Expand Down Expand Up @@ -1333,6 +1348,29 @@ template and(rules...) if (rules.length > 0)
}
}
// This function rewrites the ParseTree to move the failedChild into its parents children
// this is done whenever an 'and' rule fails and there are such a child further down
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"there are such a child" = "there is such a child"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a nested function inside and(), to clarify that it is only called from there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially misunderstood what this function does. The way this is documented, it seemed like a descendant was being pulled up the ancestry tree to be inserted higher up. That would be very strange, of course.

bool failedChildFixup(ref ParseTree p) {
if (p.failedChild.length > 0) {
p.children ~= p.failedChild[0];
p.failedChild = [];
p.successful = false;
p.end = p.failEnd;
p.failEnd = 0;
return true;
} else {
foreach(ref c; p.children) {
if (failedChildFixup(c)) {
p.end = c.end;
p.successful = false;
p.failEnd = 0;
return true;
}
}
}
return false;
}
unittest // 'and' unit test
{
alias literal!"abc" abc;
Expand Down Expand Up @@ -1524,6 +1562,11 @@ template or(rules...) if (rules.length > 0)
{
temp.children = [temp];
temp.name = name;
// if there is a child that failed but parsed more
if (longestFail.failEnd > temp.end) {
temp.failEnd = longestFail.failEnd;
temp.failedChild = [longestFail];
}
version (tracer)
{
if (shouldTrace(getName!(r)(), p))
Expand Down Expand Up @@ -2166,13 +2209,18 @@ template zeroOrMore(alias r)
result.matches ~= temp.matches;
result.children ~= temp;
result.end = temp.end;
result.failEnd = max(result.failEnd, temp.failEnd);
version (tracer)
{
if (shouldTrace(getName!(r)(), p))
trace(traceMsg(result, name, getName!(r)()));
}
temp = r(result);
}
if (temp.end > result.failEnd) {
result.failedChild = [temp];
result.failEnd = temp.end;
}
result.successful = true;
version (tracer)
{
Expand Down Expand Up @@ -2328,13 +2376,18 @@ template oneOrMore(alias r)
result.matches ~= temp.matches;
result.children ~= temp;
result.end = temp.end;
result.failEnd = max(result.failEnd, temp.failEnd);
version (tracer)
{
if (shouldTrace(getName!(r)(), p))
trace(traceMsg(result, name, getName!(r)()));
}
temp = r(result);
}
if (temp.end > result.failEnd) {
result.failedChild = [temp];
result.failEnd = temp.end;
}
result.successful = true;
}
version (tracer)
Expand Down Expand Up @@ -2451,9 +2504,9 @@ template option(alias r)
}
ParseTree result = r(p);
if (result.successful)
return ParseTree(name, true, result.matches, result.input, result.begin, result.end, [result]);
return ParseTree(name, true, result.matches, result.input, result.begin, result.end, [result], result.failEnd);
else
return ParseTree(name, true, [], p.input, p.end, p.end, null);
return ParseTree(name, true, [], p.input, p.end, p.end, null, result.failEnd, [result]);
}
ParseTree option(string input)
Expand Down