Improve error suggestion when using logical operators. Fixes #2715 #2738

fernandes · 2016-06-03T19:06:16Z

When using if foo and bar the compiler was suggestion the use of
rand method, and that is because rand is defined at top level and
Crystal::MatchesLookup was finding it as the nearest option.

Now it checks the missing method is and or or and suggest the right
&& and || operators.

fernandes · 2016-06-03T19:10:30Z

src/compiler/crystal/semantic/call_error.cr

@@ -282,6 +285,19 @@ class Crystal::Call
    raise message, owner_trace
  end

+  def logical_operator?(def_name)


created these methods to avoid getting the raise_matches_not_found method getting bigger and bigger.

If makes sense to keep them, my idea is to keep extracting more methods (is is a good idea?)

fernandes · 2016-06-03T19:26:08Z

totally missed the crystal tool format pushed the indented code 😎

jhass · 2016-06-03T19:46:45Z

src/compiler/crystal/semantic/call_error.cr

+  def convert_def_name_to_logical_operator(def_name)
+    case def_name
+    when "and"; "&&"
+    when "or" ; "||"


Not sure about the formatting here, might just do a Hash(String, String) to a constant and look up?

It's fine with me, I'm not sure adding a Hash is worth it

yeah... my first solution was a hash, then I thought was polluting unnecessary, and changed to method hahahah what you think is the best approach, I'm ok with :)

jhass · 2016-06-03T19:48:09Z

Looking at this I wonder whether we should future proof this a bit, we could generically define a list of static overrides for the suggestion, mentioning logic operators in the identifiers chosen here doesn't even seem necessary.

fernandes · 2016-06-03T19:51:42Z

@jhass in terms of code would be changing from logical_operator? to static_override? something like this?

jhass · 2016-06-03T19:55:44Z

src/compiler/crystal/semantic/call_error.cr

@@ -90,6 +90,9 @@ class Crystal::Call
      error_msg = String.build do |msg|
        if obj && owner != mod
          msg << "undefined method '#{def_name}' for #{owner}"
+        elsif logical_operator?(def_name)
+          msg << "undefined method '#{def_name}'"
+          similar_name = convert_def_name_to_logical_operator(def_name)


Why not just

similar_name = STATIC_SUGGESTIONS[def_name]? || owner.lookup_similar_def_name(def_name, self.args.size, block)

in line 88 above?

Note that this shouldn't always be suggested, only when there's no receiver. Doing that in line 88 will always suggest it, which is not very good.

Ah, that's what I missed.

asterite · 2016-06-03T20:05:28Z

We can add this if many feel this is OK. I tried doing if (1 and 2) in Java and C# and I don't get that suggestion. Should we start adding suggestions for every bit of Ruby syntax we are missing? Maybe it's too much.

(but I'm OK with adding these suggestions to make it easier for people coming from Ruby, the problem is when to stop)

fernandes · 2016-06-03T20:21:33Z

and/or are commonly used in Ruby, specially because && and and have different behaviors and in a lot of cases and is used.

I don't think we should add for every bit of Ruby, just for most common use cases (I know, what is a "common" case? again, when to stop) - that's the reason I added only for and, or and not for xor, not

asterite · 2016-06-03T20:23:19Z

But "not" is a keyword in Ruby. Maybe we should add it too.

fernandes · 2016-06-03T20:24:56Z

@asterite what would be the suggestion?

did you mean "!"?

asterite · 2016-06-03T20:25:39Z

@fernandes Yes

fernandes · 2016-06-03T20:27:16Z

@asterite done! 😄

asterite · 2016-06-04T14:33:48Z

@fernandes Just a final refactor before merging this. There's no need for two methods, logical_operator? and convert_def_name_to_logical_operator. We can have just one, logical_operator? that returns nil if it's not an operator, or the replacement if it is (this is your current convert_def_name_to_logical_operator). Then do:

if obj && owner != mod
   msg << "undefined method '#{def_name}' for #{owner}"
elsif op = logical_operator?(def_name)
  msg << "undefined method '#{def_name}'"
  similar_name = op

That way there's less code and the method is only called once (a bit faster, but won't be noticeable).

fernandes · 2016-06-04T15:15:41Z

@asterite thanks for the tips... what do you think about convert_to_logical_operator ?

usually methods like foo? are interest when returning boolean, is there any convention like this in crystal or do you prefer to keep logical_operator?

asterite · 2016-06-04T15:21:40Z

The convention of methods ending with ? is that they either return a bool or a nilable type. I think it's fine for logical_operator? to return the converted name, or nil if there's no conversion to be done, and then use that return value.

asterite · 2016-06-04T15:22:15Z

But you can use convert_to_logical_operator too, with the same meaning, choose what you like more :-)

When using `if foo and bar` the compiler was suggestion the use of `rand` method, and that is because `rand` is defined at top level and Crystal::MatchesLookup was finding it as the nearest option. Now it checks the missing method is `and` or `or` and suggest the right `&&` and `||` operators.

fernandes · 2016-06-04T21:42:13Z

@asterite ready! 👍

asterite · 2016-06-04T23:34:27Z

Thanks!!

fernandes · 2016-06-05T01:44:14Z

🙇

fernandes reviewed Jun 3, 2016
View reviewed changes

jhass reviewed Jun 3, 2016
View reviewed changes

jhass added the topic:compiler label Jun 3, 2016

asterite merged commit 356bf1a into crystal-lang:master Jun 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error suggestion when using logical operators. Fixes #2715 #2738

Improve error suggestion when using logical operators. Fixes #2715 #2738

fernandes commented Jun 3, 2016

fernandes Jun 3, 2016

fernandes commented Jun 3, 2016

jhass Jun 3, 2016

asterite Jun 3, 2016

fernandes Jun 3, 2016

jhass commented Jun 3, 2016 •

edited

Loading

fernandes commented Jun 3, 2016

jhass Jun 3, 2016

asterite Jun 3, 2016

jhass Jun 3, 2016

asterite commented Jun 3, 2016

fernandes commented Jun 3, 2016

asterite commented Jun 3, 2016

fernandes commented Jun 3, 2016

asterite commented Jun 3, 2016

fernandes commented Jun 3, 2016

asterite commented Jun 4, 2016

fernandes commented Jun 4, 2016

asterite commented Jun 4, 2016

asterite commented Jun 4, 2016

fernandes commented Jun 4, 2016

asterite commented Jun 4, 2016

fernandes commented Jun 5, 2016

Improve error suggestion when using logical operators. Fixes #2715 #2738

Improve error suggestion when using logical operators. Fixes #2715 #2738

Conversation

fernandes commented Jun 3, 2016

fernandes Jun 3, 2016

Choose a reason for hiding this comment

fernandes commented Jun 3, 2016

jhass Jun 3, 2016

Choose a reason for hiding this comment

asterite Jun 3, 2016

Choose a reason for hiding this comment

fernandes Jun 3, 2016

Choose a reason for hiding this comment

jhass commented Jun 3, 2016 • edited Loading

fernandes commented Jun 3, 2016

jhass Jun 3, 2016

Choose a reason for hiding this comment

asterite Jun 3, 2016

Choose a reason for hiding this comment

jhass Jun 3, 2016

Choose a reason for hiding this comment

asterite commented Jun 3, 2016

fernandes commented Jun 3, 2016

asterite commented Jun 3, 2016

fernandes commented Jun 3, 2016

asterite commented Jun 3, 2016

fernandes commented Jun 3, 2016

asterite commented Jun 4, 2016

fernandes commented Jun 4, 2016

asterite commented Jun 4, 2016

asterite commented Jun 4, 2016

fernandes commented Jun 4, 2016

asterite commented Jun 4, 2016

fernandes commented Jun 5, 2016

jhass commented Jun 3, 2016 •

edited

Loading