Flow: Doesn't check the type of an exception

[langco]

This function will return a number, without flow complaining about it.

function foo(x: ? number): string {
try {
        throw x
    } catch (e) {
        return e // returns x, which is a number
    }
}

[vkurchatkin]

I agree, e should be mixed instead of any

[vkurchatkin]

I think also annotating error should be possible:

try {
 } catch (e: Error) { // mixed by default, but arbitrary annotation is allowed
 }

[zertosh]

In flow 0.36.0, this is a syntax error:

try {
} catch (e: Error) { // (e: Error) is a Syntax Error
}

[vkurchatkin]

@zertosh just to clarify, my proposal is:

try {
 } catch (e) {
   // e is mixed, safe behaviour
 }

try {
 } catch (e: any) {
   // e is any, works exactly as it works today
 }

try {
 } catch (e: Error) {
   // e is Error, works 99.99% percent of the time
 }

[mroch]

e: Error is unsound because we don't check the throws to make sure they're throwing Error.

allowing an annotation seems reasonable to me, but we'll have to flow mixed into it which would mean the only things you could actually annotate with are mixed or any. that's good because the default case will be mixed, but you can downcast to any.

you'd end up with errors like

} catch (e: Error) {
         ^ mixed. Some potential exception is not compatible with
} catch (e: Error) {
            ^^^^^ Error

[vkurchatkin]

@mroch everything other then mixed is unsound, but it seems to be a good compromise to allow any annotation (i.e. not flowing mixed anything into it if it's present)

[samwgoldman]

I think we should stick with any here and continue to forbid annotations unless:

1) We add support for effects, model exceptions as effects, add annotation support to cover libdefs, and actually model the data flow for exceptions

2) Find some way to indicate "untrusted" types and mark annotations here as untrusted. Any program location into which an any type flows would be untrusted, and this would be one such location.

3) Do what @mroch suggests and actually change from any to mixed. This wouldn't allow an explicit Error annotation, but you could add an explicit any annotation and cast to Error, or do a type test on the mixed.

[vkurchatkin]

@samwgoldman

1 would be the ideal, but probably not worth it. No sure what you mean by 2. What is the purpose of untrusted types?

I'm personally fine with 3, but thought that it would break too much code at once

[samwgoldman]

Avik actually has done some work on effects (1) in order to havoc refinements properly, but the work isn't landed and I don't think anyone has considered using it for exceptions.

(2) is an idea we've been kicking around, mostly in the context of an optimizing compiler which would need to insert runtime checks around untrusted types, but could elide them elsewhere. That line of thinking is still very uncertain, though. Another benefit of surfacing trustworthiness of types would be helping programmers to understand the "amount" of any in their code and possibly to reduce the impact of any-typed code.

I agree that (3) would break a lot of code and I mostly put it up there as a strawman.

Mostly I think we should maintain the status quo.

[vkurchatkin]

Another benefit of surfacing trustworthiness of types would be helping programmers to understand the "amount" of any in their code and possibly to reduce the impact of any-typed code.

Isn't it what cover does already?

I agree that (3) would break a lot of code and I mostly put it up there as a strawman.

What about allowing only mixed as annotation?

[bradennapier]

In the minimum it would be really nice if this at least would mark as covered since it is an error to annotate the value making it impossible to get full coverage.

This may be an eslint error - but not really sure specifically where.

[k]

From a dev-ops perspective this would be great to allow some sort of casting to achieve full coverage. Its much easier to use a tool that enforces 100% flow coverage rather than analyze which coverage exceptions will be allowed.

[zeorin]

I like option 3. It encourages checking to see whether what you've caught is what you expect, hopefully encouraging re-throwing it if it didn't match.

Could we enable that as a strict mode option? That would make it opt-in.

[zeorin]

I've proposed an option to automatically cast any to mixed here: https://github.com/facebook/flow/issues/7515. Not just for the type of an exception, but it would also affect exceptions.

[goodmind]

Find some way to indicate "untrusted" types and mark annotations here as untrusted. Any program location into which an any type flows would be untrusted, and this would be one such location.

I think this option already in works with recent addition of $Trusted and $Private types?

@samwgoldman

[mgreenw]

@zeorin I also like option 3. I have two questions:

1) Your proposal #7515 does not affect the ability to manually cast mixed to any in non-strict mode, correct?
2) Does #3259 implement option 3? It looks like that work allows annotating the error instead of automatically giving it mixed, but I'm not sure.

[zeorin]

@mgreenw

1. I'm not a 100% sure on that. It's possible to enable the strict mode options as lint warnings in non-strict mode, so there might be some interference still. Aliasing any to mixed would mean that a manual cast like you suggest would probably cast mixed to mixed under my proposal (unless we special-cased it). _However_ it is not meant to apply to a suppress_type (which is a custom any alias you can define in .flowconfig), so you should still be able to cast to any, effectively, but you wouldn't actually write any in that cast operation (you'd use your suppress_type alias instead).
2. It does not, and is unsound. The only sound options are 2 & 3, IMO.

[mgreenw]

@zeorin Thanks for the response! Do you know if an implementation of option 3 is underway?

[zeorin]

Not to my knowledge. If you like you can :+1: my proposal https://github.com/facebook/flow/issues/7515 and hope for the best :crossed_fingers:.

[mgreenw]

@zeorin 👍+ 🤞