Scoped imports #4439
Comments
|
Yes, I think the current Please provide a detailed proposal of how to solve this, what changes are needed, and a working PR with it. |
|
Here's the problems as I see it:
|
The same thing that happens if you
See 1.
Not sure what this means. Can you give an example?
Isn't this what (potentially) happens with |
This proposal namespaces libraries but not the libraries' dependencies as long as the libraries use require and not import (which is a fair assumption just after this feature is released, features that require adoption to work often don't get adoption, see refinements in ruby). That seems a bit counter-productive if the aim it to reduce namespace pollution.
Often libraries will themselves use types of another library in their API. If the type that this library "exports" is imported instead of required, you can't access the name of the type that this library uses without requiring it yourself. So if you need to use this transitive type dependency, you need to require the original library which can be painful. Furthermore, if we used your original syntax proposal directly, it seems like you'd have to require the library that you're importing just to access it's types.
With require, you can only realistically have 1 version of a given library at the same time, using import with multiple namespaces would relax that restriction and you end up with incompatibilities in types between multiple different versions of a library. |
I'm not sure what you mean. Private modules and classes are already supported in Crystal as of #3280. The implementation I've sketched here, which, of course, is subject to change, discussion and refinement, proposes a hook which allows a compilation unit to return a value in response to being imported e.g.: main.crcommand.crprivate class Command
# ...
end
macro imported(*args)
Command.new(...)
endcould desugar to something like (this is just a sketch): main.crcmd = CompilationUnit["command"].importedcommand.crprivate class Command
# ...
end
class << CompilationUnit.current
def self.imported(*args)
Command.new(...)
end
endwhere
I still don't follow. Can you give an example of how this issue manifests itself in, say, ES6 or CommonJS, which support monkey patching and scoped imports?
Again, this proposal doesn't affect or change namespaces in any way. |
|
Sorry for the super-long post. I tried paring it down, but there were a few distinct points I wanted to make that kept it pretty long. I'm all for implementing something like The issue that I see with it is somewhat overarching of the points that RX14 brought up: there would be two distinct paradigms for loading code, and relying on 3rd-party libraries inevitably means that users will have to deal with both in their codebase. Because there are advantages to both systems, creating a single, idiomatic approach for programmers to follow would be nearly impossible. People's opinions are unlikely to change, and as soon as one library decides to go the other way (use In theory, it's nice to say that I think it should be the library's responsibility to namespace code, not the user's. Even then, it's possible (and easy) to resolve namespace collisions with a simple re-assignment as I'll show later on, so I don't see any explicit gain in having tl;dr; opinion from the rest of this post: Languages like Python, ES6 (I think. at least with the es6 module system), and Java only provide scoped imports. They don't have global imports to a single namespace in the way that import osdoesn't import from math import floor
That's one of the fundamental differences between (in particular) Python and Ruby. Python requires that you explicitly state all dependencies in every file they are used, while Ruby allows you to build up a global namespace that is available everywhere. Each method has its pros and cons, and debating them is pointless, since there will never be consensus (if you haven't gathered already, I'm biased towards the Ruby/C/C++ method). I think it's important, though, to recognize that Crystal has adopted the # some_library.rb
module SomeLibrary
class Thing; end
end
# some_other_library.rb
module SomeOtherLibrary
class Thing; end
end
# no namespace collisions :)
require 'some_library'
require 'some_other_library'
SomeLibrary::Thing
SomeOtherLibrary::ThingIf (somehow) you still have namespace collisions, you can manually rename dependencies quite easily: require 'some_library'
SomeRenamedLibrary = SomeLibrary
load 'some_library' # using load instead of require simply to reload the same file again
# still no namespace collisions
SomeRenamedLibrary::Thing
SomeLibrary::ThingThis is a contrived example, for sure, but (again, imo) anything that doesn't follow this pattern should be considered non-idiomatic code and refactored to follow it. It's similar to the whole This hasn't been brought up directly, but I saw a mention of Elixir, so I figure I'll bring it up now. The way that Elixir loads modules is not based on
I didn't know this for a long time, and was confused by the apparent magical loading that Elixir did when I was able to do something like A similar argument could be made against the Ruby/C/C++ system, but at least there I'll eventually find a file include that shows me where to go. Again, I don't think that's worth debating, it's just been my experience with Elixir. This is somewhat unrelated, but comparing To clarify what I think RX14 was getting at with the "This proposal namespaces libraries..." point: If a library is written using For example, assuming # some_library.cr
require 'json'
class SomeThing
end
# end_user_code.cr
SomeLibrary = import "some_library"What namespace does the Even if SomeLibrary = import "some_library"
# does this work? It would with the way `require` works now
JSON.parse("[]")
# or is it namespaced under SomeLibrary? `require` would need
# to change for this to work
SomeLibrary::JSON.parse("[]")
# or is it not available at all? Again, `require` would have to change
SomeLibrary::JSON # => undefined constant SomeLibrary::JSONtl;dr again: |
|
@chocolateboy thanks for sharing the refinements talk. It really gives a refreshing look to that topic. Using refinements in a file level lexical scope is something that could make sense IMO. But there is still "the issue" of the shared global namespace of where that refinement lives. In that sense a require system that is more explicit (and less fun) like ES6 might be more coherent. Finally as highlighted by the refinements presentation, since they are not so widely adopted it might no be clear still if they are a good enough solution. |
|
@faultyserver you absolutely nailed what I was getting at, thanks so much! |
|
One major concern about the current To solve this, maybe we can remove top-level methods and macros altogether. That would force everyone to namespace their declarations. We can't make sure that people will use different module/class names, but chances of collision are now lower. Now, to achieve something like being able to write: get "/" do
endwhich is nice and convenient, we can maybe have the top-level include Kemal::DSL
get "/" do
endThere would be an extra line in these files, which is not that cumbersome, but at least we can sleep without worries at night :-P Because the standard library already defines some pretty common and useful top-level methods and macros like There's still the issue about reopening existing classes like String and adding methods to them, which can bring some conflicts. Maybe we'd need something similar for that: adding extension methods but scoped to a file (I don't think we need this scoped to a module). Anyway, these are just thoughts... |
|
In fact, we could still allow methods and macros to appear at the top-level, but these will always be file-private. These are convenient for short scripts or specs. Hmmm... I might try to implement this and see how it goes, shouldn't be that hard. Then we can really see if it's useful/convenient. |
I don't see how this is any different from using any library in any language.
This proposal has nothing to do with namespaces and doesn't change the semantics of
ES6 supports both, which is why I mentioned it and asked for examples of conflicts between the two usages. Examples include: import 'source-map-support/register'
import 'shelljs/global'
import 'core-js'
Using both is entrenched in JavaScript, in which both monkey patching (i.e. polyfills) and scoped imports (pretty much everything on NPM) are widely used. I've never encountered any "confusion" between these uses. If someone has a concrete example of this, I'm all ears, but at the moment these objections are purely hypothetical and don't reflect real world usage.
I'm more than happy if someone else wants to take a crack at implementing this, but at the moment the proposed implementation doesn't add any complexity to the compiler that I'm aware of. It merely sugars something that can (almost) be done already. |
If you do this, please consider doing it against another ticket. This proposal deliberately avoids namespace-related changes, which are (traditionally) the domain of It would be great if the scope of this discussion could be limited to this proposal and its suggested implementation, rather than going the way of #140, which quickly lost sight of its original topic. IMO, other proposals regarding refinements and namespace changes would be better served by their own separate discussions. |
|
@chocolateboy The proposed implementation doesn't work out of the box. If you return the I'd also like to see some real examples other than just a I think I'm also against having two ways of "importing" code. I understand that JS has two ways, but I believe that's for historical reasons. |
This was the first thing I tested, and it appears to work as expected: test.crrequire "./foo/bar.cr"
test = Exported.imported
puts test
puts test.class
puts test.class.new
puts Command #=> Errorfoo/bar.crprivate module Command
class Runner
def initialize(@shell = "bash"); end
def run(command)
"#{@shell} -c #{command.inspect}"
end
end
end
module Exported
def self.imported
Command::Runner.new
end
endoutputWhat am I missing? |
Sure.
Doesn't Kemal provide a DSL? If so, why would you use |
|
@chocolateboy What if I want to use |
|
It's just that I need a real-world example to consider this (at least me). Because in the previous example we can just have a runner = MyLibrary::Command::Runner.new(...)
runner.cmd(...)Of course you have to be careful to namespace your command runner, but that's what you have to do in every Crystal library. And the amount of typing is more or less the same. |
Use
Have you tried it? test.crrequire "./foo/bar.cr"
class Test
def initialize
@test = Exported.imported
end
def test
puts @test
puts @test.class
puts @test.class.new
# puts Command # Error
end
end
Test.new.testoutput
It's no more or less possible with this implementation since private modules and classes already exist and are used extensively in Crystal's standard library and specs: If there are inconsistencies or other issues relating to private modules and classes, by all means raise them in another issue, but, as it currently stands, this proposal doesn't introduce anything relating to them that isn't already implemented. |
|
But here |
|
@chocolateboy In addition to what @asterite said, using @asterite I really like the idea of having the top level be always file-private, but how would that work with the stdlib? If you add a special modifier to make top-level things be public then we'll probably end up back where we started. |
This is just a reified example of the implementation sketched above i.e.: would desugar to (something like): Are you saying this isn't possible? |
|
@chocolateboy There are always times in which the type inference for class vars isn't possible, so you have to specify the type explicitly. It's often that you want to do this for explicitness. This proposal makes you use |
Feedback on, and criticism of, private modules/classes belong elsewhere (e.g. on #3280), rather than here, since this proposal doesn't introduce or impact that functionality in any way.
By all means close this issue if it is to be superseded by a different implementation, but please take the discussion of that different implementation to a different issue. We already have an example of a derailed version of this discussion in #140. Let's please try to keep at least one version of it focused on the actual proposal. |
|
@chocolateboy Maybe it's possible. But I'm still looking for real examples where one library that you currently use with I'll soon write a proposal about changing the top-level |
No, that's a mischaracterisation spread in this discussion, but which has never been part of this proposal. I'm not proposing any changes to namespaces/visibility or The only thing new in this proposal is the |
|
I mean, if you have: # file: "one.cr"
class One
end
macro imported
One.new
end
# file: "two.cr"
one = import "./one"
# This compiles fine, because I didn't say One was private
One.newIs that expected behaviour? I also find it a bit strange that a file just imports methods, where one would generally like to use types, and possibly refer to types inside that file. How would you handle that case? |
Yes. Just the same as JavaScript: test.jsfoobar.jsThough I doubt this would be the typical use case. |
|
I think the main reason that namespacing is being discussed here is the second line in the original post:
Node's Because Crystal does have that global, shared context, something about the * there is some shared state with |
It's not a convention. Conventions are optional. |
|
@chocolateboy so is this feature |
|
@chocolateboy Wait a minute...if I define a Anyone who reopens |
No. Test and MyLibrary pollute the global namespace, just like Exported in this example. |
Right, but if I'm developing a library, I will be doing everything within some base module for that library, no? This is what @ysbaddaden mentions #4439 (comment). And if I'm just writing a quick script, I don't need to care whether I'm polluting the global namespace, as the script won't be EDIT: Ah, so the point is for library developers not to be forced to introduce a global module at all and instead give them at least the option of introducing nothing globally (except for their dependencies, which must still enter the global namespace if they don't provide The caveat in parentheses above seems to be the main reason why this proposal has been so confusing (at least for me) and controversial...trying to keep both those who prefer an optionally local |
|
Consider a bookstore-vs.-library analogy: Except the I leave the decision on whether this would be a good or bad feature to have up to someone else :-) |
|
@asoffa That's a really good summarization, at least from my understanding of both sides. Thanks for making it so clear :) |
|
@faultyserver I tried :-) |
|
Alright. I was bored and had a spare hour, tried to make something that simulates # main.cr
macro import(file_name, as container)
{% raw_file = `cat #{file_name.id}`.stringify.lines %}
{% body_started = false %}
{% for line in raw_file %}
{% if line.starts_with?("require") %}
{{ line.id }}
{% else %}
{% unless body_started %}
module {{container}}
{% body_started = true %}
{% end %}
{{ line.id }}
{% end %}
{% end %}
# Is there a better way to output `end` to the result of the macro?
{{ "end".id }}
end
import "./other.cr", as: Library
puts Library::SOME_CONSTANT
thing = Library::Thing.new
thing.foo = "hi"
puts thing.foo
puts JSON.parse("[]")
# other.cr
require "json"
SOME_CONSTANT = 2
class Thing
property foo : String?
endwhich compiles just fine and outputs: The output of the macro that gets substituted at the call site looks like this: require "json"
module Library
class Thing
property foo : String?
end
endAs you can see in the example:
It's not exactly the same, but it's close enough imo, and shows that the language doesn't need to change to support this kind of functionality. If people want it badly enough, this could easily be made into a shard and expanded to work better:
In any case (actually more so now), I'm still of the mind that this doesn't need to be (rather, shouldn't be) added as part of the Crystal language/stdlib itself. *EDIT: Actually, private definitions will be hidden by default (private to the containing module, inaccessible from anywhere else), so this point is kind of moot. |
|
inb4 "encapsulation is orthogonal to this proposal": everything I've seen about Not only is that more semantically consistent, it's generally better, as the types defined in the imported file are now available at compile-time and as type restrictions for other definitions. |
|
@faultyserver Nice little concoction there! One small addition to #4439 (comment) if we would want macro import(file_name, as container, public=false)
{% raw_file = `cat #{file_name.id}`.stringify.lines %}
{% body_started = false %}
{% for line in raw_file %}
{% if line.starts_with?("require") %}
{{ line.id }}
{% else %}
{% unless body_started %}
{% if public %}
module {{container}}
{% else %}
private module {{container}}
{% end %}
{% body_started = true %}
{% end %}
{{ line.id }}
{% end %}
{% end %}And a check to disallow cyclic
...but let's see if people are interested before going too far down the rabbit hole :-) |
|
@faultyserver I don't see how that "solves" this issue as it merely moves the global pollution from the callee into the caller: leak.crrequire "./main.cr"
puts LibraryoutputOne could just as easily define the Exported module in this example in
Of course it's possible to "export" a type by polluting the global namespace. Feel free to pursue that in a separate issue or a shard, but it's not what's being proposed here. |
Hence my private-by-default suggestion: #4439 (comment) (though when I try this as-is with the |
...to where the caller has 100% control over the naming of the module, so all naming collisions (one of the "surprises" from the OP, root cause of another one. Third has nothing to do with
The logical converse of these statements:
Why would you import "./main.cr", NotLibrary
puts Library*Doesn't currently work because the nested imports aren't there (because the
That's part of the problem. Why would I want to get back an object instance that I don't actually have type information for? (Crystal doesn't run on a VM, Side note:
Haven't seen one here yet (nor in my years of writing Ruby and C/C++) that isn't entirely avoidable with modularization or the P.S. Would still love to see a PR with a proper implementation of this so we can actually discuss what it does, not what we think it does. P.P.S.: There's an open PR for a |
|
@faultyserver You've already established that you don't have a use for this feature, which may explain why you keep proposing to solve a different problem and trying to pull this issue away from its stated goal. We've already been down that route with #140, but, as I say, feel free to explore those possibilities in a separate issue. |
Hence my "would love to see a PR with a proper implementation". Clearly no one else knows exactly what is being proposed here, so let's get an implementation that we can pragmatically test and discuss. Everything so far has been hypothetic. |
|
Side note: can you clarify how exactly this proposal differs from #140? |
|
|
|
Surely we could conceive that Not that i'm any more for the whole concept in the first place: having 2 ways of doing things is a bad idea, especially this late in the game. |
|
Quick summary, since this is now the 80th comment: Two solutions proposed so far:
value = import "path/to/file"
# `value` now set to whatever the `imported` hook in file.cr returns
import "path/to/file", as: Library
# fresh copy of code in file.cr now loaded into `Library`(1) allows the author (and only the author) to set up a non-global return from a file, whereas (2) allows the user to make the contents of a file non-global (independent of the author's public-vs.-private decisions). Both solve the above problem, but in different ways. Sound about right? |
|
And finally, going in the opposite direction, a third solution to the problem of not being able to use code from another file without introducing/reopening a global referenced by @faultyserver in #4439 (comment):
The proposal here is only for (1) in the previous comment, but it's important to clarify the issue and discuss all options first before deciding on and implementing a solution to the issue |
|
I think you covered everything pretty well, but I wouldn't suggest that either of my proposals be taken into the stdlib. They are proposals for not accepting the feature and working around that decision. Both are prone to random errors as they cheat around proper scoping and rely on assumptions that won't always hold. |
|
Agreed - if we can reduce the issue to a non-issue, we wouldn't (necessarily) want to implement a solution to that non-issue :-) |
|
And combining the ideas from (2) and (3) above, we arrive at # other.cr
require "json"
SOME_CONSTANT = 2
class Thing
property foo : String?
end# main.cr
require "json" # user responsible for explicit `require`s for `Library` below
# ...or not:
{{ `cat ./other.cr | grep 'require '` }}
# add some `import`ant functionality:
private module Library
{{ `cat ./other.cr | grep -v 'require '` }}
end
puts Library::SOME_CONSTANT
thing = Library::Thing.new
thing.foo = "hi"
puts thing.foo
puts JSON.parse("[]")# check.cr
require "./main"
puts Library # error, as desiredThe above is clean enough to allow for sufficiently easy use, yet unclean enough to discourage overuse. Looks like a win-win to me :-) |
|
In the end, most (all?) Crystal library developers will split their libraries across multiple files and use the current |
|
Hi, I like Python When I discover Crystal and learn more Ruby I realize that I can do this: # file src/helper.cr
struct Calculate
def cube_sum(args : [] of Int32)
args.map { |n| n ** 3 }.sum
end
end# file src/calc1.cr
struct MathCalc1
def op_use_cube
Calculate.new.cube_sum([2,3,7])
end
end# file src/calc2.cr
struct MathCalc2
def op_use_cube
Calculate.new.cube_sum([8,1,3])
end
end# file main.cr
require "src/*"
MathCalc1.new.op_use_cube
MathCalc2.new.op_use_cubeI don't need to require explicitly every file because main.cr call all. |
|
I'm closing this. If one day a different way to require code appears, it will come from the core team. Please don't send any more proposals related to this. Thank you. |
Crystal doesn't currently provide a way to control the scope of imported bindings e.g.:
The workaround pollutes the global namespace e.g.:
This is fine in many cases, but can lead to surprises, not all of which are caught by the compiler:
Many languages and platforms provide a way to limit the scope of imports (as well as other conveniences such as renaming them to avoid collisions) e.g.:
In addition, Ruby provides refinements as a way to mitigate this problem.
A method for importing exported values has been proposed before, but the discussion quickly devolved into a defence of
requireand the Ruby culture of monkey patching. As a result, such proposals have (correctly, IMO) been rejected on the grounds that:But there is no need to break/change
require. The two concepts are not mutually exclusive e.g. one can do both in Node.js:I'd like to propose a separate/new method which supports scoped imports, and which complements (i.e. does not replace or override)
require. If this proposal is going to be rejected, it should be rejected in its own right.Implementation wise, there could be a new (file-scoped) hook:
main.cr
command.cr
See Also
require#4368The text was updated successfully, but these errors were encountered: