search

stchang / parsack

A basic Parsec-like monadic parser combinator library implementation in Racket.
MIT License
50 stars 10 forks source link

`string` parse fail error msg has wrong "unexpected" on partial parse #23

Closed stchang closed 10 years ago

stchang commented 10 years ago 
(parse (string "ab") "ac")

=>

parse ERROR: at 1:2:2
unexpected: ""
expected: "b"

unexpected should be "c"

greghendershott commented 10 years ago

Hmm did I bork this when I added stringAnyCase in commit 939e1a35, here or was it wrong previously?

Quick glance, I don't see how it was my change, but if so of course you can assign it to me to fix unless you'd prefer handling it yourself.

stchang commented 10 years ago

No I think it's always been broken. I will fix.

stchang commented 10 years ago

By the way I noticed that the order of fields in Pos is offset-line-col but when throwing an error it's line-col-offset. This makes things somewhat confusing when trying to read the result of parse vs an error message. Would it be terrible for you if I changed one of them to stay consistent? I don't have a preference.

greghendershott commented 10 years ago

Yeah sorry about that. Because it was originally just an offset position, I figured it was less confusing if I made (Pos ofs line col) because I was "extending" it. And ofs is "primary" (which is why only it sufficed before), the others are only "derived" and only used for error messages.

So fine, but as I wrote unit tests I noticed what you mentioned: the messagea are line:col:ofs. And to add to the cognitive load, add1 each of them. Yeah.

line:col:ofs seems to be the convention for tools, and emacs and vim can go-to-error on those. So, maybe the one to change is the Pos struct?

stchang commented 10 years ago

Ok so I will change Pos. Good point on 1- vs 0-based as well. Would you mind if I changed the numbers in Pos to be 1-based to match?

It's not a problem. I appreciate the improvements. I just wanted to make sure I wouldn't break too much of your stuff.

greghendershott commented 10 years ago

On Tuesday, November 12, 2013, stchang wrote:

Ok so I will change Pos. Good point on 1- vs 0-based as well. Would you mind if I changed the numbers in Pos to be 1-based to match?

Oh, sure. Old C guy just assumed they needed to be zero based. But I guess even ofs is only used for error display? Could be 13-based and parse still work, just evil error messages. :)

stchang commented 10 years ago

I fixed this, as well as the error messages for the "lookahead" combinators (<!> and notFollowedBy). I ran your markdown tests and everything passes (I think?) but please let me know if I broke anything.

(still need to change the offset-line-col ordering)

greghendershott commented 10 years ago

About half an hour ago I got an issue. I think they happened to get the very latest parsack with this commit. After I pull it and try to run the markdown tests, I'm getting what looks like the same contract violation on list->string (just on different input):

greg@mbp in ~/src/racket/collects/markdown/markdown on master
$ raco test -x test.rkt 
raco test: (submod "test.rkt" test)
list->string: contract violation
  expected: (listof char?)
  given: '(strong () "bold")
  context...:
   /Users/greg/src/scheme/collects/parsack/parsack/parsack.rkt:214:2
   /Users/greg/src/scheme/collects/parsack/parsack/parsack.rkt:128:2
   /Users/greg/src/scheme/collects/parsack/parsack/parsack.rkt:128:2
   /Users/greg/src/scheme/collects/parsack/parsack/parsack.rkt:128:2
   /Users/greg/src/scheme/collects/parsack/parsack/parsack.rkt:152:2
   /Users/greg/src/scheme/collects/parsack/parsack/parsack.rkt:128:2
   /Users/greg/src/scheme/collects/parsack/parsack/parsack.rkt:140:8: .../parsack/parsack.rkt:140:8
   /Applications/Racket_v5.3.5/collects/racket/private/promise.rkt:53:8
   /Applications/Racket_v5.3.5/collects/racket/private/more-scheme.rkt:263:2: call-with-exception-handler
   /Users/greg/src/scheme/collects/parsack/parsack/parsack.rkt:140:8: .../parsack/parsack.rkt:140:8
   /Applications/Racket_v5.3.5/collects/racket/private/promise.rkt:55:10: loop
   /Applications/Racket_v5.3.5/collects/racket/private/more-scheme.rkt:263:2: call-with-exception-handler
   /Users/greg/src/scheme/collects/parsack/parsack/parsack.rkt:140:8: .../parsack/parsack.rkt:140:8
   /Applications/Racket_v5.3.5/collects/racket/private/promise.rkt:55:10: loop
   /Applications/Racket_v5.3.5/collects/racket/private/more-scheme.rkt:263:2: call-with-exception-handler
   /Users/greg/src/scheme/collects/parsack/parsack/parsack.rkt:140:8: .../parsack/parsack.rkt:140:8...

I'm digging in now to see what I can figure out, but wanted to give you a heads-up...

greghendershott commented 10 years ago

It looks like the implementations of <!> and notFollowedBy changed pretty significantly. Although I don't think I'm using <!>, I'm definitely using notFollowedBy.

The original pattern was:

(try (<or> (parser-compose (c <- (try p))
                            (unexpected c))
            (return null))))

Maybe the new versions are equivalent at a deeper level than I can understand right now, but, I think probably not?

greghendershott commented 10 years ago

Oh, I think it might be simpler, that the new result->str assumes that a list? is a (listof char?) but it could be another list, in my case an xexpr like (strong () "bold").

I just made a branch and will work on a fix...

stchang commented 10 years ago

Dammit sorry. I'm on it too. The functionality should be the same just with better errors.

I did change it so the errors always report things in terms of strings so see if that fixes things.

greghendershott commented 10 years ago

I just made a PR. Please note the two commits. The first one passes the tests, but is too slow. The second one doesn't pass the tests (the messages aren't formatted nicely) but is fast-enough and passes all of Parsack's tests as well as the downstream markdown parser tests.