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

Fix parser swallowing whitespace in Src annotations #1563

Merged
merged 4 commits into from
Nov 21, 2019

Conversation

EggBaconAndSpam
Copy link
Collaborator

The parser annotated application expressions incorrectly: whitespace
between function and argument was swallowed when constructing the Src
annotation. This bug resulted in strange behaviour in the hovering
functionality in dhall-lsp-server.

As an example, previously the Dhall expression 0 0 would be parsed as

Note (Src {..., srcText = "00"})
     (App (Note (Src {..., srcText = "0"}) (NaturalLit 0))
          (Note (Src {..., srcText = "0"}) (NaturalLit 0)))

while we now get

Note (Src {..., srcText = "0 0"})
     (App (Note (Src {..., srcText = "0"}) (NaturalLit 0))
          (Note (Src {..., srcText = "0"}) (NaturalLit 0)))

as expected.

The parser annotated application expressions incorrectly: whitespace
between function and argument was swallowed when constructing the Src
annotation. This bug resulted in strange behaviour in the hovering
functionality in dhall-lsp-server.

As an example, previously the Dhall expression `0 0` would be parsed as
```
Note (Src {..., srcText = "00"})
     (App (Note (Src {..., srcText = "0"}) (NaturalLit 0))
          (Note (Src {..., srcText = "0"}) (NaturalLit 0)))
```
while we now get
```
Note (Src {..., srcText = "0 0"})
     (App (Note (Src {..., srcText = "0"}) (NaturalLit 0))
          (Note (Src {..., srcText = "0"}) (NaturalLit 0)))
```
as expected.
@EggBaconAndSpam EggBaconAndSpam changed the title Fix swallowed whitespace in Src annotations Fix parser swallowing whitespace in Src annotations Nov 20, 2019
As suggested by @sjakobi. Note that there are quite a few more
occurences of foldl where we don't actually need laziness, but I leave
that for someone else to fix :)
@sjakobi
Copy link
Collaborator

sjakobi commented Nov 20, 2019

Note that there are quite a few more
occurences of foldl where we don't actually need laziness, but I leave
that for someone else to fix :)

Can you make an issue so we don't forget about it?

@EggBaconAndSpam
Copy link
Collaborator Author

Good point ;)

@EggBaconAndSpam
Copy link
Collaborator Author

I made a separate pull request for using foldl' everywhere.

Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

Maybe we should come up with a way to test the Notes that we parse directly: Find out what properties dhall-lsp-server relies on and try to put them into property tests…

dhall/src/Dhall/Parser/Expression.hs Show resolved Hide resolved
| Note (Src left _ bytesL) _ <- a
, Note (Src _ right bytesR) _ <- b
= Note (Src left right (bytesL <> sep <> bytesR)) (App a b)
app a (_, b) =
Copy link
Collaborator

Choose a reason for hiding this comment

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

| otherwise would work too, but it doesn't matter a lot.

@EggBaconAndSpam
Copy link
Collaborator Author

Maybe we should come up with a way to test the Notes that we parse directly: Find out what properties dhall-lsp-server relies on and try to put them into property tests…

Feel free! I don't think this is as trivial as one would hope, since we really want to test all Notes inside a parsed expression; the top-level parser wraps everything in a Note anyway, so just looking at the outermost Note wouldn't be too useful.

The only way I see to go about this is to construct a 'concatenative' pretty-printer and to compare the Notes inside a parsed, pretty-printed expression against the result of pretty-printing the sub-expressions. The problem is that our pretty-printer isn't concatenative...

@mergify mergify bot merged commit a41be1a into master Nov 21, 2019
@mergify mergify bot deleted the frederik/fix-parser-swallowing-whitespace branch November 21, 2019 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants