rules_closure icon indicating copy to clipboard operation
rules_closure copied to clipboard

reportUnknownTypes can't figure out catch blocks

Open jschaf opened this issue 8 years ago • 2 comments

Closure errors out if you try to get ex.message from a catch block.

Since reportUnknownTypes is enabled by default and recommended for new code bases with these rules_closure, I'm filing the bug here.

All of the following have errors. However, with --new_type_inf, parseComment1 works.

function parseComment0() {
  try {
    throw new Error;
  } catch (ex) {
    return ex.message;
  }
}

function parseComment1() {
  try {
    throw new Error;
  } catch (ex) {
    if (ex instanceof Error) return ex.message;
  }
}

function parseComment2() {
  try {
    throw new Error;
  } catch (ex) {
    return /** @type {!Error} */ (ex).message;
  }
}

function parseComment3() {
  try {
    throw new Error;
  } catch (ex) {
    const error = /** @type {!Error} */ (ex);
    return error.message;
  }
}

Full working repo at https://gist.github.com/jschaf/8614760290a95551a905d07107cfd7e7. Steps to reproduce:

  1. git clone https://gist.github.com/jschaf/8614760290a95551a905d07107cfd7e7 closure_catch_bug
  2. cd closure_catch_bug
  3. bazel build :foo_bin

Related Issues: https://github.com/google/closure-compiler/issues/427 https://github.com/google/closure-compiler/issues/152

jschaf avatar Jan 22 '17 01:01 jschaf

Thank you for bringing this to our attention.

For the record, if you're using Closure Rules at HEAD (which currently isn't recommended) then suppress = ["reportUnknownTypes"] will work around this bug. This is of course suboptimal. But we're working hard to improve the compiler and create better workarounds.

Before Closure Rules 0.5.0 is released, we're hoping to implement full incremental type checking. (Closure Rules only runs a subset of checks incrementally at the moment.) This will allow new_type_inf = True to be enabled on a per-library basis, so the user can use it in the situations where it helps, but not in the situations where it doesn't.

jart avatar Jan 22 '17 22:01 jart

The following works with @suppress {reportUnknownTypes} only applied in a minimal scope

function parseComment4() {
    try {
        throw new Error;
    } catch (ex) {
        return (/** @return {string} @suppress {reportUnknownTypes} */ function () {
            return ex.message;
        })();
    }
}

Seems like caught exceptions are inferred as @type {?}. If it were an easy change with acceptable side-effects, perhaps it might be preferable if they were inferred as @type {*}. This would enable the parseComment2 and parseComment3 functions, above, to work as their author desired.

Andrew-Cottrell avatar Sep 28 '19 19:09 Andrew-Cottrell