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
Breaking change in parser (probably related to GPR#1064 - Extended indexing operators) #7637
Comments
Comment author: @alainfrisch This is indeed an unfortunate side effect of #1064. This PR adds support for things like "a.?(i)", but it does so by considering that ".?" is a token, thus breaking cases where the "DOTOP"" is not followed by (, { or [. I can see two directions:
2 is simpler and opens the door to other uses of the current DOTOP token in the grammar. My preference goes to it. |
Comment author: @gasche I would also think of 2. as the best way forward, but I think we should evaluate the extent of the compatibility breakage on OPAM. |
Comment author: @mshinwell In general, I don't like enforcing whitespace conventions in the language, but given the situation we find ourselves in I wouldn't be opposed to one here. Morally speaking, there should be a space after that dot, IMO. |
Comment author: @damiendoligez I'm running a test against OPAM now I'll report when it's done (probably tomorrow). |
Comment author: @Octachron In term of tests, I was thinking that testing "4.04.2+lexer changes" might be simpler: https://github.com/Octachron/ocaml/tree/indexop-test . |
Comment author: @Octachron After a little bit of testing on my side with the above compiler patch over a subset of 1453(/1704) opam packages; I could only detect one syntax error failure, in the earley package. Note that the missing 221 opam package were either not installable on version 4.04.2 (for 78 packages), missing hard to automate external dependencies (for 74 packages) or failed to build due to another kind of errors (for 69 packages), see https://gist.github.com/Octachron/e9691b3ea1c43463730349f806894050 for the full list. |
Comment author: @gasche I'm interested in the testing methodology, did you reuse Damien's opam-testing script or something else? |
Comment author: @damiendoligez Same here, earley is the only package that breaks between before and after the merge of #1064. I got a strange error with gasoline.0.4.0, but I don't think it's related (and there is a newer version of gasoline anyway). |
Comment author: @Octachron Since I initially wanted to test few packages, I opted for a small hand written script: https://gist.github.com/Octachron/4c196486df168e2a0b68e7621e45c018 . |
Comment author: ChriChri The next version of earley will have no problem with that... |
Comment author: @gasche Let's suppose that this breakage is not going to change (I updated the issue status accordingly). Do we need to document it better? |
Comment author: @Octachron I think that we need to mark the change as breaking at the very least: |
Original bug ID: 7637
Reporter: @nojb
Assigned to: @gasche
Status: resolved (set by @gasche on 2017-10-19T15:08:15Z)
Resolution: won't fix
Priority: high
Severity: minor
Target version: 4.06.0 +dev/beta1/beta2/rc1
Fixed in version: 4.06.0 +dev/beta1/beta2/rc1
Category: lexing and parsing
Bug description
4.05 accepted
type t = < f: 'a.?foo:'a -> unit >
but 4.06 requires a space between '.' and '?':
type t = < f: 'a. ?foo:'a -> unit >
The text was updated successfully, but these errors were encountered: