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

Ability for cross shard unknown type assignment? #4232

Closed
danielpclark opened this issue Apr 3, 2017 · 13 comments
Closed

Ability for cross shard unknown type assignment? #4232

danielpclark opened this issue Apr 3, 2017 · 13 comments

Comments

@danielpclark
Copy link

Here's a scenario. The Ruby Arel project has a feature in which a class called Table has the property @engine but the project doesn't use it itself (other than a test with a FakeEngine). Now this is meant to be used by a separate project which will include Arel and set their own engine on it. Any engine has to have a few specific methods defined on it (the API as it were).

There is no way to know what the object name or type is that will be assigned here as it is outside the scope of the project. So what do I do about the complaint the compiler gives me about Can't infer the type of instance variable '@engine' of Table.

The point is that I don't really care yet about the type and neither should the compiler since it won't be used in this project other than the fake engine test. Not until another project like ActiveRecord assigns it's own engine to it.

Do we have a kind of compiler flag to declare to ignore the nil type for now but raise an exception should another project use it incorrectly? @engine should eventually point to an object that meets the protocol specifications — but what can I do to help this kind of thing to work in Crystal?

@bcardiff
Copy link
Member

bcardiff commented Apr 3, 2017

A workaround could probably be to declare the @engine with a module type restriction and require it to be reopen with more method by the consume.

Another approach would be to use a generic Table and let it receive the engine type.

PS: Stackoverflow / mailing list is probably better for this kind of discussion.

@refi64
Copy link
Contributor

refi64 commented Apr 3, 2017

FWIW do you have a smaller example? Maybe you could use generics or macros somehow.

@danielpclark
Copy link
Author

danielpclark commented Apr 3, 2017

PS: Stackoverflow / mailing list is probably better for this kind of discussion.

I see. I had though about what place would be best for this. StackOverflow didn't seem right to me. But by you saying this it sounds like the Crystal language already accounts for this kind of scenario, in which case I'm mistaken.

FWIW do you have a smaller example?

The class itself is like this:

class Table
  @engine = nil
  property :engine
end

Other classes simply get the value set in Table.engine and then call the necessary protocol methods on it. The only methods that use it in Arel is to_sql


So after opening this issue I tried removing the @engine = nil line and Crystal stopped complaining about it. Could it be that it was merely this simple? The property :engine still exists… is that enough for this to work as planned?

@bcardiff
Copy link
Member

bcardiff commented Apr 3, 2017

Could it be that it was merely this simple?

It depends on the rest of the code.

class Table
  property engine
end

will generate a getter/setter and ivar without type annotation. From the initializers it will get it's type and from there it will or will not work as you hope.

@danielpclark
Copy link
Author

danielpclark commented Apr 3, 2017

So it sounds like this is not a valid solution.

It isn't written in the initializer. It's not used anywhere else in the code base other than when the method to_sql is called. The only time it gets assigned is when another project includes this shard and assigns Table.engine=.

Does Crystal have a solution for this kind of possibility built in?

@bcardiff
Copy link
Member

bcardiff commented Apr 3, 2017

If that assignment of Table.engine = is performed at the beginning of the program it could be enough for your use case. Yet I could discourage that kind of solution in the general case.

My way to go would be the same two I point before: put a Mixin restriction or use Generics if you want any other types.

Other sources of inspiration could be: juanedi/micrate and fridgerator/crecto which deal with the dialect per driver dilema.

@danielpclark
Copy link
Author

Other sources of inspiration could be: juanedi/micrate and fridgerator/crecto which deal with the dialect per driver dilema.

I'll have a look at those. It make take me a decent amount of time to come up with an alternative implementation. So I may take a while to get back to you on this.

But thank you so much for taking the time and pointing me in this direction.

@danielpclark
Copy link
Author

danielpclark commented Apr 3, 2017

One last thought. I'm very good with Ruby but still very new to Crystal. I was thinking that this could be implemented with a Macro. Is something like this possible?

class Table
  def self.engine
    raise "Not Implemented!"
  end

  def self.engine=(engine of T)
    # Some macro to
    # * 1) Redefine the `engine` method as a propper getter
    # * 2) Create the @engine instance variable and assign the engine to it of type T
    # * 3) Enable the `to_sql` method on a few other classes
  end
end

It seems I forgot to mention that this should be defined directly on the Table class and not instances of it.

@bcardiff
Copy link
Member

bcardiff commented Apr 3, 2017

You can reopen and redefine methods. So 1) is cover.

For 2), the ivars aren't created, they exist in the source to compile. Using macros is just a way to code less. This is a difference on how ruby works, in Crystal every object of a class responds with the same code/ivars structure.

I don't get the following: if @engine of a table is not needed at the creation of it, then you can have a engine selected by the context where you want to use the table and the design won't need that. If @engine is needed at creation and used on some other methods in Table, then you can expect it to implement a module or inherit a class.

@danielpclark
Copy link
Author

danielpclark commented Apr 3, 2017

I don't get the following: if @engine of a table is not needed at the creation of it, then you can have a engine selected by the context where you want to use the table and the design won't need that. If @engine is needed at creation and used on some other methods in Table, then you can expect it to implement a module or inherit a class.

Table.engine is considered a global configuration for any component to use. For example when Rails starts up it assigns ActiveRecord::Base as the engine. Then on any Arel::Node you can call to_sql to render.

Pseudo Code:

class Table
  def self.engine
    @@engine
  end
  def self.engine=(eng)
    @@engine = eng
  end
end

class Node
  def to_sql(engine = Table.engine)
    collector = Arel::Collectors::SQLString.new
    collector = engine.connection.visitor.accept self, collector
    collector.value
  end
end

# OTHER PROJECT
# Like Rails
# config/application

Table.engine = ActiveRecord::Base

some_node_result.to_sql

If @engine is needed at creation and used on some other methods in Table, then you can expect it to implement a module or inherit a class.

I think I have an idea of what you mean, but it's not clear to me.

@RX14
Copy link
Contributor

RX14 commented Apr 3, 2017

As you said earlier, to use engine you need to have an API for it. If that API is the same between all implementations (and if it's not, you can use the adaptor pattern to make it so) you should create a module with abstract methods and use that as your type restriction. Example:

module Engine
  # Methods that you want engine to have
  abstract def connection
  abstract def foo(bar)
end

class Table
  class_property engine : Engine
end

class Node
  def to_sql(engine = Table.engine)
    # Here typeof(engine.connection) should be a union of the return type of all the implementations, so you can call common mehods
    collector = Arel::Collectors::SQLString.new
    collector = engine.connection.visitor.accept self, collector
    collector.value
  end
end

class EngineImpl
  include Engine

  def connection
    # impl
  end
end

class EngineAdapter
  include Engine

  getter thing : OtherShard::Thing

  def connection
    thing.do_thing
  end
end

The problem with this solution is that it requires you to write adaptors. You could get away without it by reopening external classes (class SemiCompatibleShard::Thing; include Arel::Engine; end), but I tend to think that it's rather unlikely the external shard will keep the correct naming and semantics for you to not have to write special compatibility code.

@danielpclark
Copy link
Author

danielpclark commented Apr 3, 2017

@RX14 Thank you so much for this. Am I correct in understanding that EngineImpl would be my own shards implementation and the EngineAdapter is written for cases when other specific shards use it?

If that's so I would think the OtherShard::Thing would only work when the code is currently included and therefore needs to be written in that other shard. Am I right on this?

although with you saying “Here typeof(engine.connection) should be a union of the return type of all the implementations, so you can call common methods” that would seem to indicate I would write all the other implementations myself which agrees with you later saying “I tend to think that it's rather unlikely the external shard will keep the correct naming and semantics for you to not have to write special compatibility code.”

So it sounds like I would write the module names for the other project within my own code like OtherShard::EngineAdapter.

Also I now understand what @bcardiff was saying with “you can expect it to implement a module or inherit a class.” I didn't know you could use a type like in class_property engine : Engine and that would work for modules that used include Engine. So types aren't strictly the type named but rather act like a is_a? or kind_of?.

Now that I know about abstract I absolutely love the idea of it!

@RX14
Copy link
Contributor

RX14 commented Apr 3, 2017

@danielpclark You could separate the parts that depend on other shards into separate files then require them separately. Like src/othershard_adapter.cr would contain the code which required and depended on the external shard, then users can simply require "arel/othershard_adapter".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants