Skip to content
This repository was archived by the owner on Jun 15, 2023. It is now read-only.

Introduce guard syntax #20

Closed
wants to merge 1 commit into from
Closed

Conversation

tomgasson
Copy link
Contributor

This introduces syntax sugar to allow early return. This comes in 2 forms:

Conditional....

let safeDivideZero = (num, denom) => {
  guard denom !== 0 else {
    0
  }
  num / denom
}

And pattern based....

let getSnippet = (message) => {
  guard let Some(author) = message.author else {
    "Someone sent you a message"
  }
  author ++ ": " ++ message.body
}

Which de-sugar into these respectively

let safeDivideZero = (num, denom) => {
  if denom !== 0 {
    num / denom
  } else {
    0
  }
}

and

let getSnippet = (message) => {
  switch(message.author){
    | Some(author) =>  author ++ ": " ++ message.body
    | _ => "Someone sent you a message"
  }
}

This reduces rightward drift and nesting, and allows top-down control flow through preconditions

Using a guard statement for requirements improves the readability of your code, compared to doing the same check with an if statement. It lets you write the code that’s typically executed without wrapping it in an else block, and it lets you keep the code that handles a violated requirement next to the requirement.

Precedence

reasonml/reason#1302
Swift

guard /* i1 */ denom /* i2 */ !== /* i3 */ 0 /* i4 */ else {
0
}
// t5
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs fixing

@chenglou
Copy link
Member

chenglou commented Jul 6, 2020

Postponing this, as this is purely additive and we need to focus on tweaking some things that'd break code if we don't ship now, e.g. list{.

(Latter seems to be causing some nontrivial regression in the benchmarks)

@bobzhang
Copy link
Member

bobzhang commented Feb 3, 2021

@tomgasson We are interested in this PR. Is there anyway to avoid introducing a keyword guard?

Edit: seems assert is already a keyword. how about this:

assert denom ! = 0 else {
  body
}

@chenglou
Copy link
Member

chenglou commented Feb 3, 2021

The assert is neat. However guard was done considering also the similar feature we might add:

guard let Some(a) = myOptional else {
  // "early return" logic
}
...

Which is the cousin of:

if let Some(a) = myOptional {
  // "early return" logic
}
...

I wonder if assert let ... sounds as right

@bobzhang
Copy link
Member

bobzhang commented Feb 4, 2021

https://stackoverflow.com/questions/32256834/swift-guard-let-vs-if-let
See this discussion, it seems only the benefit of guard let justifies the complexity.
With regard to the concrete syntax, guard may look nicer but you have to pay. While introducing assert let does not cost anything except bringing more convenience.

@chenglou
Copy link
Member

chenglou commented Feb 4, 2021

Not the same for us though; Swift handles null in a more conventional way, so if let is slightly redundant for them. For us, nested nulls would be solved by if let:

if let Some(student) = payload.student 
  let Some(score) = student.score {
  Js.log(score)
} else {
  // ...
}

@bobzhang
Copy link
Member

bobzhang commented Feb 4, 2021

For people who are interested in this PR, this answer explains the motivation very well
https://stackoverflow.com/questions/32256834/swift-guard-let-vs-if-let/38356532#38356532
it serves as a sugar for fp's early return

@jfrolich
Copy link

jfrolich commented Feb 4, 2021

Not the same for us though; Swift handles null in a more conventional way, so if let is slightly redundant for them. For us, nested nulls would be solved by if let:

if let Some(student) = payload.student 
  let Some(score) = student.score {
  Js.log(score)
} else {
  // ...
}

How would that be better than switch + pattern matching? (The stated example would probably be nicer with that I think?).

switch payload.student {
| Some({score: Some(score)}) => Js.log(score)
| _ => // ...
}

@chenglou
Copy link
Member

chenglou commented Feb 4, 2021

Destructuring on a record wouldn't work with more elaborate examples. Perhaps I should have taken another example. switch you'd end up with multiple nested layers:

switch payload.student {
| None => // ...
| Some(student) =>
  switch process(student.score) {
  | None => // ...
  | Some(score) => Js.log(score)
  }
}

vs

if let Some(student) = payload.student 
  let Some(score) = process(student.score) {
  Js.log(score)
} else {
  // ...
}

There are drawbacks too of course. Mainly:

  • Extra syntax. We're short on syntax and cognitive estate.
  • Isn't really better if the None cases need different handling
  • Potential abuse of if let Some(Result(a)) = payload and then the else case being a bad catch-all

That's part of why if let got postponed.

Anyway, I've added the context as a reply to Bob of a few other things to maybe consider. Or not!

@IwanKaramazow
Copy link
Contributor

@bobzhang Can you elaborate a bit on what you mean by the cost of the guard keyword? Implementation of the parser? Cognitive load for the user? "Bigger" syntax? Efficiency?

@bobzhang
Copy link
Member

bobzhang commented Feb 4, 2021

@IwanKaramazow
An in-complete list of cost:

  • It breaks existing code which use the word.
  • It add more cognitive load even for people who dont use such feature, because he can not define such variable any more.
  • It makes some part hard to express, now guard can not appear in record any more.
  • Tools have to be adapted, e.g, syntax coloring etc.
  • From branding side, it makes the language design not serious. If you look at the evolution of C/C++, to add a new keyword, the benefit has to be phenomenal to have everyone on board.

In general, I think we should hold a high bar when considering adding new keywords. Otherwise, whenever we plan to add a feature, we can always introduce a new keyword, it is not sustainable.

@chenglou
Copy link
Member

chenglou commented Feb 4, 2021

Yeah I think if we ever accept this feature, assert is fine

@cknitt
Copy link
Member

cknitt commented Feb 4, 2021

I do not think assert should be used for this. I believe people knowing assert from basically any other language would expect it to interrupt the program / raise an exception in case of an error.

@tomgasson
Copy link
Contributor Author

I like the scrutiny here, that high bar is what makes the language great.

I don't have any other suggestions, but not too keen on assert.

  • Being able to put an else on assert removes the meaning of assert.
  • assert is signifying something went wrong before here in execution, but this is about the logic belonging to this function, not prior
  • Depending on the language, assert can often be stripped in prod and might be mistaken as not having relevance to the running of your code

@bobzhang
Copy link
Member

bobzhang commented Feb 4, 2021

Assert without else will abort as before. With else it will go the other branch.
This matches the semantics of ‘guard if’ where else is an uncommon path

@bobzhang
Copy link
Member

bobzhang commented Feb 4, 2021

note if we can figure out a better syntax without introducing a new keyword, I would be happy to see it. The assert seems to be the best candidate here

@tomgasson
Copy link
Contributor Author

Assert without else will abort as before

I didn't call it out clear enough but my implementation here doesn't line up with your assumption there.

This implementation has guard x meaning the same thing as guard x else { () }. That would lead to a difference

Classical assert():

denom code behavior
1 assert(denom != 0) noop, continues execution
0 assert(denom != 0) raise(AssertionFailed)

Using assert instead of guard here (ignore the parens):

denom code behavior
1 assert(denom != 0) noop, continues execution
0 assert(denom != 0) return unit, stop execution
1 assert(denom != 0) else { body } noop, continues execution
0 assert(denom != 0) else { body } return body, stop execution

@bobzhang
Copy link
Member

bobzhang commented Feb 4, 2021

Ah, what I proposed is a generalized assert. I will write down the semantics later

@bobzhang
Copy link
Member

bobzhang commented Feb 5, 2021

Here is my proposal of generalized assert.
In the language, only asssert false is treated specially and it can be unified with any type in the type system.

  • special case:
assert predicate ; 
follow_body

The semantics is equivalent to

if not predicate {
   assert false // this type checks since `assert false` unifies with any type
} else {
   follow_body
}
  • generalized assert with else
    The else branch defaults to assert false, but user can override it
assert predicate else { console.error("BOOM"); exit(1)}
follow_body

The semantics is equivalent to

if not predicate {
  console.error("BOOM");
  exit (1)  // type a
} else {
   follow_body // type b
}
// type a should unify with type b
  • generalised assert with let
assert let Some (x) = x ;
follow_body

rewrite to

switch x {
Some (x) => follow_body
_ => assert false
}
  • generalized asssert with let and else
assert let Some (x) = x else { other_expression}
follow_body

rewrite to

switch (x) {
Some (x) => follow_body
_ => other_expression
}
  • Nested assert
assert let Some (x) = x else {exp_x}
assert let Some (y) = y else {exp_y}
follow_body

rewrite to

switch (x){
Some (x) => {
    switch(y) {
      Some(y) => follow_body
      _ => exp_y
    }
   }
_ => exp_x
}

This illustrates how indentation hell is solved using assert let, the exceptional cases are put in the beginning

@IwanKaramazow
Copy link
Contributor

IwanKaramazow commented Feb 5, 2021

The generalized assert looks quite nice and natural. A couple of code examples:
image

Compared with the current code: edit: see chenglou's post below

@cknitt what do you think of these examples?

@chenglou
Copy link
Member

chenglou commented Feb 5, 2021

The proposal looks enticing. However I'm not sure @IwanKaramazow's examples work out. Here's what the examples would look like currently:

image

  • safeDivide doesn't need assert
  • submit looks much easier with pattern matching on tuple (same perf here, no tuple alloc)
  • getChangesCount should flip its predicate anyway

The sweet spot might be a long function body with a single assert or two at the top...? If that's worthwhile.

@cknitt
Copy link
Member

cknitt commented Feb 5, 2021

I think the early returns instead of indentation hell are really great! 👍

My concern is just with the keyword assert plus the fact that it now has two meanings / use cases:

  1. early return when a check fails
  2. crash the program when a check fails

I think for newcomers, this may be confusing, and they may forget the else and end up inadvertently crashing the program.

Experienced developers, on the other hand, when reading the keyword assert, will (at first) expect it to have the second meaning.

For me, when I see those asserts in the code, I think they will probably make me nervous every time and cause me to double check that I didn't forget the else...

That's why I would prefer the guard keyword (which people will also know from Swift with the same meaning).

Dumb question: Why do we need the assert (the one that crashes the program) at all in ReScript? In a web app, I do not see a use case for it. In a Node.js app, I might as well bind to the Node.js assert function.

So in short my preference would be to have just guard for early returns and no assert at all. 🙂

@Kingdutch
Copy link

Kingdutch commented Feb 5, 2021

I keep somewhat getting tripped up by both examples (with guard and assert). Is the keyword <negative predicate> else { <early_return> } really needed? Why not just keyword <predicate> { <early return> }

I was wondering why it kept feeling like mental gymnastics, but that's because it's read as: "guard that something is true but otherwise return this" which translates to "if this is false the next line runs". Whereas every other if statement is that "if this is true this piece of code runs".

Ruby has the unless keyword (although it's used postfix, e.g. return 0 unless denom !== 0). That could help here too:

let safeDivideZero = (num, denom) => {
  unless denom !== 0 {
    0
  }
  num / denom
}

And pattern based....

let getSnippet = (message) => {
  unless let Some(author) = message.author {
    "Someone sent you a message"
  }
  author ++ ": " ++ message.body
}

This still uses the if it's false (which I suppose is needed to make the let ... work) but also clearly says so at the start of the line.

EDIT: I suppose some of this is covered by Eric Cerney's article linked in the OP but I thought I'd share anyway.

@alex35mil
Copy link

I definitely agree with @cknitt. assert triggers me and guard might be a better choice since it does what it does in Swift afaiu. Not a deal breaker though and it's great to have an option to early return regardless how it's worded.

It also would be very convinient if guard myOption ... would early return None if it's the value of myOption and guard myResult ... would early return Error(err) if it's the value of myResult. Personally, I don't use booleans much in ReScript code and it would force my to convert optionals/results to booleans every time I want to guard. Unfortunate that ReScript doesn't have a way to implement shared behavior in abstract way to make it as usable as Rust's ?.

@bobzhang
Copy link
Member

bobzhang commented Feb 9, 2021

Is the keyword else { <early_return> } really needed?

It is keyword predicate else {}. The negative is used to explain that the assertion is a generalised concept which is consistent with the current semantics

@bobzhang
Copy link
Member

bobzhang commented Feb 9, 2021

@cknitt The assert is already a keyword in both ReScript and OCaml. The type checker even have a special treatment for assert false. As I said, it is a high bar to change the existing keyword set. I would be happy to hear if you have a better proposal.

Note I would like to add another use case besides solving the indentation hell issue. It makes it possible to write bare metal efficient code for cases like below:

let hd = (hs) => {
   let len = List.length (hs)
   if (len > 0) {
     assert let list{hd, ...} = hs
     hd
   } else {
     ..
   }
}

The snippet above is a contrived example, the idea is that there are some cases when you know the pattern match is for sure to succeed, you can use assert let to make the generated code ignore the else branch (enabled with some flags). The real world example is here https://github.com/rescript-lang/rescript-compiler/blob/master/jscomp/others/belt_internalAVLtree.ml#L69

@1uckyneo
Copy link

1uckyneo commented Jan 9, 2023

assert let and if let
I want both😁
if let sugar in Rust is fine to me

@tx46
Copy link

tx46 commented Mar 31, 2023

Status on this?

@cristianoc
Copy link
Contributor

Status on this?

There's more related discussion here: rescript-lang/rescript#5628

Basically, there are mixed opinions on how to introduce something like this. So it's been pretty much sitting there.
But anyone can revive the discussion any time.
The main thing is that there has to be sufficient community support to bring something like this forward.

@stale
Copy link

stale bot commented May 28, 2023

The rescript-lang/syntax repo is obsolete and will be archived soon. If this issue is still relevant, please reopen in the compiler repo (https://github.com/rescript-lang/rescript-compiler) or comment here to ask for it to be moved. Thank you for your contributions.

@stale stale bot added the stale label May 28, 2023
@stale stale bot closed this Jun 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.