reasonml / reason

Simple, fast & type safe code that leverages the JavaScript & OCaml ecosystems
http://reasonml.github.io
MIT License
10.13k stars 430 forks source link

Print ternary as if-else #1700

Open chenglou opened 6 years ago

chenglou commented 6 years ago

We can still accept ternaries but print them as if-else.

jordwalke commented 6 years ago

I kind of like ternaries! They're a little shorter. Right now we force-print the { } braces for if/else which makes them a little more verbose for very brief inline comparisons.

List.map(i => i < 0 ? Neg : Pos, list);
List.map(i => if (i < 0) {Neg} else {Pos}, list);

And the if statement requires parenthesis (which are already in abundance).

chenglou commented 6 years ago

I just refactored a code with double ternaries, a pattern I see often in JS land:

foo ? bar ? baz : baaz : qux
foo ? bar : baz ? baaz : qux

Gotta drive folks to pattern matching instead.

Agree that if is slightly visually verbose right now. But your example might only be for the most trivial case. Though that trivial case might also be used often...

Slightly related, I've been thinking about this sugar

List.map(if ($ < 0) {Neg} else {Pos}, list);
kennetpostigo commented 6 years ago

I mentioned this on discord yesterday, can we stop switch -> ternaries and instead make it only happen on if/else -> ternaries?

jordwalke commented 6 years ago

@kennetpostigo Yes, I like ternaries but I agree they probably shouldn't be switches. Although, this is a nice real estate of the syntax that was unused in practice, so it was easy to repurpose switch/true/false for ternaries. The better way would be to use if/else as you suggested but simply tag the if/else with a [@Reason.ternary] attribute which is searched for while printing. Feel free to submit a PR for that if you are inclined.

strega-nil commented 6 years ago

@jordwalke why not just remove the parentheses around the if condition? Then, it becomes

List.map(i => if i < 0 {Neg} else {Pos}, list);

which is pretty reasonable.

edit: probably can't do that without forcing braces around the branch. I think this should be the case, but it would mean a breaking change.

jordwalke commented 6 years ago

Exactly it would mandate braces around the branches. We can define “pseudo backwards compat” for changes that promise not to break any code that has been ran through refmt.

Still with mandatory braces the if/else just won’t ever feel as light weight as ternary to me.

strega-nil commented 6 years ago

@jordwalke imo, this is one of those cases where it doesn't actually need to be as lightweight as it can be - the readability, for me, is what matters here, and if-else is more readable, despite it being a little longer to type out. However, I've been using Rust for years, so that might just be my own thing :)

jordwalke commented 6 years ago

note that if you mandate the parens around the condition (as it is currently mandated), then you can omit the braces around the branches and it is a little more concise.

strega-nil commented 6 years ago

@jordwalke I'd argue that that is terrible style, and also leads to the dangling else issue.

yawaramin commented 6 years ago

Another reason to print if/else: compare the JavaScript outputs of the following:

Reason:

let f(env) = env == "prod" ? 1 : 0;
let g(env) = if (env == "prod") 1 else 0;

JavaScript:

function f(env) {
  var match = +(env === "prod");
  if (match !== 0) {
    return 1;
  } else {
    return 0;
  }
}

function g(env) {
  if (env === "prod") {
    return 1;
  } else {
    return 0;
  }
}

Discovered by @thangngoc89 https://reasonml.chat/t/dead-code-elimination-with-uglify-and-bucklescript/242

chenglou commented 6 years ago

Ok yeah, let's do it. I don't think there's any breaking change here

jordwalke commented 6 years ago

It requires embedding more "style" attributes in the form of @refmt as my pending PR for preserving braces in function bodies. That diff includes a refactor which will make this and many other stylistic improvements easier.

bordoley commented 6 years ago

An interesting factoid, for native code I noticed a non-trivial performance difference between if/then/else and ternaries, in favor of the former, when tuning immutable.re last year. For this reason, I would suggest converting ternaries into if/else statements in the generated ocaml ast.

jordwalke commented 6 years ago

@bordoley Yes, I have noticed this too! I think that if/else would make a much better representation of ternaries and we should move to those. We just want to make sure people's code can be migrated automatically. Would there be issues with parsing ternaries as if statements when they were once switch statements? They should be semantically equivalent.