Compiler: fix closured instance variable not being considered as such

[asterite]

Hopefully fixes #5609, take 2

[asterite]

It doesn't, but the approach is a good start... to fix this I'll also have to fix #3093 . I'll see if I can do that this week.

[asterite]

Actually this is already pretty good and #3093 might not be necessary to go with this fix, though it is a small breaking change because in some cases the compiler won't realize that a closured var doesn't actually change, like before, but in cases where it previously (incorrectly) didn't.

I'll still try to think of a way to do #3093 but if I can't we should at least go with this fix because #5609 is pretty nasty (code compiles and runs, but runs incorrectly).

[asterite]

It worked!

I think we should merge this now, even though this is a small breaking change. In the compiler and std source code I had to change two places to make it work well.

Basically, previously the compiler would check whether a variable was closured and bind all assigned types to it regardless of if is_a?, type filters and location, but it didn't do that for variables used before the closured variable was detected. Now it works fine, but this means that in some cases the compiler will give a correct-ish error where it previously didn't. There's still the issue that the compiler should be smarted about immutable closured vars ( #3093 ), this change just makes this be triggered in more (correct) places.

[straight-shoota]

The breaking change seems very counter-intuitive to me. The change in process.cr shouldn't be necessary, really. The stdio is only closured in other branches. There is no way it can be anything else than IO::FileDescriptor in a IO::FileDescriptor branch.
I'm not sure we should accept that unless #3093 is fixed as well.

[asterite]

Strange, it works fine in mac but not in linux... no idea why.

[asterite]

I guess nevermind, I don't know what's going on...

[asterite]

As a follow up, I'd really like to go with the three first commits. It's a breaking change, and some places will start failing to compile where previously they didn't, but I prefer that to a segfault or incorrect behavior at runtime. We can make sure no bad semantic is generated first, then we can worry about making it work for cases where it doesn't but should.

I won't do it, but feel free to do that for 0.33.0.

[asterite]

Could someone using linux try running the specs locally? They work fine in my mac, and linux CI is having issues lately so I'm not sure it's related to these changes.

[RX14]

@asterite the linux CI issues are #7177

[asterite]

@RX14 thanks, but this PR was failing in linux before that started happening. That's why I'd like someone to run the entire spec suit (with a compiled compiler too) in Linux on their machine, not on CI.

[konovod]

Under WSL (not exactly supported target, but what i have.)
(this PR)

user@laptop:/mnt/d/Projects/crystal/crystal$ ./bin/crystal build --threads 1  --exclude-warnings spec/std --exclude-warnings spec/compiler -o .build/std_spec spec/std_spec.cr
Using compiled compiler at .build/crystal
user@laptop:/mnt/d/Projects/crystal/crystal$ .build/std_spec
.build/std_spec: relocation error: .build/std_spec: symbol __gmon_start__, version OPENSSL_1.0.1 not defined in file libssl.so.1.0.0 with link time reference

master branch:

user@laptop:/mnt/d/Projects/crystal/crystal$ .build/std_spec
.build/std_spec: relocation error: .build/std_spec: symbol __gmon_start__, version LIBXML2_2.4.30 not defined in file libxml2.so.2 with link time reference

release/0.32 branch:
works

[asterite]

Thanks! I'm still not sure if it's a problem with this branch or not... it's so strange that it works just fine in darwin.

Maybe someone with real linux and not WSL?

[konovod]

FWIW I've tried lld-8 as suggested in #7177 (comment) (still on WSL)
Now master works and this PR shows

Using /usr/bin/llvm-config-8 [version=8.0.1]
./bin/crystal build  --exclude-warnings spec/std --exclude-warnings spec/compiler -o .build/std_spec spec/std_spec.cr
Using compiled compiler at .build/crystal
.build/std_spec
.Invalid memory access (signal 11) at address 0x7fae5e8c9f88
[0x11b1bd6] *CallStack::print_backtrace:Int32 +118
[0xbdf749] __crystal_sigfault_handler +249
[0x2a568d7] sigfault_handler +40
[0x7fae58f91390] ???
[0x7fae5e8c9f88] ???
Makefile:100: recipe for target 'std_spec' failed
make: *** [std_spec] Error 11

[asterite]

Thanks! Then yes, there's something wrong in linux. Unfortunately I won't be able to continue working on this to find our what's wrong, but if someone wants to debug it please go ahead.

[RX14]

@asterite the error happens when you exceed a total number of types (really, llvm modules) in the program. Any new PRs which add more generic instantiations or types can trigger this. The linker corrupts the object file and you'll get a runtime link error. It works in darwin because osx uses lld by default.

[asterite]

@RX14 but if you look at the last output provided a couple of comments above you'll see the specs start running, one passes and then it crashes. Do you think that's related to what you are saying?

[RX14]

Not the version compiled with lld, no, that's another bug. I was still talking about the CI failures.

[asterite]

This didn't work, so closing.