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

Give proper error when trying to instantiate Module #6735

Merged
merged 7 commits into from
Jan 5, 2019

Conversation

wooster0
Copy link
Contributor

Fixes #3885

Now this:

module Foo
  def initialize
  end
end
Foo.new

gives:

Error in test.cr:5: instantiating 'Foo:Module#new()'

Foo.new
    ^~~

in test.cr:2: cannot instantiate Foo:Module

  def initialize
  ^

instead of:

Error in line 5: instantiating 'Foo:Module#new()'

in line 2: undefined local variable or method 'allocate'

@straight-shoota
Copy link
Member

I'm not sure this is a good change. allocate could just be a regular method on a module and could be called as suche. The error message is at least misleading in a case where the method was just not defined - because some include is missing, or the def is mispelled for example. This could very well be a valid call even on a module.

module Foo
  def self.allocate
  end
end

Foo.allocate`

Failing to call a method allocate on a module doesn't mean you're trying to instantiate it.

I'd leave the error message as is and maybe add a note about being unable to instantiate a module.

@jhass
Copy link
Member

jhass commented Sep 16, 2018

Perhaps we rather should make sure the call to .new already fails unless it's defined explicitly

@straight-shoota
Copy link
Member

@r00ster91 Can you change this to add a note to the original error instead? @jhass's suggestion can come later.

@RX14
Copy link
Contributor

RX14 commented Jan 2, 2019

Specs need updating

@RX14 RX14 requested a review from asterite January 5, 2019 13:28
@@ -100,6 +100,10 @@ class Crystal::Call
end
end

if def_name == "allocate" && owner.is_a?(ModuleType) && owner.is_a?(MetaclassType)
Copy link
Member

Choose a reason for hiding this comment

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

This condition should check if onwer is a MetaclassType and owner.instance_type.module?

The current check reads well but makes no sense because a MetaclassType inherits from ModuleType. It just happens to work because the only metaclasses that don't have an allocate are modules, though I think this check will also trigger for enums

@RX14 RX14 added this to the 0.27.1 milestone Jan 5, 2019
@RX14 RX14 merged commit 0329057 into crystal-lang:master Jan 5, 2019
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.

5 participants