fable-compiler / Fable

F# to JavaScript, TypeScript, Python, Rust and Dart Compiler
http://fable.io/
MIT License
2.93k stars 301 forks source link

Repl regression #1269

Closed ncave closed 6 years ago

ncave commented 6 years ago

Repl is currently broken, most probably by #1265.

alfonsogarciacaro commented 6 years ago

Damn, I really need to add the REPL to CI 😬

alfonsogarciacaro commented 6 years ago

I'm about to merge #1271 an it seems the JS version is able to compile the Sudoku sample with the current code, how can we test this regression?

ncave commented 6 years ago

@alfonsogarciacaro Did you change something from #1265? It was definitely not working before.

alfonsogarciacaro commented 6 years ago

Nothing in particular I can recall, during the tests for #1271 at one point I commented out the suspect lines but I brought them again after confirming they weren't causing any problem.

One noticeable change though is I converted the test_script.fsx into the module test_script.fs because I was getting an "unexpected behavior" error. Is that what you were seeing? What was broken exactly? The FCS/Fable compilation itself, or compiling F# code to JS with the REPL?

ncave commented 6 years ago

@alfonsogarciacaro Idk, it works now, I guess you fixed it :)

alfonsogarciacaro commented 6 years ago

Ok, let's call it a fix then! 😄 Hopefully now that the REPL is being built by CI I won't be breaking your work as many times as before :wink:

alfonsogarciacaro commented 6 years ago

This is not surprising but you were right :wing: For some reason, the test I added to CI passed fine, but when trying to do autocompletion in the repl there were errors, commenting out the Assignments active pattern does prevent the error.

ncave commented 6 years ago

@alfonsogarciacaro Yes, I can confirm that commenting that Assignments active pattern (above) fixes it. I would say just take it out, at least temporarily until it can be fixed, and close this issue.

alfonsogarciacaro commented 6 years ago

Ok, #1284 should fix it. I was interested in having this optimization as it removes a lot of the generated closures to put assignments in expression position. Hopefully this also helps improve the performance of the REPL.

Note to self: don't expose mutable fields, you can't control them ;)

ncave commented 6 years ago

@alfonsogarciacaro Thanks, that fixes it.