CheckSuspiciousCode.SUSPICIOUS_BREAKING_OUT_OF_OPTIONAL_CHAIN not part of the SUSPICIOUS_CODE DiagnosticGroup

[Jacob Nowjack]

We have set --jscomp_error suspiciousCode in our codebase and noticed that we were getting warnings instead of errors for SUSPICIOUS_BREAKING_OUT_OF_OPTIONAL_CHAIN. We would like this to be an error instead of a warning in line with the other DiagnosticTypes defined in CheckSuspiciousCode.

If I opened a change request to add CheckSuspiciousCode.SUSPICIOUS_BREAKING_OUT_OF_OPTIONAL_CHAIN to the SUSPICIOUS_CODE DiagnosticGroup here in DiagnosticGroups, would that patch be accepted? 

The contributing guide said to ask here first about any potential changes.

[lha...@google.com]

That seems like a good PR, I think we'd be happy to accept it. Thanks!

I suspect SUSPICIOUS_BREAKING_OUT_OF_OPTIONAL_CHAIN not being in suspiciousCode was just an oversight.

[Jacob Nowjack]

Awesome! I've gone ahead and opened a PR here: https://github.com/google/closure-compiler/pull/4221

[Bradford Smith]

Alas, that might be a PR we will be unable to accept.

Most projects within Google do the equivalent of `--jscomp_error=suspiciousCode` to treat those warnings as errors.

If we accept a PR that adds `SUSPICIOUS_BREAKING_OUT_OF_OPTIONAL_CHAIN` to that list, then we risk breaking existing project builds.

We had a detailed discussion about how to treat this warning/error back in 2022 (http://b/206900693 visible only to Googlers, sadly).

The complicating factor is our internal use of TS converted to JS. TSC does not consider this case to be an error or even something to warn about.

Here's a modified quote of my summary from that discussion.

This is a case where closure-compiler is being more restrictive than TypeScript compiler. As a result, the TS compiler passes code through, then it gets rejected by closure-compiler. This creates a poor developer experience when the error is reported because:

1. It takes longer for the error to be reported. In particular it won't be reported in the developer's initial edit-test cycle for those cases where closure-compiler is not invoked.
2. It exposes closure-compiler to the TypeScript developer. I believe it is something of an implicit design goal to hide closure-compiler as much as possible.
3. Likely at least one more thing I'm not thinking of just now.

If we were to decide that this error message is something we really want to report, then we would want to remove this rough edge. That would require us to get the TS team at MS to agree with us and modify TSC to report this as an error. Or, possibly we could modify tsickle to explicitly look for it and report it.

...

If I didn't have to worry about TS, I would prefer to just leave this check in place, because I believe it is identifying a legitimately bad coding pattern that should be eliminated.

However, we do have to be concerned about TS, and I don't think this coding pattern is likely to appear in handwritten code often anyway.

So, I am now in favor of removing this check from closure-compiler.

Obviously, we never actually removed the check.

The discussion sort of trails off after that, but I believe the decision we made was we should really move this check to the TS and JS linters instead and not ever treat it as an error.

This then went into a pile of fix-it-at-some-point issues.

Now, this doesn't necessarily mean you can't get what you want at all.

The main problem you have is that the error isn't currently included in any `DiagnosticGroup`, so you have no way to tell closure-compiler to make it an error.

I think we could at least consider accepting a PR that simply adds a new `DiagnosticGroup` containing only this one message.

I cannot guarantee we'd accept it. I'm not the only person who would need to be convinced that the extra complexity added would be OK.

Best regards,

Bradford

[Jacob Nowjack]

That makes sense. Thanks for the detailed explanation! 

I will go ahead and create a PR to add a new `DiagnosticGroup` instead since we still find value in this check and would like it to be an error. Hopefully we can convince those who need to be convinced, but I can understand if it's not accepted.