crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.46k stars 1.62k forks source link

Unsupported system-specific functionality - compile-time or run-time error? #4905

Closed oprypin closed 5 years ago

oprypin commented 7 years ago

Crystal has been gaining support for different platforms, but some functionality is not available on all platforms. Regardless of the outcome, such things should definitely be stated in the documentation. But a decision should be made between one of the two options:

asterite commented 7 years ago

I say it should be a runtime error. Imagine I use a library that in some scenario uses a method that's not available in my platform, but in my specific case, because of some runtime decision (that maybe the library makes) the call will never be made. I don't want the compilation to fail.

I believe every other programming language in the world probably prefers runtime errors in this case too.

bew commented 7 years ago

This Crystal::System indirection was introduced by @ysbaddaden initially, he may have something to say about this! (I'm directly mentioning you to be sure you don't miss this issue)

konovod commented 7 years ago

I would definitely prefer if compiler checks as much as possible at compile-time. IMHO it is easier to add workaround if compilation fails, than to discover that something is unsupported in a hard way and then fix it with trial-and-error (if you e.g. don't have an easy access to failing configuration).

luislavena commented 7 years ago

I would definitely prefer compile-time errors than runtime errors.

Coming from Ruby and working on improving gems to work on Windows, NotImplemented errors hunted my dreams for years.

Crystal already provides mechanisms to require or skip conditional code depending on the platform, so I would expect a library author or Crystal contributor fence the code around platform specific details, simply failing during compilation with there is no such method x or entirely raise platform unsupported (or something like that).

Cheers.

bararchy commented 7 years ago

@luislavena it makes sense , but , thinking about it for a little more time produce this: you cannot cross compile , and you cannot compile code on OSa and use on OSb , it also harms portability , as it means you produce a binery for a system you don't compile on.

RX14 commented 7 years ago

Perhaps the best compromise is runtime errors with a static analysis tool which tells you where the platform specific stuff which will raise is in your code. That way you won't be hunting down these errors forever.

luislavena commented 7 years ago

@bararchy I beg to differ. If the cross-compilation process doesn't acknowledge the specified platform flags, then the cross-compilation process is flawed and needs fixing.

In other languages (C, for example), I can cross compile without issues, link to the specific target platform libraries and obtain the error during linking time when such library doesn't export specific function for that platform.

Moving from that to a non-error scenario that will fail, at runtime, seems not the appropriate approach.

asterite commented 7 years ago

Imagine this:

# some library provides this:

def do_it(fork = false)
  if fork
    Process.fork { do_some_task }
  else
    spawn { do_some_task }
  end
end

(supposing there's a way to fork in the language)

Now, there's no way to fork in Windows.

Now, I build a program that uses that library where I can configure, via an env var or something like that, whether do_it is invoked in a fork or not.

According to many comments here, when I compile this program in Windows, it should error... at compile-time or runtime?

In my program, the only thing I can do is to read the value for fork from the env var, and then, if we are on windows {% if flag?(:window) %} if fork is true we give a runtime error... there's simply no way to give a compile-time error because I don't know if fork will be true or false at compile time, I only know this at runtime.

Maybe the library should be different and provide two methods: do_it and do_it_with_fork. Or maybe that method is invoked, again, via a configuration option. Of course the library can give a runtime error when trying to fork on Windows, but there's absolutely no way to give a compile error when the functionality is determined at runtime.

I might be missing something fundamental here, but how can we make it, in this scenario, to always give a compile error?

lbguilherme commented 7 years ago

@asterite In this case I think this library was poorly implemented and it should do {% if flag?(:window) %} in its own code. Then it can decide to spawn or raise or whatever. But the problem is that if this method generate a error at runtime, then it will always generate such error.

This argument is pretty similar to "NoMethodError" being a compile-time error instead of a run-time one.

konovod commented 7 years ago

The library just won't compile on Windows, so author (or somebody else) will have to fix it by adding a flag.

asterite commented 7 years ago

@lbguilherme Can you change do_it to use your solution?

matiasgarciaisaia commented 7 years ago

do_it, as-is, won't compile on Windows - which is A Nice Thing To Happen™. The shard uses non-Windows code, so it won't work on Windows.

If the author wanted it to work on Windows, it should handle that case - just as you have to handle a nilable variable in your code.

I'd personally change do_it to be:

def do_it
  {% if flag?(:windows) %}
    Process.fork { do_some_task }
  {% else %}
    spawn { do_some_task }
  {% end %}
end

Ideally, I'd love to use {% flag?(:windows) %} inlined with my Crystal code, so I'd only change the if in your version to if fork && {% !flag?(:windows) %} to keep the semantics and allow the caller to choose fork whenever it is available, but it doesn't seem to be supported now. You can do something more verbose:

def do_it(fork = false)
  supports_fork = true
  {% if !flag?(:windows) %}
    supports_fork = false
  {% end %}
  if fork && supports_fork
    Process.fork { do_some_task }
  else
   spawn { do_some_task }
  end
end
asterite commented 7 years ago

@matiasgarciaisaia That last snippet won't compile on Windows because:

Process.fork { do_some_task }

is not guarded by a flag? check.

asterite commented 7 years ago

Or maybe the library should just ignore the fork argument if it's not on windows.

I'm still not convinced that compile-time errors are the best way to do, but we can try and see if it works. If we find a convincing example that can't be worked-around then we can switch to runtime.

RX14 commented 7 years ago

Ideally - for this usecase - if false or if false && <foo> (with false coming from {{ flag?(:windows) }}) would be detected by the compiler and be completely removed from the AST but I seem to remember having this conversation prior and deciding that if false shouldn't mask compile errors.

matiasgarciaisaia commented 7 years ago

I don't know how to implement this, but Crystal's proposal is "If it compiles, it will somewhat probably work". Deferring the error to run time goes against that.

It may very well be that we find cases in which we can't fail at compile-time for whatever reason, but we should try our bests to get that. Having all that type-safety to avoid NullPointerExceptions just to fail at runtime for an unsupported platform doesn't seem The Crystal Way™.

On one hand, having @luislavena's opinion on Ruby/Windows is a strong signal. On the other, having @asterite thinking otherwise is another strong signal :) So I haven't settled on an opinion yet, but feel compile-time error is the way.

RX14 commented 7 years ago

I think it's best to think about what compile time errors actually give us here. If there's a type error in your code, it's a bug and the compiler should error until you fix it. In fact it can't possibly continue without. In the case of platform-specifics, it's not a bug in your code per-se. The most common case for getting an unsupported platform error will be trying something written on unix on windows. In this case it's not a bug in your code, it's a platform incompatibility. So what can you do? In this case you're going to have to add more code to get it to work - and you might not even need the code you're fixing in the first place.

The downside is that you can't be sure whether the executable you just compiled really supports windows or not (really, we should support pretty much everything on both platforms come 1.0), so we should provide a tool which checks and shows you where in your code all the raise NotImplementeds are.

oprypin commented 7 years ago

I think that compile-time checks can work, if the following is done:

ysbaddaden commented 7 years ago

Just my 2 cents: the stdlib does its best to abstract boring platform discrepancies, avoiding the developer to have to deal with them; yet...

If we don't support a feature on some platform (yet), for example Random::System on Windows, I would prefer compilation to fail, not have the impression that it's supported but see it fail at runtime. If a program uses Random::System immediately after starting you'll quickly notice the issue, but what if the call is delayed by some hours? I'd prefer to know that beforehand, at compile time.

Sometimes we just can't support a feature on a given platform. For example fork is unavailable on windows (an maybe other platforms). That's an issue. Maybe:

  1. we shouldn't support fork in Crystal's stdlib —whatever how useful it is, and error prone calling LibC.fork manually can be (and lead to security issues);
  2. we should let Process.fork be undefined on windows, but then platform discrepancies is pushed to the developer. Hopefully, this won't happen much?

Please note that I'd expect to check whether a feature is available rather than maintaining an arbitrary list of platforms to call this or that —just like we don't expect Netscape or Internet Explorer in JavaScript anymore. I'd love a NodeType#responds_to? macro method, to complement NodeType#has_method?, so we could:

{% if Process.responds_to?(:fork) %} # good
  Process.fork {}
{% end %}

Instead of:

{% unless flag?(:windows) %} # bad!
  Process.fork {}
{% end %}
oprypin commented 7 years ago

I think we can all agree that explicitly checking for platform is not good. The check needs to be universal and conceptually simple. I feel that {% .responds_to? %} does not fulfill that, and it could lead to a chain of feature requests for cases it doesn't take into account. It's also not DRY, so to speak.

I'm being fully serious about a {% rescue %} for compilation errors. Fallbacks are the only use case for the checks that we've been discussing, and rescue naturally fits in. You try one thing, and, if it doesn't compile, you do the fallback.

{% begin %}
  cards.shuffle!(Random::System)
{% rescue %}
  cards.shuffle!(Random::DEFAULT)
{% end %}

The only similar tool of this caliber would be an if compiles(arbitrary expression) but it's easy to abuse (same argument as the one against super powered macros), and also doesn't fit into the macro world (needs to access normal code) and doesn't solve anything if it's in the non-macro world.

konovod commented 7 years ago

@oprypin and if in a next version Random::System gets renamed back to SecureRandom (or some other change break compilation), all applications will silently switch to PCG32. explicit {% if flag? :has_system_random %} IMHO is better.

oprypin commented 7 years ago

@konovod, your initial argument sounds compelling but it's an edge case for pre-1.0 and not worth worrying about.


Also just thought of something, if we do the Process.responds_to?(:fork) thing, we can't even properly document the method, because it needs to not be initially defined. And in general, I don't think it's a good idea to allow the public API methods to not be defined.

konovod commented 7 years ago

Well, even after 1.0 there could be rare breaking changes. Another case is that some patch can accidentally introduce compile-time error to the first branch, silently switching app to fallback. This can be partially solved if {% rescue %} will work only for compile-time {℅ raise %}s, not just any error, but it is still error-prone.

RX14 commented 7 years ago

I'm not even sure that a {% rescue %} would be technically possible in the compiler outside of macro raises in the current macro. Unless you want it to be inconsistent and only rescue some compile errors, based on the compiler pass.

luislavena commented 7 years ago

Hello folks,

Sorry for the delay on getting back on this thread, but wanted to take the proper time and provide some notes from my personal experience, which I hope bring some value into the conversation.

@asterite example of fork is a good one, but before jumping directly to it, I would like to present an average workflow with examples, without theorizing things.

I would use C as language of reference and the workflow around local and cross-compilation. Please bear with me since this might be longer than regular comments, apologies for that.


Let's take the simplest program we can think off, a Hello World! one:

#include <stdio.h>

int main(void)
{
    puts("Hello World!");
    return 0;
}

Now, compiling and running this program locally using GCC or clang should be pretty straightforward:

$ gcc hello.c -o hello

$ file hello
hello: ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, BuildID[sha1]=0099440eb5b5d10a4e7d132b3ccff3b2e7ba7c33, not stripped

$ ./hello
Hello World!

With my program working locally, will be great to explore how to get it running on Windows. Thankfully projects like mingw-w64 provides cross-compilers that run on Linux (ie. Ubuntu with apt install mingw-w64).

$ x86_64-w64-mingw32-gcc hello.c -o hello.exe

$ file hello.exe 
hello.exe: PE32+ executable (console) x86-64, for MS Windows

I can now transfer this executable to Windows, or using Wine, run it:

$ wine hello.exe 
Hello World!

With a basic binary working, let's now attempt to compile another example that uses fork:

#include <stdio.h>
#include <unistd.h>

int main(void)
{
    int i;
    pid_t pid;

    fork();
    pid = getpid();

    for (int i = 0; i < 3; i++)
    {
        printf("This line is from pid %d, value = %d\n", pid, i);
    }

    return 0;
}

Compiling and running on Linux, works as expected:

$ gcc simple-fork.c -o simple-fork

$ file simple-fork
simple-fork: ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, BuildID[sha1]=d0aff50666b468844f04fc996a8bb311fa0089ee, not stripped

$ ./simple-fork 
This line is from pid 32371, value = 0
This line is from pid 32371, value = 1
This line is from pid 32371, value = 2
This line is from pid 32372, value = 0
This line is from pid 32372, value = 1
This line is from pid 32372, value = 2

With that working, now trying to cross compile this program for Windows:

$ x86_64-w64-mingw32-gcc simple-fork.c -o simple-fork.exe
simple-fork.c: In function ‘main’:
simple-fork.c:9:5: warning: implicit declaration of function ‘fork’ [-Wimplicit-function-declaration]
     fork();
     ^~~~
/tmp/cc3LNxpR.o:simple-fork.c:(.text+0xe): undefined reference to `fork'
collect2: error: ld returned 1 exit status

First thing presented back is a warning: implicit declaration of fork, meaning that for that platform, fork is not defined.

Second, is an error from the linker: it cannot find a reference to fork.

While the above might frustrate me as developer, I can acknowledge that not all platforms are made equal.


Above is nothing new, this is something Crystal already does at the lower level, when targeting different variations of UNIX systems (ie. LibC.sysconf(LibC::SC_NPROCESSORS_ONLN) on Linux vs LibC.sysctl on BSD variations).

If I decided to use LibC::SC_NPROCESSORS_ONLN directly on my Linux box, will end receiving the correct response:

$ crystal eval "puts LibC::SC_NPROCESSORS_ONLN"
84

But another developer on a BSD system will receive an error instead:

$ crystal eval "puts LibC::SC_NPROCESSORS_ONLN"
Error in line 1: undefined constant LibC::SC_NPROCESSORS_ONLN

The coolest part, this already works:

$ cat test-bsd.cr 
puts LibC::SC_NPROCESSORS_ONLN

$ crystal build --cross-compile --target amd64-unknown-openbsd test-bsd.cr 
Error in test-bsd.cr:1: undefined constant LibC::SC_NPROCESSORS_ONLN

puts LibC::SC_NPROCESSORS_ONLN
     ^~~~~~~~~~~~~~~~~~~~~~~~~

You can say this is the basic of works on my machine example :grin:


Crystal is an application level programming language, as such, it already presents certain abstractions to the lower level details of your OS (like shown above) and I personally believe it should continue doing so.

As a shard author, I might want to interface with certain library or system-level component that might not be available in another platform. The language shouldn't prevent me doing that but, following the same approach, it might error when attempting to compile in another configuration.

Nothing should prevent me from creating a shard that will only work on Linux/BSD, OSX or Windows, or that will only work with certain version of an esoteric third party library.

At a lower level, Crystal already does this internally with LLVM and their different versions.

Now, back to fork example, I see it as two valid questions:

How to know if fork is available?

Julien's suggestion seems excellent. We already have something for instances (Obj.has_method?) but not for class level or lib functions.

As developer doing cross-platform work, I should receive feedback from my build process as early as possible, to help me design and architect the proper solution.

Delay failure until runtime defeats all the benefits of hints (and errors) during build process, which might increase the level of effort and alienate the same developers the language was trying to help.

Some developers might give up supporting platforms as these require more time from them, dealing with testing, VMs, CI and other details.

Should platform-specific code live within Crystal?

Perhaps, or perhaps not. Different languages approach this problem differently.

Whatever Crystal team decides, guarding platform specific code and providing proper documentation should be an integral part of that.


If you reached this point, thank you for patience reading this and looking forward your feedback and comments.

Please take all the above as my personal opinion and in no way as the one and only way of doing things.

Cheers.

akzhan commented 7 years ago

Crystal follows Go way to use coroutines. So fork should be far away from recommended practices somewhere in deephole/syscall.

jhass commented 5 years ago

codetriage The bigger consensus here seems to be that we should strive for compile time errors, even if that means pushing more platform handling code to the developer. We should gather experience with this as we add support for more diverse platforms and eventually support developers with compile time functionality for handling platform differences where the standard library cannot hide them. Discussions about specific ideas for such functionality belong to their own individual issues.

I think we can close this for now.