Limit number of valid hole fits to 10 #4288
Conversation
| @@ -449,7 +449,7 @@ setUpTypedHoles df | |||
| $ df | |||
| { refLevelHoleFits = Just 1 -- becomes slow at higher levels | |||
| , maxRefHoleFits = Just 10 -- quantity does not impact speed | |||
| , maxValidHoleFits = Nothing -- quantity does not impact speed | |||
| , maxValidHoleFits = Just 10 -- quantity does not impact speed | |||
There was a problem hiding this comment.
outdated comment also on the previous line
Can we have a comment explaining the choice of the magic number? "I made this up" is fine, it indicates we don't have a good reason for it, which is helpful information for the future
There was a problem hiding this comment.
Is this the version we want? I don't really know.
There was a problem hiding this comment.
Fixed the comments.
| @@ -69,7 +69,8 @@ processHoleSuggestions mm = (holeSuggestions, refSuggestions) | |||
| (mrAfter . (=~ t " *Valid (hole fits|substitutions) include")) | |||
| validHolesSection | |||
| let holeFit = T.strip $ T.takeWhile (/= ':') holeFitLine | |||
| guard (not $ T.null holeFit) | |||
| guard $ not $ holeFit =~ t "Some hole fits suppressed" | |||
There was a problem hiding this comment.
What's going on here? Why are we doing this?
There was a problem hiding this comment.
GHC prints this line where we expect a hole fit to be when the number of hole fits is limited. This line just ignores that. We do this in the refinement hole fits too.
| @@ -449,7 +449,7 @@ setUpTypedHoles df | |||
| $ df | |||
| { refLevelHoleFits = Just 1 -- becomes slow at higher levels | |||
There was a problem hiding this comment.
So, I think these things are also user-configurable. Generally it's nice if we don't overwrite user settings if they've set them. So can we adjust these so that they use the value from df if it's not Nothing and otherwise use these values? I actually think that's what the old PR did?
There was a problem hiding this comment.
Now respecting previous values like the other PR.
In cases where GHC doesn't know anything about the type of a hole, it suggests every available symbol as a hole fit, which can cause editors to crash or at least be very slow. 10 seems to be a fair number to limit hole fits to.
…valid hole fits are limited
akshaymankar
force-pushed
the
limit-hole-fits
branch
from
70dd76a to
0d9bb43
Compare
|
Genuine test failure in |
Just replaced the test with a type which has fewer hole-fits. For me getting 20 code suggestions for a hole feels a bit overwhelming (10 is already pushing it honestly), but its just a feeling. |
Now that we limit number of hole fits recommended by GHC, the test that hopes to find `+` being recommended for `Int -> Int -> Int` becomes unpredictable because there are too many symbols which match that type and the sorting has little control over which symbols get recommended. There are way fewer matches for `(Int -> Maybe Int) -> Maybe Int -> Maybe Int`, so it makes the test consistently succeed.
akshaymankar
force-pushed
the
limit-hole-fits
branch
from
0576add to
6a8e424
Compare
|
Great, happy to take your read on the UX. |
|
Hmm. Does this then now not inform the users when there are hole-fits found but are suppressed by this setting? That seems confusing and counterintuitive, though I might be biased. |
|
This doesn't change how the normal diagnostic from GHC is produced, I think. |
|
We do get this info from GHC that some hole-fits are suppressed. I don't know what would be a good way to communicate this info. |
|
If you mean e.g. in offering completions, I don't think we have a good way of conveying that information... |
Fixes #4198
Supersedes #1914