-
Notifications
You must be signed in to change notification settings - Fork 279
Error Messages: on name resolution failure, suggest similar names #5522
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
Conversation
95893c6
to
25bead3
Compare
Yes, please. I ran into this exact issue yesterday (using I personally agree with eliminating the plurality conditionals – I feel that more static error messages makes searching for them easier when you run into them1. But in general the Unison project aims for very natural-sounding error messages, which these run counter to. If we do eliminate the conditionals, I think simplifying to just the plural instead of “one or more” reads better. E.g.,
You could also list the top matches across all different categories (like you already do with
That example combines Footnotes
|
@xmbhasin Sorry this PR fell through the cracks and didn't get a reply until now. This is a great idea and I was about to just merge it, but I hesitated to accept the "Changes to Error Message Preambles" change. I know it's easier to code up a very generic message, but then the user receives a very generic-feeling message, which we have intentionally tried to avoid. Would you want to revert that to separate the two questions? If we can come up with a more coder-friendly way to generate the more user-friendly messages, that would be a welcome PR as well. |
todo:
|
* Use plurality-agnostic messages for suggestions for simplicity and consistency * Partially addresses unisonweb#713
ddb75e3
to
7d44be3
Compare
7d44be3
to
9aa7d59
Compare
I've updated the PR, rebasing on the latest I've added the suggested performance tweak to exclude names from indirect dependencies for fuzzy searching during name resolution that is dependencies under lib.*.lib for performance. I've also adjusted the error messages for the different name resolution suggestion classes to be very close to the original messages including their plurality-gnostic nature. See |
9aa7d59
to
321e35d
Compare
321e35d
to
f622319
Compare
These were changed by unisonweb#5522. It doesn't seem like it should be a consequential change, but the nix build seems to be treating the following as a fatal error that breaks the build: ``` unison-runtime.cabal was generated with a newer version of hpack, please upgrade and try again. ```
Overview
This change improves error messages when the user provides a name in Unison source code files that does not exactly match a known name in the codebase or within the file. Specifically, it suggests that the user try one of the top 3 closest name matches by Levenshtein edit distance. This is intended to aid users in more quickly resolving typos and similar errors.
The PR introduces a few new suggestion categories that align with this fuzzy matching behaviour. Suggestions for matches with a similar name and the right type are presented first if possible. Otherwise, suggestions for similar name but wrong type are presented.
For example,
Changes to Error Message Preambles
Additionally, suggestions in error messages on name resolution failure now use plurality-agnostic messages for simplicity and consistency. For example we now have,
For example, this simplifies some of the error message code from:
to
Relevant Issues
Partially addresses #713.
Implementation notes
How does it accomplish it, in broad strokes? i.e. How does it change the Haskell codebase?
Unison.FileParsers
, functioncomputeTypecheckingEnvironment
, to produce aTypechecker.Env
with a new fieldfreeNameToFuzzyTermsByShortName
.freeNameToFuzzyTermsByShortName
is a mapping from the free names in the Unison file that needs to be type-checked to a mapping from short names to terms (similar to the fieldtermsByShortname
).fuzzyFindByEditDistanceRanked
used to produce fuzzy matches ranked by edit distance. This function uses two new functions inUnison.Names
,queryEditDistances
andqueryEditDistances'
that search throughNames
andSet Name
respectively to get edit distnaces.Unison.Typechecker
such that theresolve
function used byresolveNote
and in turn bytypeDirectedNameResolution
to produce and append fuzzy match suggestions to theResolution
data type.Unison.Typechecker.Context
to include theSimilarNameRightType
andSimilarNameWrongType
cases.Unison.PrintError
to handle printing of the new suggestion types.Interesting/controversial decisions
1
I chose fuzzy matching by edit distance because it is usually more appropriate than FZF-style fuzzy finding for suggesting typos or other user errors. I tried FZF-style matching initially since existing code used it, but it would sometimes produce strange matches. Additionally, the FZF-style matching code is significantly slower and currently includes a prefilter that restricts matching to substrings and is not as useful.
2
A tricky piece of improving fuzzy match results was preprocessing names in the search space to the last N name segments where N is the number of name segments in the free name the user provided. This helped reduce edit distance bias towards shorter, fully-qualified names. For example, a deeply nested name like
foo.bar.biz.bite
would be deprioritized compared to a name likebyz.byte
if the user tried to usebete
orbaz.bete
despite the edit distance being the same when considering only the last N segments. This could perhaps be improved by also considering aliases and qualified imports, but the adjustment here proved relatively simple and effective.This behaviour is covered by one of the transcript tests.
3
Somewhat arbitrarily, I chose to show only the top 3 matches by edit distance.
Additionally, I decided to filter out matches with large edit distance. Here I made the decision after some manual testing to go with the following max score formula:
4
There may be some unintended impact to the dependence of some transcript tests to the global namespace. When the global namespace changes, fuzzy matching results could change and this may change transcript test outputs. This will usually change in ucm transcripts with the
:error
directive so the change should be automatically captured, but needs to be reviewed.Test coverage
Some existing transcript tests cover name resolution failure error messages.
A new transcript
name-resolution-fuzzy.md
was added to more thoroughly cover all fuzzy name resolution scenarios.As noted in this transcript, there is another case where the term matches with terms with the wrong name but the right type. This case was not added here because it is unclear how to produce a concrete example. This kind of suggestion does not seem to be produced in the code paths in
Unison.TypeChecker
.Loose ends
1
It would be nice to have LSP code actions for accepting suggestions. This should be a separate ticket, and the design here hopefully doesn't conflict with that goal, but I don't have enough experience in the codebase to know.
2
It would be nice if LSP error reporting did not stop on the first error in the file. Although in the UCM it may be overwhelming to print all errors, it is useful while working on source code to be able to see all error hints so the user can work through errors in an order that works for them.
3
When name resolution fails it normally prints
However, when suggestions are available this isn't printed. If none of the suggestions make sense, then it might be good to for the user to be shown more general guidance, perhaps after the suggestions.
4
This PR addresses some of the situations documented by @seagreen in #713, but not all of them.
Example 1 - Local Name Resolution (Directly Addressed)
Now gives the more helpful suggestions:
Example 2 - Type Symbol Resolution (Not Addressed)
The following case where the name of the type is wrong (
Bool
instead ofBoolean
) is not addressed here as type symbol resolution seems to occur before term resolution and needs additional work.Example 3 - Global Name Resolution (Partially Addressed)
Unfortunately, some builtins like
true
don't seem to be readily found in the parsing environment and global namespace so the following provides some awkward suggestions:However, many other global names can be found and this scenario usually works. See the example above with
truncate
in the overview.