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.

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

Better compile errors #467

Merged
merged 5 commits into from Apr 15, 2019

Conversation

Projects
None yet
3 participants
@slpopejoy
Copy link
Contributor

commented Apr 13, 2019

  • Non-insane handling of error results in runCompile
  • tokenErr' is main change, to insert label as main error item instead of token
  • suppression of meta errors (not finding a meta resulted in "Expected: literal, Expected: atom" that wouldn't get cleared out)
  • possibly extraneous commit add in reservedAtom that does no harm
  • better interactive test functions in Compile (ie, _compileStrWith allows running an individual Compile parser)

@slpopejoy slpopejoy requested review from fosskers and joelburget Apr 13, 2019

@joelburget

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

Re suppression of meta errors -- I don't understand the fix here. I guess I would expect adding a label to meta.

I think the changes in ExpParser look good.

Show resolved Hide resolved src/Pact/Compile.hs Outdated
@slpopejoy

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

@joelburget The silencing was because I was bothered that meta failures show up with an unrelated failure:

_compileStr "(module f 'k (bless \"abc!\"))"
*** Exception: user error ("abc!": Failure: Syntax error: bad hash: 
Base16 decode failed: "abc!", docstr, meta-pairs)

(Note that this is with labels in meta, ie label "meta-pairs" atPairs <|> label "docstr" (try docStr) <|> return def). My goal in silencing them was to just show the error on the bless hash. Here's what it is now:

λ> _compileStr "(module f 'k (bless \"abc!\"))"
*** Exception: user error ("abc!": Failure: Syntax error: bad hash: 
Base16 decode failed: "abc!")

However, in responding to you here, I realized that maybe the reason the labels aren't vanishing is because the failed bless is in the same position, which led me to try a failed form in the second position, and sadly it looks like I have more work to do:

λ> _compileStr "(module f 'k (use foo) (bless \"!\"))"
*** Exception: user error ((bless "!"): Failure: Syntax error: Unexpected end of input)

But in any case, I still see those meta errors as a little confusing. We can go with the labels if we think that's more "correct". Note however that the silencing doesn't stop a bad label from reporting correctly:

λ> _compileStr "(module f 'k @meta foobar)"
*** Exception: user error (@meta : Failure: Syntax error: Expected: list)

What's your opinion on including the labels given the above? I probably prefer the silencing but it only affects errors on the first form, so could go either way.
EDIT: I think until we figure this out further we need to silence them, it's just too confusing:

λ> _compileStr "(module f 'k (deftable \"sdfjh\"))"
*** Exception: user error ("sdfjh": Failure: Syntax error: Expected: atom, docstr, meta-pairs)

That's just bizarre.

@slpopejoy

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

Phew!! Figured out a major problem which is that some is inappropriate for body forms, due to its use of optional -- which causes backtracking on errors, which would then trip <* eof in sexp far above. Fix was to peek forward in stream and deterministically parse all forms found. Finally decent errors!

@joelburget
Copy link
Contributor

left a comment

I'm guessing there are more improvements to be had here, but this is a good start.

@slpopejoy slpopejoy merged commit e3b65eb into master Apr 15, 2019

3 checks passed

ci/gitlab/gitlab.com Pipeline passed on GitLab
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@slpopejoy slpopejoy deleted the feat/better-compile-errors branch Apr 15, 2019

bts added a commit that referenced this pull request Apr 26, 2019

Merge remote-tracking branch 'origin/master' into analyze-yield-resume
* origin/master:
  add warning at top (#477)
  Fix `img` symlinks [skip ci] (#478)
  Revert "Fix Doc Symlinks (#476)"
  Fix Doc Symlinks (#476)
  Add note about bad z3 version to the readme. (#474)
  JSON serialization of Term/Scope (#473)
  Non-malleability: signatories and chain version in payload  (#470)
  Shorter hashes (#471)
  Fix run.sh -- application type with curl, syntax fix, hashes (#472)
  Better compile errors (#467)
  links -- downloads, pactlang.org tutorials, pactweb (#468)
  History persist test (#465)
  Fixed select to match insert and schema. (#463)
  PactOutput type for reliable Term output, yields (#460)
  Remove duplicate definition of `read-cp-master`. (#447)
  Disallow rollbacks on the last step of a pact. (#448)
  Tweak order of docs for natives. (#454)
  Fix pretty-printing of strings (#441)

@LindaOrtega LindaOrtega added Pact 3.0 and removed Pact 3.0 labels May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.