Skip to content

Int maxBound causes non-integer JSON value for outline #726

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

Closed
alanz opened this issue Dec 23, 2020 · 6 comments
Closed

Int maxBound causes non-integer JSON value for outline #726

alanz opened this issue Dec 23, 2020 · 6 comments
Labels
component: ghcide type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..

Comments

@alanz
Copy link
Collaborator

alanz commented Dec 23, 2020

See emacs-lsp/lsp-mode#2435

It comes about from the following

       moduleSymbol = hsmodName >>= \case
                 (L (RealSrcSpan l) m) -> Just $
                   (defDocumentSymbol l :: DocumentSymbol)
                     { _name  = pprText m
                     , _kind  = SkFile
                     , _range = Range (Position 0 0) (Position maxBound 0) -- _ltop is 0 0 0 0
                     }

where the HsModule span does not have a reasonable value, due to the way GHC parses a module.

I think we should either limit it to something that can be represented in JSON without going to floating point, or perhaps leave out this span in the outline. Or get the end from the actual length of the file.

@pepeiborra pepeiborra transferred this issue from haskell/ghcide Dec 30, 2020
@jneira jneira added component: ghcide type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. labels Dec 30, 2020
@byorgey
Copy link
Member

byorgey commented Jun 11, 2021

This bug has been annoying me so I decided to poke at it a bit. Position contains an Int, so assuming a 64-bit architecture maxBound will be 9223372036854775807, which is the value seen in emacs-lsp/lsp-mode#2435 (but in scientific notation). But I am trying to understand why scientific notation ends up getting used. JSON itself has no inherent limit on the size of integers. aeson is being used to serialize this; as far as I can tell so far, Int gets turned into a Number Value containing a Scientific value; the fromInteger implementation for Scientific creates a value with zero exponent; and aeson's encoding for Scientific in this case just calls a bytestring-builder function to serialize an Integer. At least that's what I think should happen, but obviously I am missing something. It's as if the value is getting converted to Double and back somewhere along the way --- notice how the formatted value is missing some significant digits.

@ndmitchell
Copy link
Collaborator

Is using 1000 as the end line a pragmatic and absolutely fine solution, which doesn't require us to trip into lots of corner cases behaviours? In truth, I think 10 would probably be absolutely fine, and maybe even preferable - the "fall back" behaviour of highlighting the entire file in red squigglies makes it way harder to edit the file for real.

@alanz
Copy link
Collaborator Author

alanz commented Jun 13, 2021

And the proper solution is to fix GHC, the end position is ignored so that when reading just the header it gives a result, but that is a separate rule in the grammar, so the one parsing the whole file (which we use) could actually populate this properly.

But @ndmitchell 's comment is still valid, having a squiggle for the whole file makes the UI unwieldy.

@byorgey
Copy link
Member

byorgey commented Jun 14, 2021

If others can come to a consensus about what we would like to do here I am happy to put together a patch.

@alanz
Copy link
Collaborator Author

alanz commented Jun 14, 2021

This is for an outline, and I understand the top-level element needs to completely wrap the children. I may be wrong though.

If that is the case, a workaround would be to set the end span to the same as the end span of the last child element in the outline.

@jneira
Copy link
Member

jneira commented Jan 31, 2022

The underlying representation in lsp changed to match lsp spec one so maybe it fixed this, feel free to reopen if you continue hitting the bug
//cc @michaelpj

@jneira jneira closed this as completed Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ghcide type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..
Projects
None yet
Development

No branches or pull requests

4 participants