Limit the number of valid hole fits

[expipiplus1]

Originally haskell/ghcide#888

[jneira]

@pepeiborra commented in the original pr

I think the limit (be it 5 or 15) should be moved to the GHC flags that we set in GHC.Compat, and make sure we don't override a user value.

I think we should take in account the ghc flag in some way, at least to honour it if the user has set it on purpose.
I would be a little bit surprised if i set 20 and only see 10 in the editor.

So i think that maybe we should follow the recommendation and set the flag auto for the specific ghc session for hls if users didnt set it explicitly.

[pepeiborra]

Thanks Joe!

[Ailrun]

@expipiplus1 Tests from

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

Lines 2433 to 2458 in b970e25

	, testSession "postfix hole uses postfix notation of infix operator" $ do
	let mkDoc x = T.unlines
	[ "module Testing where"
	, "test :: Int -> Int -> Int"
	, "test a1 a2 = " <> x <> " a1 a2"
	]
	doc <- createDoc "Test.hs" "haskell" $ mkDoc "_"
	_ <- waitForDiagnostics
	actions <- getCodeActions doc (Range (Position 2 13) (Position 2 14))
	chosen <- liftIO $ pickActionWithTitle "replace _ with (+)" actions
	executeCodeAction chosen
	modifiedCode <- documentContents doc
	liftIO $ mkDoc "(+)" @=? modifiedCode
	, testSession "filling infix type hole uses infix operator" $ do
	let mkDoc x = T.unlines
	[ "module Testing where"
	, "test :: Int -> Int -> Int"
	, "test a1 a2 = a1 " <> x <> " a2"
	]
	doc <- createDoc "Test.hs" "haskell" $ mkDoc "`_`"
	_ <- waitForDiagnostics
	actions <- getCodeActions doc (Range (Position 2 16) (Position 2 19))
	chosen <- liftIO $ pickActionWithTitle "replace _ with (+)" actions
	executeCodeAction chosen
	modifiedCode <- documentContents doc
	liftIO $ mkDoc "+" @=? modifiedCode

use filtered actions, so these keep failing.

[expipiplus1]

hmm, the tests don't run for me locally. Would it be possible for someone else to take over this PR, please?

❯ cabal run test:ghcide-tests -- --pattern "postfix hole"
Up to date
ghcide
  code actions
    fill typed holes
      postfix hole uses postfix notation of infix operator: FAIL
        Exception: ghcide: createProcess: runInteractiveProcess: exec: does not exist (No such file or directory)

[jneira]

tests failing:

 Test suite ghcide-tests: RUNNING...
ghcide
  code actions
    fill typed holes
      postfix hole uses postfix notation of infix operator: FAIL (1.82s)
        test/exe/Main.hs:5546:
        Found no matching actions for "replace _ with (+)" in ["replace _ with test","replace _ with (-)","replace _ with (*)","replace _ with asTypeOf","replace _ with const","replace _ with pure","replace _ with (Some hole fits suppressed; use -fmax-valid-hole-fits=N or -fno-max-valid-hole-fits)","replace _ with curry _","replace _ with ($) _","replace _ with ($!) _","replace _ with const _","replace _ with flip _","replace _ with id _"]
        Use -p '/postfix hole uses postfix notation of infix operator/' to rerun this test only.
      filling infix type hole uses infix operator:          FAIL (1.83s)
        test/exe/Main.hs:5546:
        Found no matching actions for "replace _ with (+)" in ["replace _ with test","replace _ with (-)","replace _ with (*)","replace _ with asTypeOf","replace _ with const","replace _ with pure","replace _ with (Some hole fits suppressed; use -fmax-valid-hole-fits=N or -fno-max-valid-hole-fits)","replace _ with curry _","replace _ with ($) _","replace _ with ($!) _","replace _ with const _","replace _ with flip _","replace _ with id _"]
        Use -p '/filling infix type hole uses infix operator/' to rerun this test only.
  cradle
    dependencies

[Tritlo]

Note! Valid hole-fits are a lot faster now on HEAD, so this might not be required anymore.

[expipiplus1]

That's good news but my gripe was that when there are too many to choose from at a glance then one may as well just type it. The speed at which they appear wasn't really an issue for me here.

…

On Wed, 6 Oct 2021, 8:11 am Matthías Páll Gissurarson, < ***@***.***> wrote: Note! Valid hole-fits are a lot faster now on HEAD, so this might not be required anymore. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1914 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGRJXA3VBGKM7UHEIZTPSLUFOH2NANCNFSM46TIJRFQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[Anton-Latukha]

Hole fitting solving would always be costly for the foreseeable futuristic future.

I consider 10 as a sane default (which would terminate mostly in the sane time & not clutter much, which I would prefer).

Lets not bikeshed - don't let the ideal be the enemy of the good.

---

@expipiplus1 would you be kind to push it over the last bump? ❤️

[expipiplus1]

Sorry, I won't be able to get to this in a timely manner. Happy for someone else to take over though.

[Anton-Latukha]

Yep. Looked into commits.

Rebasing for this diff was a bit excessive 🤣

---

Ok then somebody from us would finish this.

[dnikolovv]

Hey all.

What's left for this one to get in?

[michaelpj]

This continues to be desirable but this implementation seems dead. Does anyone want to pick it up?

[michaelpj]

Superseded.