Replace a lot of uses of expect with tokenCheck. #403
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #403 +/- ##
==========================================
- Coverage 83.71% 83.59% -0.12%
==========================================
Files 11 11
Lines 8540 8478 -62
==========================================
- Hits 7149 7087 -62
Misses 1391 1391
Continue to review full report in Codecov by Sentry.
|
|
wouldn't this remove a lot from the current error recovery? For example in line 839 it would now return null for the entire AsmStatement because of a nearly complete AsmInstruction just missing a semicolon. |
|
I think that we should replace that accidental error recovery with intentional error recovery. If we're parsing a list of things, each of the element functions should return null if they fail, and the parsing function for the list should handle creating an incomplete list. An example of this is the |
|
maybe we should add more tests for the error recovery though to avoid confusing users with libdparse updates or tools offering automatic fixes which depend on certain errors being present. I'm not a fan of so many |
|
Do you know what kind of behavior resulting from these mistakes that we'd like to preserve? |
WebFreak001
reviewed
May 5, 2020
There was a problem hiding this comment.
I have commented where I think we have rather small breakages from the previous version. Error recovery wasn't very good but now at these parts it's slightly worse.
For good error recovery we probably want a separate PR doing better recovery from semicolons, but for this PR I think the marked lines shouldn't be changed.
Test script
#!/bin/bash
echo "input:"
echo '```d'
INPUT=$(cat)
echo '```'
echo
echo "AST before:"
echo '```d'
echo $INPUT | ./testrecovery-old
echo '```'
echo
echo "AST after:"
echo '```d'
echo $INPUT | ./testrecovery
echo '```'Test App
#!/usr/bin/env dub
/+ dub.sdl:
name "testrecovery"
dependency "libdparse" path="."
+/
import std;
import dparse.ast;
import dparse.formatter;
import dparse.lexer;
import dparse.parser;
import dparse.rollback_allocator;
void main()
{
auto code = appender!string;
foreach (c; stdin.byChunk(4096))
code.put(c);
StringCache cache = StringCache(64);
LexerConfig config;
auto tok = getTokensForParser(cast(ubyte[])code.data, config, &cache);
auto mod = parseModule(tok, "stdin", new RollbackAllocator());
auto output = appender!string;
format(output, mod, true);
writeln(output.data);
}| @@ -3064,7 +3064,7 @@ class Parser | |||
| else | |||
| { | |||
| mixin(parseNodeQ!(`node.test`, `Expression`)); | |||
| expect(tok!";"); | |||
| mixin(tokenCheck!";"); | |||
There was a problem hiding this comment.
input:
void foo()
{
for (; i < 3)
{}
}AST before:
stdin(1:26)[error]: Expected `;` instead of `)`
void foo()do
{
for (; i < 3; ) {}
}AST after:
stdin(1:26)[error]: Expected `;` instead of `)`
stdin(1:26)[error]: Primary expression expected
void foo()do
{ {}
}
| @@ -4534,7 +4534,7 @@ class Parser | |||
| error("`(` or identifier expected"); | |||
| return null; | |||
| } | |||
| expect(tok!";"); | |||
| mixin(tokenCheck!";"); | |||
There was a problem hiding this comment.
input:
mixin Something
void main()
{
}AST before:
stdin(1:17)[error]: Expected `;` instead of `void`
stdin(1:29)[error]: declaration expected instead of `{`
mixin Something ;AST after:
stdin(1:17)[error]: Expected `;` instead of `void`
stdin(1:29)[error]: declaration expected instead of `{`
src/dparse/parser.d
Outdated
| mixin(parseNodeQ!(`node.expression`, `Expression`)); | ||
| expect(tok!";"); | ||
| mixin(tokenCheck!";"); |
There was a problem hiding this comment.
input:
void main()
{
throw new Exception()
writeln("hello");
}AST before:
stdin(1:37)[error]: Expected `;` instead of `writeln`
stdin(1:53)[warn]: Empty declaration
void main()do
{
throw new Exception();
}AST after:
stdin(1:37)[error]: Expected `;` instead of `writeln`
stdin(1:53)[warn]: Empty declaration
void main()do
{
}There was a problem hiding this comment.
no surprise here, tokenCheck returns early and drop the invalid node
| @@ -7561,7 +7561,7 @@ class Parser | |||
| return null; | |||
| } | |||
| node.token = advance(); | |||
| expect(tok!";"); | |||
| mixin(tokenCheck!";"); | |||
There was a problem hiding this comment.
input:
version = 5
void main()
{
}AST before:
stdin(1:13)[error]: Expected `;` instead of `void`
stdin(1:25)[error]: declaration expected instead of `{`
version = 5;AST after:
stdin(1:13)[error]: Expected `;` instead of `void`
stdin(1:25)[error]: declaration expected instead of `{`
| StackBuffer instructions; | ||
| while (moreTokens() && !currentIs(tok!"}")) | ||
| { | ||
| auto c = allocator.setCheckpoint(); | ||
| if (!instructions.put(parseAsmInstruction())) | ||
| allocator.rollback(c); | ||
| else | ||
| expect(tok!";"); | ||
| mixin(tokenCheck!";"); |
There was a problem hiding this comment.
this is probably a big change, but the dparse.formatter doesn't support dumping ASM so I can't easily test it.
|
Most of these are consequences of the way that the |
ghost
mentioned this pull request
…parsing functions to correctly return null when a token is missing.
|
DCD BUILD FAILED Build statistics: ------ libdparse statistics ------
statistics (-before, +after)
-library size=3544196 libdparse.a
+library size=3544684 libdparse.a
rough build time=17s
------ DCD statistics ------
statistics (-before, +after)
client size=1055832 bin/dcd-client
server size=3052720 bin/dcd-server
-rough build time=80s
+rough build time=81s
-DCD run_tests.sh Elapsed (wall clock) time (h:mm:ss or m:ss): 0:05.92
-DCD run_tests.sh Maximum resident set size (kbytes): 7896
+DCD run_tests.sh Elapsed (wall clock) time (h:mm:ss or m:ss): 0:05.91
+DCD run_tests.sh Maximum resident set size (kbytes): 7904
short requests: (217x)
min request time = 0.013ms
- 10th percentile = 0.129ms
- median time = 0.481ms
- 90th percentile = 0.820ms
- max request time = 1.642ms
+ 10th percentile = 0.128ms
+ median time = 0.465ms
+ 90th percentile = 0.797ms
+ max request time = 1.448ms
top 5 GC sources in server:Full build output |
|
DCD build failure is due to https://issues.dlang.org/show_bug.cgi?id=23874 |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will cause the parsing functions to correctly return null when a token is missing. Fixes #392 and other similar issues that have not yet been discovered.