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.

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

Compiler: better error message for symbol against enum #12478

Merged
merged 9 commits into from
Sep 21, 2022

Conversation

asterite
Copy link
Member

@asterite asterite commented Sep 13, 2022

Another follow-up to the improved error messages.

If you have code like this:

enum Color
  Red
  Green
  Blue
end

def foo(x : Color)
end

foo(:redd)

Before this PR this was the error (I won't list "all overloads" because they are always shown):

In foo.cr:10:5

 10 | foo(:rred)
          ^
Error: expected argument #1 to 'foo' to be Color, not Symbol

With this PR:

In foo.cr:10:5

 10 | foo(:rred)
          ^
Error: expected argument #1 symbol to 'foo' to match a Color enum member.

Did you mean :red?

Much better, don't you think?

Here the name was similar but what if it isn't? Let's try it:

enum Color
  Red
  Green
  Blue
end

def foo(x : Color)
end

foo(:hello_world)

We get this error:

In foo.cr:10:5

 10 | foo(:hello_world)
          ^
Error: expected argument #1 symbol to 'foo' to match a Color enum member.

Options are: :red, :green and :blue

Cool!

What if the enum member has a lot of options? We don't show what are the options in that case (if they are more than 10)

Fixes #7284

@straight-shoota
Copy link
Member

I'm not sure how useful the "some options" path is. Just listing the first 5 seems very random. Perhaps they're the most common ones, but then you probably wouldn't have completely mistyped them in the first place.
It might even be harmful if people don't understand that "some options" means it shows just a subset and not all available members.
So I would suggest to perhaps print nothing when there are too many members in total (and too many could be something like 10 maybe?) and none matched for "did you mean".

@asterite
Copy link
Member Author

Done!

@HertzDevil
Copy link
Contributor

It seems this could be easily extended to multiple Enum-accepting overloads. For example:

enum Foo
  W
  X
  Y
end

enum Bar
  Y
  Z
end

def foo(x : Foo); end
def foo(x : Bar); end

foo :abc

This should show :w, :x and :z. The list shouldn't include :y because the autocast becomes ambiguous; in the extreme case there might be no usable symbols at all, because every name appears more than once in the list of enum types.

@asterite
Copy link
Member Author

@HertzDevil Sounds good, but I would do that in a separate PR.

@straight-shoota
Copy link
Member

Yeah, I'm not even sure this is really relevant. How often would you get such a case where different overloads accept different enum types for the same parameter?

@asterite
Copy link
Member Author

Will this go into 1.6?

@straight-shoota
Copy link
Member

Sure. I just missed adding it to the milestone.

@straight-shoota straight-shoota merged commit a9ab64f into master Sep 21, 2022
@straight-shoota straight-shoota deleted the errors/symbol-vs-enum branch September 21, 2022 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add detail to errors involving automatic casting of symbols
4 participants