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: always mention the scope (and with ... yield scope, if any) on undefined method error #7384

Merged

Conversation

Projects
None yet
7 participants
@asterite
Copy link
Member

commented Feb 6, 2019

Fixes #7357

For the code in the issue:

def make_object
  "test"
  puts "This was my error"
end

def doit
	with make_object() yield
end

doit {
  size
}

You now get this error:

Error in baz.cr:11: instantiating 'doit()'

doit {
^~~~

in baz.cr:11: instantiating 'doit()'

doit {
^~~~

in baz.cr:12: undefined local variable or method 'size' for Nil (for `with ... yield`) and top-level (current scope)

  size
  ^~~~

The error now mentions both scopes (in order) where the method was searched.

Additionally, the scope is now always mentioned to make it easy to understand what went wrong. For example, for this code:

hello

Before: undefined local variable or method 'hello'
After: undefined local variable or method 'hello' for top-level

Similarly, for this code:

class Foo
  def foo
    hello
  end
end

Foo.new.foo

Before: undefined local variable or method 'hello'
After: undefined local variable or method 'hello' for Foo

Or...

class Foo
  def self.foo
    hello
  end
end

Foo.foo

Before: undefined local variable or method 'hello'
After: undefined local variable or method 'hello' for Foo.class ("Ooooh... I totally forgot I was in a class method!")

@asterite asterite force-pushed the asterite:feature/improve-undefined-method-error branch from f5c7ff8 to c6e0094 Feb 6, 2019

@bew

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

For: undefined local variable or method 'hello' for Foo

We should mention top-level too?

@asterite

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

@bew I think at that point it will be too confusing... the top-level is always there. Plus if Foo inherited some other class or included some other modules, should we mention all the hierarchy?

Or maybe yes... but if we go that way, I'd actually like to change all error messages to be more friendly and comprehensive, kind of like what Elm does.

@asterite asterite force-pushed the asterite:feature/improve-undefined-method-error branch from c6e0094 to 25a0a3c Feb 6, 2019

@bew

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

@bew I think at that point it will be too confusing... the top-level is always there. Plus if Foo inherited some other class or included some other modules, should we mention all the hierarchy?

Indeed, let's keep it that way for now.

@mpcjanssen

This comment has been minimized.

Copy link

commented Feb 6, 2019

@asterite is the Int32 in the initial comment an example from a different piece of code? I would expect the error here to reference nil (the result of make_object:

in baz.cr:12: undefined local variable or method 'size' for Nil (for `with ... yield`) and top-level (current scope)

edit: I see the error is copy-pasted from the test suite result which yields with 1 as receiver.

@asterite

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

@mpcjanssen Oops, you are right. It actually prints Nil instead of Int32. That was from some other example I had.

@asterite

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

I added a second commit to not use colorize in compiler specs because it was making testing the messages a bit tedious and error-prone.

@ysbaddaden ysbaddaden merged commit 4c5f1fe into crystal-lang:master Feb 15, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ysbaddaden ysbaddaden added this to the 0.28.0 milestone Feb 15, 2019

@asterite asterite deleted the asterite:feature/improve-undefined-method-error branch Mar 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.