codemix / babel-plugin-typecheck

Static and runtime type checking for JavaScript in the form of a Babel plugin.
MIT License
886 stars 44 forks source link

Support for VariableDeclaration and fix for ArrowFunctionExpression #25

Closed fixplz closed 8 years ago

fixplz commented 8 years ago

Arrow functions needed a fix for when the body is an expression.

Added type checking for variables at declaration time.

fixplz commented 8 years ago

I started to use direct mutation in some places because I don't know how to use the NodePath methods to do the same.

We can add support for checking mutating assignments by accumulating type info in the visitor state or by retrieving type annotations from the scope with scope.getAllBindings().

phpnode commented 8 years ago

@fixplz this is very cool!

phpnode commented 8 years ago

Yes it would be very nice if we could do something like this:

let foo: string = "foo";

foo = 123; // should fail
fixplz commented 8 years ago

@phpnode given that assignment is/can be an expression, how should we emit the type check for it?

phpnode commented 8 years ago

@fixplz I think we need to bite the bullet and start extracting some (or all) kinds of checks into specialist functions. E.g.

let foo: string = "foo";
foo = something(foo);
foo = somethingElse(foo);

would compile to:

var foo = "foo";
foo = $_assertString(something(foo));
foo = $_assertString(somethingElse(foo));

function $_assertString (input) {
  if (typeof input !== "string") {
   throw new TypeError("Expected string got " + typeof input);
  }
  return input;
}

Being able to do this will make it easier to support things like Array<string|number>, Array<string, number> and type aliases. It'll also make the file sizes smaller so we should probably make this change.

fixplz commented 8 years ago

Right, this is what Babel proper does (ie the _classCallCheck function for classes). But the downside to moving the throw to a helper function is that the error stack trace information will become worse (instead of showing the relevant code location as the source of the error it will show the helper function). This demotivates me to make the change because I value stack trace quality.

We could do this

_typeCheck(val, Type, new TypeError)

and generate the error message in the function – this looks right in Chrome dev tools (not as much in Firefox, but Firefox dev tools looks bad without doctoring anyway that I can tell).

But maybe I can solve the problem with stack trace quality for myself as a separate project (using the fact that it's possible to manipulate stack traces at runtime in v8), in which case we can just do what Babel does.

phpnode commented 8 years ago

@fixplz yes this is definitely a major downside. I thought it might be possible to get around in some browsers with Error.captureStackTrace() but I couldn't get it working when I fiddled with it earlier.

A really horrible idea I just had - what if we manipulated the loc fields of the generated functions so that they point to the original call sites? These fields contain the position of each AST node in the source file, manipulating them wouldn't affect any code but it'd affect the generated source maps. Browsers that support source maps should then show the right stack trace, in theory, perhaps?

Regardless, I do think that this is a good reason to keep the existing checks inline where possible, and only use these generated helpers where it's really required. Having a slightly funky stack trace is worth the trade off.

fixplz commented 8 years ago

@phpnode Like this (shows the location of makeError() call as origin of error)

function makeError () {
  var err = Object.create(Error.prototype)
  err.message = "error desu"
  Error.captureStackTrace(err, makeError)
  return err
}

but this is V8-only.

If you want to set the location for generated functions you would need a new function for each site. So there's no deduplication and the functions could be IIFEs anyway.

Let's just expand assignments that are not statements to a do{} expression and let Babel handle it.

phpnode commented 8 years ago

@fixplz yes totally agree