reasonml / reason-cli

Globally installable Reason toolchain.
MIT License
291 stars 23 forks source link

Compiler should warn when using a keyword as a variable name #79

Closed eliknebel closed 6 years ago

eliknebel commented 6 years ago

It is very easy (for someone new to Reason) to use a reserved language keyword as a variable name and not know it. When this happens, the compiler might happily compile without warning and lead to strange results. Also, other interfaces (such as react props) may be designed with a prop name that is the same as a language keyword and it is difficult to debug this.

It would be nice if there was a compiler warning when using keywords as variable names or at least some documentation with all the reserved keywords and best practices for how to convert these to safer names from something like React props.

Ex:

let component = ReasonReact.statelessComponent("MyComponent");

let make = (
  ~className: option(string),
  ~match: option(string),
  ~onEditMatch: (. string, string) => unit
) => {
  ...component,
  render: _self => {

    /* Some will never happen... */
    switch (match) {
      | Some(_) => Js.log("Nope!")
      | None => Js.log("Every time")
    };

    <div> /* other content */ </div>
  }
};

let jsComponent =
  ReasonReact.wrapReasonForJs(
    ~component,
    (jsProps) => {
      make(
        ~className=Js.Nullable.toOption(jsProps##className),
        ~match=Js.Nullable.toOption(jsProps##match),
        ~onEditMatch=jsProps##onEditMatch
      )
    }
  );

because match is an OCaml keyword, it doesn't produce an optional value here as expected but there are no compiler warnings about it. Changing match to another name, this code works just fine.

eliknebel commented 6 years ago

This is probably more of a Reason React issue, docs on using existing react components with prop names that collide with keywords do exist and are located here: https://github.com/reasonml/reason-react/blob/master/docs/interop.md#usage

While I still think it would be useful to flag this by the compiler, I wouldn't necessarily call it a bug so I am closing this issue.