Discussion about None and =

[Zdancewic]

This issue is just to raise a discussion about our best practices for style.

In particular, I wonder about using None and =, which is currently marked as poor style, but sometimes the alternative isn't as pleasant in my opinion. The typical case is something like the deq or deq' code below. I prefer the first (and a lot of the examples from class use that), but our style checker insists on the second.

Is there a nicer way to write this code?

let deq (q : 'a queue) : 'a =
    begin match q.head with
      | None -> failwith "empty queue"
      | Some n ->
        q.head <- n.next;
        if q.head = None then q.tail <- None; 
        n.v
    end

    let deq' (q : 'a queue) : 'a =
    begin match q.head with
      | None -> failwith "empty queue"
      | Some n ->
        q.head <- n.next;
        begin match q.head with
          | None -> q.tail <- None  
          | Some _ -> ()
        end; 
        n.v
    end

[KeenWill]

I agree that the first way is cleaner. I think we originally designed this rule to prevent stuff like:

if q.head = None then ... else ...

Or worse, the case where they use an if statement on Some... In this case, since there are two branches, I would think it would be nicer to pattern match rather than use if statements.

Maybe if there was a way we could check for the existence of an else branch, we could flag it with Camelot? And otherwise if they were just doing if x = None then ... (* no else *) that would not be flagged. What do you think @Zdancewic?

[Vighnesh-V]

In general my motivation for writing this rule was to enforce pattern matching to extract the contents of optional values, not to discourage checking for None.

I agree that the first way is cleaner, but here's something I suggested to a student on piazza:

let deq (q : 'a queue) : 'a =
    begin match q.head with
      | None -> failwith "empty queue"
      | Some n ->
        q.head <- n.next;
        if is_none q.head then q.tail <- None; 
        n.v
    end

I think replacing the q.head = None with is_none q.head reads a little more like english and looks a lot nicer (it's also in the Stdlib module). We could either bring it into scope for this problem / or even have them define it as a warm up in imp.ml. In this assignment, I just told the student to define their own version of is_none. @Zdancewic

Two possible fixes as I see it are:

1. Instead of checking for None = _ | _ = None, instead check if Some _ appears on one side of an = in an if statement.
2. Change the fix for this rule to be use Option.is_none instead.

@KeenWill the code for iterating over the Parsetree is taken straight from the OCaml compiler, and this handles iterating into the optional else branch :). We can manually check this by ensuring that the node is an if, and the else branch is None, which can also address the point you were making about discouraging if _ = None then ... else ...

[sweirich]

Has this been resolved? I think the lecture videos & slides use the x = None pattern a lot, so it will be confusing to students if this is flagged this semester.

[Vighnesh-V]

I've been a little busy with work and some of the yaml config changes to camelot, so I haven't been able to get around to this yet :( . A quick way to get this behaviour is by changing the function is_option_lit in /lib/utils/astutils.ml at from:

let is_option_lit : exp -> bool = fun e ->
  e =| "Some" || e =| "None"

to

let is_option_lit : exp -> bool = fun e ->
   e =| "Some"

I think this should produce the behaviour of ignoring x = None as a pattern, but I'm not sure if this breaks other things ...

[KeenWill]

I made a slightly different implementation by creating a is_some_lit version that keeps other things from breaking. I'm going to push to opam soon once I finish off a couple of other things.