Throw on unhandled license type

[cbracken]

If a LicenseType is ever left unhandled in the switch statement, we now
throw an exception. This also fixes a 'missing return statements' lint.

[Hixie]

this shouldn't be necessary, the linter will complain if there's ever an enum value added that isn't handled by the switch.

[cbracken]

See the description: This is specifically to address the missing return lint. Not sure if lints are able to have awareness of other lints; if so, we can file a bug against the missing return lint.

[Hixie]

The usual pattern we use is to assert non-null before the switch, and return null after. The assert and the return with go away with NNBD.

[cbracken]

The usual pattern we use is to assert non-null before the switch, and return null after.

Happy to change it, though given that we don't run the license tool with asserts enabled, that seems less safe than throwing. e.g. someone edits our lints to be less strict.

The assert and the return with go away with NNBD.

I'm looking forward to this as much as anyone :)

[Hixie]

why is it less safe?

[cbracken]

For the reason in my comment — two options will eliminate the lint: a return or a throw. Given a choice between the two, we’d want to throw with an obvious error message for a statement that's meant to be un-hittable. This should only ever occur if the switch lint were ever removed.

[Hixie]

If that lint were ever removed we'd have bigger problems.
I think the way to address that is to have a test that verifies the lint is still working.

[cbracken]

sgtm - I'm happy to add something like the existing fail test both here and in the framework.

Would you prefer I change this to return null rather than throw or fine to land as-is?

[Hixie]

Returning null would be more consistent with the rest of the codebase, but I don't feel strongly. It'll be moot soon anyway since NNBD will turn the throw or return null into dead code and we'll have to remove it.

[cbracken]

Done.

[cbracken]

/cc @devoncarew @pq given that it's possible to determine through static analysis that (a) the switch covers all possible enum values and (b) that every one of the values returns, should this be filed as a feature request/bug for the linter?

Opposing argument: there's no guarantee that the input is non-null when asserts are disabled. With that in mind, the lint seems correct as implemented (at least insofar as the implementation pertains to this PR)

[pq]

Sorry for the slow follow-up on this one. My sense is that we should be able to detect that the return is unnecessary (and actually dead code). @bwilkerson: does that seem right?