Upgrade to new version of lsp libraries

[michaelpj]

Don't look here (except @jneira), just for CI.

This branch has the new version pulled in by source-repository-package. We could merge it that way, or we could wait until we do a lsp release, which is desirable anyway.

[michaelpj]

I'll squash all these and tidy up before merging, once this works.

[jneira]

I would release LSP and changing this to use it before merge, as final sanity check over the released version (the bump of hackage index could change the behaviour)

[jneira]

wow

Exception: 

Row number out of bounds: 2644896934/0 

CallStack (from HasCallStack): 

error, called at test/exe/Main.hs:6383:26 in main:Main 

Exception thrown while showing test case: 

Row number out of bounds: 2644896934/0 

CallStack (from HasCallStack): 

error, called at test/exe/Main.hs:6383:26 in main:Main

[jneira]

2644896934

Obviusly that exceeds the max bound for Word32/Int32: 2147483647

[michaelpj]

Exciting.

[jneira]

It is due to quickcheck using arbitrary instances for a numeric type bigger than Word32:

haskell-language-server/ghcide/test/exe/Main.hs

Lines 6314 to 6344 in bb73e32

	, adjustOption (\(QuickCheckTests i) -> QuickCheckTests (max 1000 i)) $ testGroup "properties"
	[ testProperty "fromCurrent r t <=< toCurrent r t" $ do
	-- Note that it is important to use suchThatMap on all values at once
	-- instead of only using it on the position. Otherwise you can get
	-- into situations where there is no position that can be mapped back
	-- for the edit which will result in QuickCheck looping forever.
	let gen = do
	rope <- genRope
	range <- genRange rope
	PrintableText replacement <- arbitrary
	oldPos <- genPosition rope
	pure (range, replacement, oldPos)
	forAll
	(suchThatMap gen
	(\(range, replacement, oldPos) -> positionResultToMaybe $ (range, replacement, oldPos,) <$> toCurrent range replacement oldPos)) $
	\(range, replacement, oldPos, newPos) ->
	fromCurrent range replacement newPos === PositionExact oldPos
	, testProperty "toCurrent r t <=< fromCurrent r t" $ do
	let gen = do
	rope <- genRope
	range <- genRange rope
	PrintableText replacement <- arbitrary
	let newRope = applyChange rope (TextDocumentContentChangeEvent (Just range) Nothing replacement)
	newPos <- genPosition newRope
	pure (range, replacement, newPos)
	forAll
	(suchThatMap gen
	(\(range, replacement, newPos) -> positionResultToMaybe $ (range, replacement, newPos,) <$> fromCurrent range replacement newPos)) $
	\(range, replacement, newPos, oldPos) ->
	toCurrent range replacement oldPos === PositionExact newPos
	]

[jneira]

I think i have a fix

[jneira]

Why do you want runtime errors from dynamic types if you can have them with pesky type classes? :-P

[michaelpj]

Hang on, I'm fixing a bunch of these.

[michaelpj]

I did a different fix, oops 😅

[michaelpj]

Releasing the lock for a bit now!

Honestly, the fromIntegral :: Int -> Word32 conversions are kind of dodgy. Maybe we should use some function that checks, or rounds to 0 or something.

[michaelpj]

I also wonder about having an IntegralPosition pattern synonym or something.

[jneira]

Releasing the lock for a bit now!

Honestly, the fromIntegral :: Int -> Word32 conversions are kind of dodgy. Maybe we should use some function that checks, or rounds to 0 or something.

well i checked manually the inputs for nthLine to ensure they were always in (0, maxBound :: Word32) and not sure what would be the sensible option, i guess we will want round for some and error for others 🤷

[michaelpj]

It's not nthLine that I'm worried about. e.g there was a test that maybe some CPP preprocessor errors come back from GHC with column -1, which just wraps 😱 There are lots of conversions everywhere we make a Postion...

[jneira]

Ugh so ghc can return negative values as i vaguely remebered 🤦

[michaelpj]

In some ways this is good though: I believe technically a lsp client could simply have refused to parse a message where the position had a negative value, now we're forced to deal with it earlier.

[michaelpj]

Tests seem to pass... windows builds hanging (?) as usual.

[jneira]

the stack build is failing due to unused imports:

haskell-language-server > /root/build/test/functional/TypeDefinition.hs:7:1: error: [-Wunused-imports, -Werror=unused-imports] haskell-language-server > The import of ‘Data.Word’ is redundant haskell-language-server > except perhaps to import instances from ‘Data.Word’ haskell-language-server > To import instances alone, use: import Data.Word() haskell-language-server > | haskell-language-server > 7 | import Data.Word haskell-language-server > | ^^^^^^^^^^^^^^^^ haskell-language-server > -- While building package haskell-language-server-1.5.1.0 (scroll up to its section to see the error) using: /root/.stack/setup-exe-cache/x86_64-linux/Cabal-simple_mPHDZzAJ_3.2.1.0_ghc-8.10.6 --builddir=.stack-work/dist/x86_64-linux/Cabal-3.2.1.0 build lib:haskell-language-server exe:haskell-language-server exe:haskell-language-server-wrapper test:func-test test:wrapper-test --ghc-options " -fdiagnostics-color=always" Process exited with code: ExitFailure 1 Exited with code exit status 1 

[michaelpj]

We fail on unused imports in some builds but not others? oy vey.

[jneira]

ya inconsistent, I would make cabal ones stricter

[michaelpj]

This will currently wrap. Should we do something else here? Like take max 0 first?

[jneira]

yeah not sure if we have another sensible alternative?

[michaelpj]

Not really. I think either let it wrap or max with 0.

[michaelpj]

Uh oh. I think this means I got it wrong: uinteger according to the LSP specification is not, in fact a 32-bit unsigned integer, but a 31-bit unsigned integer. Presumably because it's actually a 32-bit signed integer that they just restrict to be positive. Terrible. I don't even know what to do about that.

[michaelpj]

I made them an issue: microsoft/language-server-protocol#1394

But in the short term this kind of sucks...

[michaelpj]

I guess I can go back and use a custom type instead of Word32 in lsp, sigh.

[jneira]

Ouch, but we can go ahead with this lsp version? or you want to redefine the type in lso before the release
I guess we can continue using the hardcoded maxBound?

[michaelpj]

It would be nice not to release a version of lsp that disruptively requires people to change their code and then doesn't even fix the problem :(

I think the best thing might be to change to a custom type in lsp indeed. It shouldn't affect this PR too much. Either that or we should just revert the change in lsp.

[jneira]

hmm but if we use UInt31 in LSP with all the required intances, including from integral, would no be matter of replacing Word32 with UInt31 here?

[michaelpj]

Yes, that's basically it. I just need to do it. I'll try and find time tomorrow.

[jneira]

maybe with a more generic name like LSPUInt? not sure if the data type could even change in the spec but I can imagine it could be bigger

[michaelpj]

haskell/lsp#382

[jneira]

So this needs the change to use the new UInt31 from lsp

[michaelpj]

Yes, and I was going to try and remove the dependent-map dependency from lsp and then we could do a release.

[Bodigrim]

Yes, and I was going to try and remove the dependent-map dependency from lsp and then we could do a release.

haskell/lsp#384 awaits for review.

[jneira]

I have the stack.yamls changed to use a recent commit and they works here #2551

[jneira]

lgtm many thanks for working on this

[jneira]

9.0.1 is not picking the new index state

Warning: Requested index-state 2021-12-29T12:30:08Z is newer than 

'hackage.haskell.org'! Falling back to older state (2021-12-24T16:25:00Z).

🤔

[michaelpj]

Yeah, it seems like it's not calling cabal update and I don't know why. #2536 removed a bunch of cabal update calls which seems suspicious.

[jneira]

ugh we removed it cause the Haskell action seemed to do it, but maybe not in all cases
/cc @Anton-Latukha

[jneira]

hmm, is it related with the lsp changes? maybe with the drop of dependent-sum?

src/Ide/Types.hs:302:37: error:
Error:     • Could not deduce (GEq SMethod) arising from a use of ‘geq’
      from the context: PluginMethod a
        bound by a pattern with constructor:
                   IdeMethod :: forall (m :: Method 'FromClient 'Request).
                                PluginMethod m =>
                                SMethod m -> IdeMethod m,
                 in an equation for ‘geq’
        at src/Ide/Types.hs:302:8-18
      or from: PluginMethod b
        bound by a pattern with constructor:
                   IdeMethod :: forall (m :: Method 'FromClient 'Request).
                                PluginMethod m =>
                                SMethod m -> IdeMethod m,
                 in an equation for ‘geq’
        at src/Ide/Types.hs:302:22-32
    • In the expression: geq a b
      In an equation for ‘geq’: geq (IdeMethod a) (IdeMethod b) = geq a b
      In the instance declaration for ‘GEq IdeMethod’
    |
302 |   geq (IdeMethod a) (IdeMethod b) = geq a b
    |                                     ^^^^^^^

[Bodigrim]

@jneira try cabal update now?

[Anton-Latukha]

#2494 (comment)

Yes.

I pretty much was wondering when Action particularly runs update. But there is too much layers I do not know enough info to say particularly. If to believe if switch in https://github.com/haskell/actions/blob/a64ac8cca0d4aec4e3fd4a5fbc9ae3c4953e20aa/setup/src/setup-haskell.ts#L75, if (!opts.stack.enable) await exec('cabal update'); implies it runs when stack is not in use & we declare that explicitly. But Action also does not say anything when it updates, does not produces the index-state information.
Then Action code & process as a whole, I know GitHub Actions infrastructure does some caching, but it is not known (for proprietary reasons) when & how. But also index updates happen rarely & it is rare for index-state needed to be required which is after update. So was wondering is it safe to remove additional update or not, but in complex cases with have small consequences I tend to believe the code. And I honestly did not had another way to model & test all this process but to try it, so that is on me.

Also after all this only today noted that Action does not use the latest ghcup, so I do not know enough about how haskell/actions/setup works, and it also seems to stay unmaintained.

We also had the typo that borked (which is seen in) the freeze caching key structure #2549 (comment) (but thankfully we made so everything falls-back into the simpler procedure in that case).

In any case, if there is now some kind of error - it probably may be the rm of update & I do not have particular feels about it the other way, just wanted to shave a step from the config & the CI runs. Sorry for the inconvinience.

[jneira]

@jneira try cabal update now?

after merging master CI succeeded, not sure why (have to take a deeper look)