Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve UndefinedFunctionError for mis-cased module #12839

Merged
merged 7 commits into from
Aug 9, 2023
Merged

Improve UndefinedFunctionError for mis-cased module #12839

merged 7 commits into from
Aug 9, 2023

Conversation

davydog187
Copy link
Contributor

@davydog187 davydog187 commented Aug 6, 2023 •
edited
Loading

This PR improves the error messages when a module is correctly spelled, but has mis-cased letters.

We will now get error messages like

iex(1)> ENum.map([1, 2, 3], & &1 + 1)

09:39:24.348 [error] beam/beam_load.c(180): Error loading module 'Elixir.ENum':
  module name in object code is 'Elixir.Enum'



09:39:24.355 [error] beam/beam_load.c(180): Error loading module 'Elixir.ENum':
  module name in object code is 'Elixir.Enum'


** (UndefinedFunctionError) function ENum.map/2 is undefined (module ENum is not available). Did you mean:

      * Enum.map/2

    ENum.map([1, 2, 3], #Function<42.125776118/1 in :erl_eval.expr/6>)
    iex:1: (file)

Motivation

I saw two experienced developers who are new to Elixir get stuck on this issue on the same day, not including a mention on Twitter!

Twitter Conversation

Follow ups

Initially, I wanted to capture 3 use cases. This PR only solves for case 1, the other two I will explore as follow ups

  1. The module is miscased .e.g. ENum -> Enum
  2. The module is misspelled, e.g. Enu -> Enum
  3. The module is not qualified SomeModule -> Namespace.SomeModule

Prior Art

@davydog187
Improve UndefinedFunctionError for mispelled module
444eab6 
This is a proof of concept to see if we can improve error messages when
a module name is mispelled or otherwise incorrect. This can happen when
a user misspells a module, or does not fully qualify it. Ideally, the
elixir compiler can provide suggestions for what they meant.

If we were to merge a version of this PR, we would get error messages like

```elixir
iex(1)> Enu.map([1, 2, 3], & &1 + 1)
** (UndefinedFunctionError) function Enu.map/2 is undefined (module Enu is not available). Did you mean:

      * Enum.map/2

    Enu.map([1, 2, 3], #Function<42.125776118/1 in :erl_eval.expr/6>)
    iex:1: (file)
```

```elixir
iex(1)> defmodule Namespace.SomeModule do
...(1)>   def foo, do: :bar
...(1)> end
{:module, Namespace.SomeModule,
 <<70, 79, 82, 49, 0, 0, 5, 36, 66, 69, 65, 77, 65, 116, 85, 56, 0, 0, 0, 186,
   0, 0, 0, 18, 27, 69, 108, 105, 120, 105, 114, 46, 78, 97, 109, 101, 115, 112,
   97, 99, 101, 46, 83, 111, 109, 101, 77, ...>>, {:foo, 0}}
iex(2)> SomeModule.foo()
** (UndefinedFunctionError) function SomeModule.foo/0 is undefined (module SomeModule is not available). Did you mean:

      * Namespace.SomeModule.foo/0

    SomeModule.foo()
    iex:2: (file)
```

I saw two experienced developers who are new two Elixir get stuck on
this issue on the same day, not including a mention on Twitter!

[Twitter Conversation](https://twitter.com/davydog187/status/1687628454240428034?s=20)

- [ ] Better heuristic that is accurate for most usecases
- [ ] Consider other edge cases and measure results (forgetten alias?)
- [ ] Possibly turn off
outside of dev?
- [ ] Remove `to_atom/1`
- [ ] Write more tests

* https://groups.google.com/g/elixir-lang-core/c/-IHneszMheI/m/cCCxHB9PBwAJ
* elixir-lang#5665
@w0rd-driven
Copy link

I wonder if you could reuse did_you_mean(module, function, exports) at

elixir/lib/elixir/lib/exception.ex

Line 1387 in 444eab6

defp did_you_mean(module, function, exports) do
. The threshold may be too eager but it has up to 5 suggestions.

@davydog187
Copy link
Contributor Author

I wonder if you could reuse did_you_mean(module, function, exports) at

elixir/lib/elixir/lib/exception.ex

Line 1387 in 444eab6

defp did_you_mean(module, function, exports) do
. The threshold may be too eager but it has up to 5 suggestions.

Thanks for the suggestion, but this function makes the assumption that the module is loaded, so it cannot be reused

@whatyouhide
Copy link
Member

We could start with nicer info only on casing issues, so that the check is a matter of comparing the downcases strings for equality. I think typos are less hard to debug in general, and if that's the case we'd get more value for money, right?

@davydog187
Copy link
Contributor Author

We could start with nicer info only on casing issues, so that the check is a matter of comparing the downcases strings for equality. I think typos are less hard to debug in general, and if that's the case we'd get more value for money, right?

Yep I'll get that out first. The more complicated heuristic can be explored as a follow up to. TBD if it's good enough

@davydog187
Copy link
Contributor Author

@josevalim @whatyouhide I updated this PR to downcase the strings and updated the PR description to reflect the descope. I will explore the other 2 edge cases in another PR

@davydog187 davydog187 changed the title Improve UndefinedFunctionError for mispelled module Improve UndefinedFunctionError for mis-cased module Aug 9, 2023
whatyouhide
whatyouhide approved these changes Aug 9, 2023
View reviewed changes
Copy link
Member

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic!!!

lib/elixir/lib/exception.ex Outdated Show resolved Hide resolved
@davydog187
Copy link
Contributor Author

@whatyouhide CI seems broken on an unrelated issue

==> elixir (compile)
     warning: String.capitalize/1 is deprecated. Use :string.titlecase instead
     │
 287 │         reason = string |> List.to_string() |> String.capitalize()
     │         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     │
     └─ lib/module/types/of.ex:287: Module.Types.Of.check_deprecated/7
     └─ lib/module/types/of.ex:291: Module.Types.Of.check_deprecated/7

@josevalim josevalim closed this Aug 9, 2023
@josevalim josevalim reopened this Aug 9, 2023
@josevalim josevalim merged commit 458fcaf into elixir-lang:main Aug 9, 2023
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

davydog187 added a commit to davydog187/elixir that referenced this pull request Aug 14, 2023
@davydog187
Improve UndefinedFunctionError for unqualified module
134f320 
This PR builds off of elixir-lang#12839, handling the case where a developer
forgets to alias a module, resulting in UndefinedFunctionError.

Imagine we have the following modules in our Elixir program.

```elixir
def MyAppWeb.Context.Event do
  def foo, do: :bar
end
```

If the developer attempts to reference this module, but forgets to type
the alias, they will now get module suggestions like.

```elixir
** (UndefinedFunctionError) function Event.foo/0 is undefined (module Event is not available). Did you mean:

      * MyAppWeb.Context.Event.foo/0

    Event.foo()
    iex:2: (file)
```
@davydog187 davydog187 mentioned this pull request Aug 14, 2023
Merged
2 tasks
josevalim pushed a commit that referenced this pull request Aug 15, 2023
@davydog187
Improve UndefinedFunctionError for unqualified module (#12859)
44cce79 
This PR builds off of #12839, handling the case where a developer
forgets to alias a module, resulting in UndefinedFunctionError.

Imagine we have the following modules in our Elixir program.

```elixir
def MyAppWeb.Context.Event do
  def foo, do: :bar
end
```

If the developer attempts to reference this module, but forgets to type
the alias, they will now get module suggestions like.

```elixir
** (UndefinedFunctionError) function Event.foo/0 is undefined (module Event is not available). Did you mean:

      * MyAppWeb.Context.Event.foo/0

    Event.foo()
    iex:2: (file)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sabiwara sabiwara sabiwara left review comments

@josevalim josevalim josevalim left review comments

@whatyouhide whatyouhide whatyouhide approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@davydog187 @w0rd-driven @whatyouhide @josevalim @sabiwara