-
Notifications
You must be signed in to change notification settings - Fork 40
Add 'did you mean' help for mistaken type names #1162
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
| def availableTypes = all(id.path, scope) { | ||
| _.types.keys.toList | ||
| }.flatten.toSet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very much not a fan of this, can we do better here somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you expect? We somehow need to gather all types from all scopes. The only thing that looks a bit weird at first to me is the .keys, but that makes complete sense.
| // Priority 1: A specific heuristic | ||
| specificHeuristic(nameNotFound) orElse { | ||
| // Priority 2: Exact case-insensitive match | ||
| candidates | ||
| .find { name => name.toUpperCase == nameNotFound.toUpperCase } | ||
| .map { exactMatch => pp"Did you mean $exactMatch?" } | ||
| .orElse { | ||
| // Priority 3: Edit distance | ||
| val threshold = ((nameNotFound.length + 2) max 3) / 3 | ||
|
|
||
| candidates | ||
| .toSeq | ||
| .flatMap { candidate => | ||
| effekt.util.editDistance(nameNotFound, candidate, threshold).map((_, candidate)) | ||
| } | ||
| .sorted | ||
| .headOption | ||
| .map { case (_, name) => pp"Did you mean $name?" } | ||
| } | ||
| } foreach { msg => E.info(msg) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried very hard to structure this sanely, but it's still not what I'd prefer, is there a better way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not too bad. It is modular enough to also use it for terms. Well done!
Of course the general concern of better-error-messages-complicate-compiler still applies. At least you managed to keep the actual change in lookupType to just a few lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[prob. overengineering, but since you asked:]
Maybe there'd be a sufficiently common pattern between the two others that could be extracted into a foo(candidates: ...)(comp: (String,String) => ?) => Option[String] function?
Also depending on how much we care about efficiency here.
| val d = Array.ofDim[Int](N + 1, M + 1) | ||
| for (i <- 0 to N) { d(i)(0) = i } | ||
| for (j <- 0 to M) { d(0)(j) = j } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this performs too badly, we can always use two or three rows of the array only... (it just makes it harder to read)
| // Priority 1: A specific heuristic | ||
| specificHeuristic(nameNotFound) orElse { | ||
| // Priority 2: Exact case-insensitive match | ||
| candidates | ||
| .find { name => name.toUpperCase == nameNotFound.toUpperCase } | ||
| .map { exactMatch => pp"Did you mean $exactMatch?" } | ||
| .orElse { | ||
| // Priority 3: Edit distance | ||
| val threshold = ((nameNotFound.length + 2) max 3) / 3 | ||
|
|
||
| candidates | ||
| .toSeq | ||
| .flatMap { candidate => | ||
| effekt.util.editDistance(nameNotFound, candidate, threshold).map((_, candidate)) | ||
| } | ||
| .sorted | ||
| .headOption | ||
| .map { case (_, name) => pp"Did you mean $name?" } | ||
| } | ||
| } foreach { msg => E.info(msg) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not too bad. It is modular enough to also use it for terms. Well done!
Of course the general concern of better-error-messages-complicate-compiler still applies. At least you managed to keep the actual change in lookupType to just a few lines.
| def availableTypes = all(id.path, scope) { | ||
| _.types.keys.toList | ||
| }.flatten.toSet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you expect? We somehow need to gather all types from all scopes. The only thing that looks a bit weird at first to me is the .keys, but that makes complete sense.
| @@ -0,0 +1,3 @@ | |||
| module boolean_wrong_name | |||
|
|
|||
| def alwaysTrue(): Boolean = true No newline at end of file | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we have it for terms, we can add a hint True ~> true for @BinderDavid ;)
And S(S(Z())) ~> "Did you mean 2?"
marzipankaiser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already commented above, LGTM!
Great also for people that sometimes forget which direction we changed Bool/ean...
(👉😅👈)
|
Thanks for the reviews! |
Add a system to help the users when the compiler cannot find a name of a type: #### Heuristic 1: Some types are named differently in Effekt than in other languages <img width="469" height="100" alt="Screenshot 2025-10-27 at 15 06 41" src="https://github.com/user-attachments/assets/8b2a4a64-1860-4e5c-b1ec-05b9c3ecc609" /> This specific one (`Boolean` instead of `Bool`) is done often by students and LLMs alike. #### Heuristic 2: Try case-insensitive comparison <img width="390" height="137" alt="Screenshot 2025-10-27 at 15 06 26" src="https://github.com/user-attachments/assets/893777c5-247c-486f-8a94-704c740fe69d" /> #### Heuristic 3: Try full on edit distance! <img width="359" height="135" alt="Screenshot 2025-10-27 at 22 31 43" src="https://github.com/user-attachments/assets/9570b6ef-2222-4132-9575-e8773edf8f3b" />
Add a system to help the users when the compiler cannot find a name of a type:
Heuristic 1: Some types are named differently in Effekt than in other languages
This specific one (
Booleaninstead ofBool) is done often by students and LLMs alike.Heuristic 2: Try case-insensitive comparison
Heuristic 3: Try full on edit distance!