crystal-lang / crystal

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

Automatic cast breaks playground #6083

Open sdogruyol opened 6 years ago

sdogruyol commented 6 years ago

After #6074 merged, the following works.

x : Int8

x = 1

pp x

However it fails in playground with the following error

ERROR -- : Instrumention bug found (session=1, tag=1).
asterite commented 6 years ago

Yeah, that won't work. I think that gets re-written to:

x : Int8

x = instrument { 1 }

and of course that won't compile.

My suggestion: move the playground outside of the compiler, as a separate project. It will free us from having to track and fix these bugs here.

I really like the playground, but because of how it's currently implemented there's no way it can be exactly the Crystal language. There are many easy ways to break it by now.

j8r commented 6 years ago

Having the playground in the compiler directory looks a bit weird. Creating https://github.com/crystal-lang/playground would be good.

Jens0512 commented 6 years ago

~+1 for moving it to crystal-lang/playground~

Actually, no, I've changed my mind. The playground is fantastic, and it'll probably be doing better where it is than where it'd be by itself.

RX14 commented 6 years ago

I don't see much benefit of splitting it out. It's not crystal but it's hardly icr-levels of hacks and works as a good first approximation. Removing it is just detrimental to newcomers for purism's sake.

asterite commented 6 years ago

Who from the core team uses the playground? Or someone here that uses the playground daily?

bararchy commented 6 years ago

@asterite I do, also @artlinkov its great for prototype and testing new ideas and types

j8r commented 6 years ago

In my opinion, a "standard playground" has little sense, it's a separate project of the Crystal compiler. It has its own HTML views, has its JS libraries in public with also CSS. And perhaps in the future it will use some npm packages with a package.json, JS linters/tests frameworks...

Tere are so many awesome tools like ameba that will fit better in the compiler tools, but they aren't integrated and it is fine.

asterite commented 6 years ago

Not to mention that the compiler depends on HTTP::Server for this whole purpose, and it's why we have all that without_openssl mess, because we can't compile the compiler and easily statically/dynamically link openssl. It would simplify things so much.

jwoertink commented 6 years ago

I don't see removing it as scaring away newcomers. If the newcomers are going to go to the docs anyway, and the docs say "Hey, we have a playground, and here's how you install, run, and play with it!", then it's not really a huge deal. Plus, I think it invites more people to work on the playground because PRs won't need to worry about running the entire compiler test suite to contribute.

luislavena commented 6 years ago

IMO we should ship playground with releases, but not necessarily have it part of the compiler itself. Similar to how shards is built and bundled with a new release, the same should be for playground.

My 2 cents.

Cheers.

RX14 commented 6 years ago

If we ship it with the release that would mean we still need to have -Dwithout_openssl.

I strongly vote keep it.

j8r commented 6 years ago

Let's do a poll for the future of playground: :+1: Move it out to https://github.com/crystal-lang/playground :-1: Keep it in the compiler as is - don't change anything :confused: Something between the two last propositions (e.g. moving in its repository but integrate it to the compiler somehow)

luislavena commented 6 years ago

If we ship it with the release that would mean we still need to have -Dwithout_openssl

Yes, but just for crystal-playground binary, as crystal itself will no longer require SSL to work.

While Crystal still don't have a IPC or plugin mechanism, nothing stop us from having a shim command that invokes the playground binary (ala git with libexec's git-something binaries)

The compiler source code is always available to any project, so playground can be split from the codebase, maintained and build independently.

If the concerns are around the time will take to build playground and compiler in release mode, then we can benchmark the efficiency of the bc+obj cache of the two different projects accessing the same classes.

Cheers.

j8r commented 6 years ago

@RX14 , @bcardiff or @asterite, could you move out the playground to https://github.com/crystal-lang/playground – most of the community agree on this. Then we could eventually open an issue on the new repo for if we want to integrate it, and how, to the Crystal compiler release.

bcardiff commented 6 years ago

I would like to see this done only and as long as the compiler is extended to let the program be instrumented in a better way as it is today. Otherwise changes to the language or the the ToSVisitor are too sensible to break the playground.

As pointed out, taking out the playground will result in bigger binary to distribute and specially some more distribution tasks that should be deferred since there was already a lot of work in distribution/infrastructure tasks.

RX14 commented 6 years ago

Exactly, just because the "community agrees" on something doesn't mean it's actually a good idea.

RX14 commented 6 years ago

To get back on track - why is the code rewritten to

x : Int8

x = instrument { 1 }

instead of

x : Int8

__tmp437 = x = 1
instrument(__tmp437)

?

It seems to me this would fix a lot of bugs in the playground

bcardiff commented 6 years ago

seems a good rule, yet multi assignments wont work with that rule. but we can define different rules for multi-assign (there are already some custom rules for puts and print)

The instrumentation until now avoided adding new variables.

RX14 commented 6 years ago

seems a good rule, yet multi assignments wont work with that rule. but we can define different rules for multi-assign (there are already some custom rules for puts and print)

sounds good

The instrumentation until now avoided adding new variables.

We add new variables with unguessable names all over desugaring and in macros, so this is acceptable.

v9n commented 6 years ago

One to throw my opinion about spliting the Playground. The other day I want to take that playground for something else(basically an devops tool to write runbook for oncall and execute from playground) then I realize it depends on the compiler repo somehow and cannot easy to split it out to compile and run on its own :(.

RX14 commented 6 years ago

@v9n it'll always depend on the compiler whether or not its in a seperate repo. The crystal compiler is part of the stdlib however, so you should be able to require parts of the playground from any crystal program right now with no dependencies

bcardiff commented 6 years ago

@v9n you can build the playground (or the compiler) and tweak things. But maybe you can use the playground workbooks for your needs. There in a link in the navbar explaining a bit of them.

In a folder were you will start the $ crystal play you can create a playground subfolder and drop there .cr .md .html files that will render nicely. Each code section in .md with "``playground" or

in.html` files will render the interactive playground. You can have multiple per files.

v9n commented 6 years ago

@RX14 @bcardiff That's awesome.

vlazar commented 5 years ago

The error shown in playground is different now

error in line 2
Error: type must be Int8, not Int32
Crystal 0.31.1 on MacOS ``` $ crystal --version Crystal 0.31.1 (2019-10-02) LLVM: 8.0.1 Default target: x86_64-apple-macosx ```

If I change the code to this (added _i8)

x : Int8

x = 1_i8

pp x

The output in playground is

1 : Int8
1 : Int8