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: fix new/initialize lookup regarding modules #7818

Merged

Conversation

@asterite
Copy link
Member

commented May 23, 2019

Fixes #7007

It turned out the issue was that even this compiled:

module Base
  def initialize(x)
  end
end

class Foo
  include Base
end

Foo.new # Works, finds then one in Reference, but shouldn't!

The problem was that new/initialize lookup looked past modules that defined an initialize method and this was wrong.

@bew

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

Does that mean that this won't work intentionally?
(because once it finds a initialize in the first parent it stops)

module Base1
  def initialize(x)
  end
end

module Base2
  def initialize(x, y)
  end
end

class Foo
  include Base1
  include Base2
end

Foo.new 1
Foo.new 1, 2

(not sure which one would fail based on the order of parents, the one in Base1 or Base2?)

@asterite

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

@bew Yes, that's correct. You can't have initialize defined at multiple levels. This is probably a breaking change, but it simply doesn't make sense to allow initialize bypass a level, because then you are potentially bypassing initializing an instance variable.

@asterite asterite force-pushed the asterite:bug/include-module-initialize branch from 8b43946 to 3f03066 May 24, 2019

@asterite

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

Maybe later we can refine this rule, but for now I think this is what makes most sense. I really wouldn't advice using initialize in a module anyway.

@asterite

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

Put another way, I coded the compiler to always check the first level of initialize to determine whether all instance variables are initialized. That an initialize on a later level can be called is a bug and it wasn't intentional at all.

@asterite

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

You can always do:

module Base1
  def initialize(x)
  end
end

module Base2
  def initialize(x, y)
  end
end

class Foo
  include Base1
  include Base2

  def initialize(x)
    super
  end
end

Foo.new 1
Foo.new 1, 2

(in fact JSON::Serializable does something similar, it injects a self.new on inherited)

@asterite asterite requested a review from bcardiff May 24, 2019

@bcardiff
Copy link
Member

left a comment

To clarify: this PR disallow the default initialize/new if any of the included modules define an initialize method.

User is able to declare multiple initialize across multiple modules and include them.

Similar to when a custom #initialize is defined, if the default arg-less is wanted, it needs to be defined explicitly.

Did I miss any other aspect/scenario?

@asterite

This comment has been minimized.

Copy link
Member Author

commented May 27, 2019

User is able to declare multiple initialize across multiple modules and include them.

This is not like that. If a type defines an initialize method (any arity) then initialize in super types (parent classes or included modules) are not looked up. Think of Java or C#: constructors are never inherited. The exception in Crystal is that if a type doesn't define a constructor, parent constructors are looked up.

@bcardiff

This comment has been minimized.

Copy link
Member

commented May 27, 2019

I didn't mention inheritance, but I should have. Ok with that semantics.

I was trying to state what will happen after this PR with:

module Base1
  def initialize(x)
  end
end

module Base2
  def initialize(x, y)
  end
end

class Foo
  include Base1
  include Base2
end

Foo will have both initialize or only one?

@asterite

This comment has been minimized.

Copy link
Member Author

commented May 27, 2019

@bcardiff Only the one from Base2.

Maybe we can change that... but if we do that we should formally define how we want initialize/new to work in all cases, not just in this particular case, and how to check that all non-nilable instance vars are initialized for all including classes.

@bcardiff

This comment has been minimized.

Copy link
Member

commented May 27, 2019

Ok, the last context for now then. I see how this can be understood as consistent with not inheriting initializer from the parent from some point of view.

Let's merge this as is for now.

It's not 100% neat that super is used to point to some initializers of a module. But is ok for now I think.

@asterite asterite merged commit 7351889 into crystal-lang:master May 27, 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

@asterite asterite deleted the asterite:bug/include-module-initialize branch May 27, 2019

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

@asterite @bcardiff I'm assuming it's known that this code would no longer work come next release? Seems undesired IMO.

require "json"

abstract class Parent
  @name : String

  def initialize(@name : String); end
end

class Child < Parent
  include JSON::Serializable
end

Child.new "Fred"
Error in test.cr:39: no overload matches 'Child.new' with type String
Overloads are:
 - Child.new(pull : ::JSON::PullParser)

Child.new "Fred"
@asterite

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

Yes, constructors are not (and shouldn't) be inherited.

That said, I always wanted JSON serialization to be decoupled from constructors, but nobody else liked that idea, so...

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

@asterite Why shouldn't constructors be inherited from abstract parent classes/structs? Currently, anything that has constructors shared in a parent abstract class/struct that includes a serializable modules in a child class, just doesn't work.

@asterite

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

Because a parent constructor has no guarantee of initializing instance variables of a subclass.

@nsuchy nsuchy referenced this pull request Jun 6, 2019
0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.