Guard Clause Flow Typing

[danielpclark]

No description provided.

[kennytm]

Rendered: https://github.com/danielpclark/rfcs/blob/guard_clause_flow_typing/text/0000-guard_clause_flow_typing.md

[sfackler]

This is already valid Rust.

[danielpclark]

Correct.

[mark-i-m]

@sfackler I think the key is that the following line would currently be an "irrefutable pattern" error:

https://github.com/rust-lang/rfcs/pull/2221/files#diff-5935e1ae73da8f77fd623e86bd4eb1eeR159

[mgattozzi]

I get the point of this is supposed to provide alternatives to map_err and stuff, but I find that this:

let file = File::open("program.cfg").map_err(|_| Error::ConfigLoadFail)?;

is far more readable and a one liner than:

let file = File::open("program.cfg");
if let Err(_) = file { return Err(Error::ConfigLoadFail); }
let Ok(f) = file;

It kind of obscures what's going on and you mentioned this was going to help new users, but I find it more confusing and less clear as to what's going on. This guard clause feels like a weird version of ?/try!(). I'd expect new users to use copious amounts of match, ask why they have so many nested clauses, then get directed to ?/try!() and all of Result's methods for dealing with these, which has happened before and most new users go "Oh okay thanks" and go on about their day and start using things like map_err.

Most of the above examples above given as an example can easily be rewritten to avoid nested if/else clauses.
For example Wrong A can be rewritten:

fn read_config1() -> Result<Config, Error> {
    let file = File::open("program.cfg").map_err(|_| Error::ConfigLoadFail)?;
    let mut buf_reader = BufReader::new(f);
    let mut contents = String::new();
    buf_reader.read_to_string(&mut contents).map_err(|_| Error::ConfigLoadFail)?
    let mut data: Vec<u8> = Vec::new();
    for item in contents
                      .split("\n")
                      .map(|s| s.to_string())
                      .filter(|s| !s.is_empty())
                      .collect::<Vec<String>>()
    {
        Ok(Config { 
             data: data.push(item.parse::<u8>().map_err(|_| Error::ConfigParseFail)?) 
         } )
    }
}

Which makes the function shorter but is far more readable as well. If the goal is to teach new people Rust, then we should be focusing on the documentation to cover this and make it less confusing if there is some, because this is considered more idiomatic. I don't think we need more ways to do match.

[sfackler]

@mark-i-m That's not the line following this comment.

[mark-i-m]

@sfackler I meant the line with that I linked... It seemed like it would be confusing to add a new comment on that line instead... sorry for the confusion.

[danielpclark]

@mgattozzi In your revised code sample isn't the for loop generating a new Result<Config> through each iteration? And the ownership of data would only work if the underlying type is Copy here right? I've never seen for used in this way: generating & ignoring extra instances and only returning the last.

[sfackler]

I don't see how

let File = File::open("program.cfg");

if let Err(_) = file { return Err(Error::ConfigLoadFail };

let Ok(f) = file;

is preferable to

let f = match File::open("program.cfg") {
    Ok(f) => f,
    Err(_) => return Err(Error::ConfigLoadFail),
};

[mark-i-m]

Or

let f = File::open("program.cfg").map_err(|_| Error::ConfigLoadFail)?;

[danielpclark]

My main point on these is that the level of complexity for new software developers just learning how to program will find our current methodologies much harder to grasp as much more is going on in these code samples. One of the things that I've read within our community is that Rust wants to become easier for people to learn and not need to have so much extra syntax used for common scenarios.

I like the map_err myself, but I'm not likely going to teach people Rust as a first language if this is our primary methodology.

[sfackler]

If anything it seems like this change would increase the cognitive overhead of reading code. Interaction with algebraic datatypes is currently "local", while this change would make it nonlocal in that you can perform destructuring that would normally not be allowed as long as the control flow somewhere some arbitrary distance earlier in the function made it clear that some Result is actually only an Ok.

You're not going to teach Rust as a first language if what exactly is the primary methodology? The use of methods defined on Result? The use of closures? The use of ?? The use of match?

[danielpclark]

We, as experienced developers, take for granted what we're familiar and comfortable with. The complexity may be harder for us to see because of our own bias. So I will use visual cues to help illustrate complexity for non-Rust developers and what they need to take in. The idea is that small simple steps are easier to grasp than more compounded into the same space.

People who are coming from other languages will majoritively be comfortable with an if condition and a code block. Some may also come from pattern matching languages. These two concepts are shared by the vast majority of languages available to program in and this allows more expedient learning of a language.

As Rust developers we love our language and the power each design we have permits us to use. It's also generally true that people form their own preferences/biases in how they like to implement their code. This often leads to people in their programming language being resistant to change because "this is the way it is" and therefore "should be". What looks good to us though is a bias built over time… and that's not a bad thing, but I believe we should be aware of it and more keen to listen to other input knowing our own bias.

The colors above indicate distinct orders of concepts occurring and what we have to grok when we look at the code in groupings/code clumps. The match and map_err examples have roughly 4 phases occurring within a tightly knit space where as the simple assignment and pattern matching, though it may be a little more verbose in lines, keeps the grouped concepts of what's happening down to about two steps and the principles behind them share more with other languages in my opinion.

For us I'm perfectly fine with using match, map_err, and ? because this we know well. It is my hope that we can endevour to broaden the power and ability we have within Rust to make the language more welcoming to others. And I believe broadening the ability of pattern matching and creating a flow-typing environment will feel more natural to many.

I'm not against the way we do things, I merely want to broaden our horizons. And I think in the end we'll be better for it.

[sfackler]

But this assumes that the syntax is the limiting function in learning the language rather than the semantics, which doesn't really seem true to me.

[mark-i-m]

As Rust developers we love our language and the power each design we have permits us to use. It's also generally true that people form their own preferences/biases in how they like to implement their code. This often leads to people in their programming language being resistant to change because "this is the way it is" and therefore "should be". What looks good to us though is a bias built over time… and that's not a bad thing, but I believe we should be aware of it and more keen to listen to other input knowing our own bias.

I agree in general, but I would say that a great deal of this bias (at least for me) comes from the fact that Rust syntax tends to be very elegant, expressive, and ergonomic IMHO; I would like to keep it that way. I still spend a lot of time writing C and Java, and it still surprises me how much easier it can be to write rust.

That's not to say I'm unopen to new syntax -- I just don't think we should introduce new idiomatic syntax purely for teachability unless there is a significant community consensus that existing syntax is confusing (e.g. what sort of happened with dyn Trait).

[danielpclark]

You're not going to teach Rust as a first language if what exactly is the primary methodology? The use of methods defined on Result? The use of closures? The use of ?? The use of match?

For a new developer…Of these three I believe closures would be the more difficult of these to teach (not for me to say, for others to understand). match would take some time to get across, especially with assignment beforehand, but pattern matching would be the first thing to teach before delving into match. After pattern matching is taught the ? wouldn't be too hard to teach. But having them all in one statement would be a bit much for a beginner.

It's been both my hobby and profession to teach concepts and programming through blog posts for many years. Thinking about clarifying things is pretty much what I do in life.

[burdges]

These guards make code harder to read because they break up expressions and require more reasoning about flow control. Any new users will benefit far more from being pointed to .map_err(...)?; and learning to read about types in the std docs, not just about the base language.

As a separate matter, there are good odds this syntax would cause problems in future because it "dances around" powerful type system ideas like subtyping, refinement types, etc. that require deeper exploration to do well. In many cases, these require more exploration of type system features we already have, especially ATCs and const type parameters.

As an example, I could imagine a refine keyword or macro that worked similar to match, but required non-exaustive cases and returned some form of refinement type for the unmatched cases.

let Some(x) = refine file { None => return Err(Error::ConfigLoadFail); };
let bounded_or_negative = refine some_i32 {
    0..57 => return Err(InBadRange),
    100.. => return Err(TooBig),
};  // returns some crazy type like
    // RefinedUnion<i32, RefinedRangeTo<i32, const (..0)>, RefinedRange<i32, const (58..100)>>

I've no clue if such a syntax is wise, or even if the required type system makes sense, but at least it remains expression oriented and makes refinements explicit.

We probably need actual examples where Puffs produces faster parsers than Rust specifically through using such type system features, at which point folks will develop the interest and expertise to do these things properly. Adding reminiscent syntax haphazardly will complicate even thinking clearly about larger type system features.

[kennytm]

@danielpclark your examples would be more convincing if you use types without combinator methods e.g. extracting ownership from a serde_json::Value. OTOH I still think this is not really better than let/else in terms of overall cost+benefit, hence the 👎.-

[burdges]

If you only support one variant, then serde_json::Value::as_* methods do the trick. If you support more than one variant, then you're talking about conversions, so you'll be returning another type anyways, and need a full match anyways.

[danielpclark]

These guards make code harder to read because they break up expressions and require more reasoning about flow control.

To “break up expressions” is to simplify. And “reasoning about flow control” is what programming is and for simplified steps I don't see more harm in more reasoning.

As a separate matter, there are good odds this syntax would cause problems in future because it "dances around" powerful type system ideas like subtyping, refinement types, etc. that require deeper exploration to do well. In many cases, these require more exploration of type system features we already have, especially ATCs and const type parameters.

I don't have experience in developing languages so I'm not one to say what may come of such decisions in the future. I was only very recently introduced to type-flowing via a comment leanardo gave on the Pre-RFC for this. I've looked into it and at first glance I think further enabling Rust's pattern matching with a type flow system would be an awesome powerful utility to have within the language. The potential disadvantages I'm very much ignorant of and so I will gladly defer to input from those more knowledgeable in language design than me.

your examples would be more convincing if you use types without combinator methods e.g. extracting ownership from a serde_json::Value

Anything to help would be appreciated ;-)

[kennytm]

@burdges No as_* doesn't do the trick, what I mean is into_* which are currently missing.

[burdges]

I think as_* cover most cases for this, as either you have a copy type, or else you'd rather borrow anyways.

These let statements change the interpretation of the type, which makes your coloring argument wrong too, and worse does so as a side effect. It's nice to break up an expression with let statements, but you baked flow control into this type interpretation caused by a side effect, making it very much not a simplification. It massively magnifies the cognitive load required to read Rust code.

Instead, type refinements should change the actual type, and do so in a human expressible way, which requires fancier type system features. We might get those features eventually, but I dearly hope they pick some explicit syntax and require an assignment, like the refine keyword I mentioned, not just overload if let with a side effect.

I'm confident this would harm new users by distracting them from using std. It's true lifetimes can make closures slightly intimidating as first, but they do not impact inline closures like one normally passes to map_err, and NLL will soon make the non-inline let f = |x| ..; form equivalent.

[scottmcm]

I think the non-locality of this that sfackler mentioned is my biggest hesitation with this RFC.

Take this from one of the examples:

  // Safe use of pattern matching assignment after Guard Clause
  let Ok(f) = file;

Today (well, with the inhabitability changes from !) a line like that means that that's the only usable variant of the type, and I can use it anywhere that variable is in scope.

#![feature(never_type)]
enum Infallible {}
fn foo<T>(x: Result<T, Infallible>) -> T {
    let Ok(v) = x;
    v
}

Especially since this seems intended only for things with two variants, so tying the two together syntactically seems better to me. I really worry that allowing syntax like this encourages instanceof-style terribleness like

if let Statement(s) = node { ... }
else if let Function(f) = node { ... }
else if let Struct(s) = node { ... }
return Err(...);

instead of the match that ought to be.

[danielpclark]

if let Statement(s) = node { ... }
else if let Function(f) = node { ... }
else if let Struct(s) = node { ... }
return Err(...);

That does seem like a very bad possibility this introduces. I agree with you on that.

[danielpclark]

@scottmcm Thinking more about it, that poorly designed if else scenario is still possible in the language as it is. That being that you can check conditions of multiple separate items through an if else chain. It's not a byproduct of this feature being added to the language. It's just this feature being substituted in place for an eye sore of a potential code practice.

[mark-i-m]

Thinking more about it, that poorly designed if else scenario is still possible in the language as it is.

That's true, but you are not supposed to write code like that. The idiomatic thing to do would be to use match. OTOH, if we introduce this proposal, it seems like it would encourage writing this sort of code.

[CAD97]

I think this attempts to address the same thing that a let ... else would handle. Consider:

• proposed by RFC:

  let file = File::open("...");
  if let Err(_) = file {
      return Err(Context);
  }
  let Ok(contents) = file;

• refutable let, using "guard let" syntax from Swift:

  let file = File::open("...");
  guard let Ok(contents) = file else {
      return Err(Context);
  }

• map_err?

  let file = File::open("...");
  let contents = file.map_err(|_| Err(Context))?;

• match

  let file = File::open("...");
  let contents = match file {
      Ok(contents) => contents,
      Err(_) => return Err(Context),
  }

I agree that in this example case, the "map_err?" syntax is the best option. However, for other enums that do not have such expressive composition fn available, or even just because you don't know about the possibility to ok_or_else(|_| make_error())?.

The refutable let I believe solves the exact usability point that this RFC wants to address, and does so in a more internally consistent way (we have if let, so let else is a natural(?) counterpoint), and reuses existing destructuring rather than introducing flow typing. #373 is the issue suggesting the refutable let, and #1303 is the deferred RFC for a refutable let. See those for more on why a refutable let would be useful.

Note that though the RFC is about adding an amount of flow typing, the motivation is to enable ergonomic use of a "guard clause" without using unwrap. The refutable let is specifically for this case, as the refuted case (the else) is required to be of type ! (to panic, loop endlessly, or break to an outer control flow (continue, break, or return)).

[danielpclark]

let file = File::open("...");
guard let Ok(contents) = file else {
    return Err(Context);
}

@CAD97 I'm not sure that qualifies to be called a Guard anymore. Guards have explicit returns before use of assignment. Also the use of else without an if is a bit confusing.

If the pattern matching is going to be preceded by a keyword (or the like) I would expect the guard's block to treat it's contents as a specific return and not need a by-hand return written in it. That sounds something more like what a macro could do.

I prefer the existing map_err over the guard let … else {} syntax. I really don't think something in that form meets the definition of a guard.

[scottmcm]

@danielpclark Good point. Using the flow typing, it'd actually be enabling

if let Statement(s) = node {
    ...
} else if let Function(f) = node {
    ...
} else if let Struct(s) = node {
    ...
} else {
    let Enum(e) = node;
    ...
}

which definitely doesn't compile today, and I think is even less nice than the if-let chain in my comment above, what with that weird else block.

[nikomatsakis]

@rfcbot fcp close

If I understand correctly, this RFC proposes a flow sensitive typing in which we remember which the variants against which we have attempted to match, and thus permit exhaustive matching in cases where it would otherwise be forbidden:

let x = File::new( ...) ;
if let Ok(v) = x { }

// now we know `x` must be `Err`, so this is allowed:
let Err(e) = x;

As others have noted, the primary motivation here seems to be the same as let ... else, but the setup is significantly more complex to implement, and I think quite likely more confusing for users as well.

I'm actually not sure how we would implement this -- I don't doubt it's possible, but it doesn't trivially integrate into the existing exhaustiveness check, and we'd have to figure out quite a bit to make it work. The problem being solved doesn't seem to merit that much effort. =) Moreover, I think if we were going to address this problem, I'd prefer a syntactic setup like let .. else than introducing this form of reasoning into the compiler.

Therefore, I move to close.

Thanks to @danielpclark for the novel suggestion, in any case. I think it's a clever idea, if not one I think we should adopt at this time.

[rfcbot]

Team member @nikomatsakis has proposed to close this. The next step is review by the rest of the tagged teams:

• @aturon
• @cramertj
• @eddyb
• @nikomatsakis
• @nrc
• @pnkfelix
• @withoutboats

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[burdges]

As noted elsewhere, we maybe need a survey of what different features from formal verification languages like F* and ProVerif might look like in Rust and what benefits they bring. If refinement types were found to really add to Rust then this feature provides an elegant way to teach them, just not without some clear syntax that screams "refinement", i.e. not let .. = match ...

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period is now complete.

[aturon]

Closing as per @nikomatsakis's summary. Thanks for the RFC, @danielpclark!