Remove Crystal::System, do system-specific methods by reopening

[oprypin]

First of all, I don't expect this PR to be easily merged without discussion, I acknowledge that putting platform-specific functionality into the Crystal::System module/namespace is an important decision of the core team. The contents of the PR is more of a preview of what it could look like when Crystal::System is gone, assuming "Unsupported functionality is a run-time error" just because it looks cleaner regarding docs.

I felt that it is important to start this discussion because of some unnecessary confusion caused by this separation during the implementation of #4903 and also #4882 (comment), and @asterite's opinion in #4903 (comment) increased the confidence.
Initial discussion in IRC

Arguments against Crystal::System:

• Encourages unnecessary meaningless objects
• A lot of duplicated and wrapped methods (can be clearly seen in #4903)
• Can have worse performance due to composition of classes and methods
• Confusion for implementers:
  • on whether the system-specific type should be a class/struct/module
  • on where to draw the line between internal and public API
• Exposes internal implementation details as a public API (also need to be careful with :nodoc:)

Arguments for keeping Crystal::System (partly quoted from @RX14):

• Easier to document the methods in a centralized manner
• More obvious which methods are platform-specific (well, not if they're marked by raise NotImplementedOnPlatform or whatever)
• Allows starting with an empty prelude and requiring crystal/system without the rest of the stdlib
• The other option could be hacky (e.g. reopening, also this PR relies on require order though it's not the only way)
• Just keeping things as is

Also note that this PR contains only one of the possible implementations. The other one is to keep the redirecting stub methods as they are right now and enforce all platform-specific files to contain only private methods. That would not force us to change the errors to be run-time [fix #4905] in order to keep documentation in one place.

[txe]

Actually, I like this PR. I never understand the reason for long naming like Crystal::System::Random::something_else and become confused when I see many different namespaces that look the same such as System::Random and Random::System and others.

Also, I like the idea how to keep platform-specific functionality. Speaking about Dir/DirHandle example,
Dir became a dull wrapper for DirHandle (as @oprypin said) and it doesn't make much sense. The new approach removes that issue for certain.

[asterite]

I also like it. I always was in favor of not introducing modules that abstract the underlying implementation and directly defining the functionality in the needed types. It ends up being simpler in my opinion.

[RX14]

I don't mind this either, as long as we keep the platform-specific stuff in src/<something>/<platform>/foo.cr. I think that the Crystal::System has the advantage of making you think a lot harder about the interface between the platform specific code and the rest of the stdlib, which makes implementing things that way harder. I still like Crystal::System more (I'm biased because I have PRs which depend on the old layout) for this reason but objectively both sides have their pros and cons and I think they're pretty evenly matched.

[Fryguy]

Out of curiosity, how would this affect documentation? That is, I'm a newbie, I want to know what Random::System.random_bytes does, and googling would bring me where and for which platform?

[oprypin]

(I thought) this would not affect documentation, you'd still see it like this https://crystal-lang.org/api/System.html

But well... apparently overriding a documented method with an undocumented one discards the documentation. Which could be changed, honestly...

[ysbaddaden]

It looks fine.

I liked the simple sys namespace in Rust, which consists of simple structs that abstract the underlying platform discrepencies, then have std built on top of sys.

As long as we keep abstracting the underlying platform discrepencies, I guess reopening fits better with Crystal than external modules, especially since they shouldn't be used directly —unlike sys in Rust.

The one thing I don't like in this proposal is replacing compile time errors with runtime exceptions. Methods could be marked abstract instead. That wouldn't solve the documentation issue, thought, since overriding a def replaces the previous def, including it's documentation.

A solution could be to have private <method>_impl definitions instead; another to copy the documentation of a previous def when the new one isn't documented?

[oprypin]

Also note that this PR contains only one of the possible implementations. The other one is to keep the redirecting stub methods as they are right now and enforce all platform-specific files to contain only private methods. That would not force us to change the errors to be run-time [fix #4905] in order to keep documentation in one place.

[bew]

for the documentation issue, we could maybe add a :previous doc: or something like that to prevent documentation override? (or that would be replaced by the previous doc?)

# This is the doc
def method
end

# :previous doc:
# Add some additional details if needed? (<-- not sure about that)
def method
end

[RX14]

My problem with the "only private methods" approach is that it's too similar to what we do currently - without the benefit of the added isolation that Crystal::System provides.

[mverzilli]

I'd suggest that we leave the documentation issues out of this discussion. Once we settle on the best approach for system-specific stuff, we can identify whatever gaps need to be addressed in the documentation system and track them in their own issues.

Whatever we decide, something we need to keep in mind (which was discussed also in #4406), is that it needs to be easy to identify which parts of the code are platform specific, and it would be cool if the structure would guide people implementing new platforms, like @ysbaddaden said here: "Porting Crystal to support more platforms could be achieved by adding any required C bindings to src/lib_c then implement the API for the platform under src/sys —in the hopeful case that it won't require changes in the Sys API."

To that end, do you think adding a "platform_specific" folder under "system" could help? For example, in this PR, every file that now is under src/crystal/system/unix/ would instead move to src/crystal/system/platform_specific/unix/, e.g.:

src/crystal/system/unix/arc4random.cr becomes src/crystal/system/platform_specific/unix/arc4random.cr

[oprypin]

in src/system.cr:11: can't define abstract def on metaclass

  abstract def self.hostname

[mverzilli]

@oprypin would you elaborate on what confuses you? Is it the part about leaving the docs discussion out of this issue, the part of adding a platform_specific folder to group all the platform specific code, is it the whole comment... :P?

[oprypin]

@mverzilli
Your comment basically describes what we already have in place as if it's something that needs to be done. This PR does not do anything against it.
Second half of your comment is confusing because /crystal/system/ already is the platform-specific folder.

[mverzilli]

Thanks for clarifying! Don't want to derail the conversation, so let's leave the doc part there, I guess we're on the same page then.

About the /crystal/system part: currently we have src/crystal/system/time.cr and src/crystal/system/random.cr. If in the future we continue adding stuff there that we might want to group in folders, we'll end up with folders src/crystal/system/unix, src/crystal/system/windows and src/crystal/system/something_else_that_isnt_platform_specific. But maybe this is just a bit of a librarian OCD kicking in and I should just stop talking non-sense.

[oprypin]

What you did make me realize is that the crystal/system folder name stops making sense with these changes. So I'll add some folder renames to the mix.

[RX14]

I still don't understand the event in which something not platform specific would end up in crystal/system.

[oprypin]

Solving the issue with documentaton in #4908

[bew]

Why did you put the doc under the method? (And not above)

[oprypin]

This is a platform-specific redefinition of a method. It must not override the documentation that is meant to be public. Implementation details shouldn't surface there anyway.

[bcardiff]

I think that something sane to expect is to avoid different representation of the std in different platforms. Using wrappers in Crystal::System namespace is a way to accomplish this. If we rely on reopening classes there will be no structure to prevent that.

I want to go through the arguments against the crystal::system namespace to expose why I am more inclined to keep them, at least for now.

Arguments against Crystal::System:
Encourages unnecessary meaningless objects

Some abstraction is needed. And I don't think the std should resolve/implement the abstraction, but rely on it.
There are some stateless modules like Crystal::System::Time which I don't think they add complexity. Rather the oposite, but is just my way to see it.

A lot of duplicated and wrapped methods (can be clearly seen in #4903)

I don't see how reopening classes would be better. All overridden methods will look as duplication.
In the DirHandle PR it looks like the Dir class might be a bit meaningless from the PR, but the Dir in std will deal with the rest of the api that a programmer would expect.

This overlaps with the previous item, but I think is good to have platform specific DirHandle. The alternative would be to allow a struct pointer of each platform to be in Dir, making the representation a bit messy.

Can have worse performance due to composition of classes and methods

Was this measured?
Given the kind of code in Crystal::System I guess that an inline should be enough to avoid performance differences. But I am not so sure/experienced here.

Confusion for implementers:
on whether the system-specific type should be a class/struct/module
on where to draw the line between internal and public API
Exposes internal implementation details as a public API (also need to be careful with :nodoc:)

These applies whether the Crystal::System exists or not. I don't see a way to mitigate them other that code review the additions.

The thing that bugs me most of the current state is all the commented def that appears as intention of internal api for the abstractions. Those are a source of confusion since it happen only there. A way to solve it would be to allow the base implementation to raise, but that would move compile time to runtime error for missing parts of the platform. Maybe is not that bad, but is harder to track.

[wmoxam]

I mostly like this. I find it much easier to understand and think others will too.

The one piece that bothered me is runtime errors for unimplemented methods ... but perhaps they are only intended for developers who are porting Crystal to new systems?

[oprypin]

In the current state of the PR the errors are compile time

[ysbaddaden]

Errr... I was OK with the initial pull request, except for the runtime errors, but now I'm puzzled:

1. Why the move from crystal/system to the terrible platform_specific folder?

2. This is secondary but I'm not fond of pushing requires to the bottom of files. I know this is to overwrite the abstract methods, but still, we're burying a dependency.

[oprypin]

@ysbaddaden the folder name crystal/system reflected the namespace, but that namespace is gone and the name becomes kinda arbitrary (makes no sense to me; not clear what it represents).
#4906 (comment) made me notice that.
But I don't care much about this and would easily revert it.

[RX14]

I think the true issue uncovered by this PR is that there's a big difference between heavily platform-specific classes (IO::FileDescriptor) and lightly platform specific classes (Time). In the case of lightly platform specific classes it makes much more sense to simply do as Crystal::System is currently, and abstract methods only. The approach in this PR is better for highly platform-specific classes.

Perhaps the best way to compromise is to just export modules in Crystal::System that we use as mixins to IO::FileDescriptor. That way we don't need to change structure, and we can get the benefits of this PR.