Do not emit @type if @define is specified

[shicks]

Instead, just trust whatever type is specified in @define, which should be either string, number, or boolean.

This is a blocker for rolling out the new module-compatible style of goog.define to TypeScript code.

[shicks]

Addressed the points. PTAL.

[shicks]

This should be ready to merge, though it will need some cleanup internally to go along with any CL that brings it in, since all the types on @defines are now warnings.

[mprobst]

thanks, this looks good!

[shicks]

No, if you look at the jsdoc.js test case, you'll see the badConstThing property declared as @type {string} instead of @const {string}. It doesn't matter if warnings are promoted to errors, but if folks are running this without promoting warnings to errors then this could be unfortunate.

With only a single required annotation and no optional, I'm less sold on the win with the map, though in my opinion it is more readable: (a) using an enum rather than an inline condition makes it clear why it's doing what it's doing, and (b) it's potentially less repetition of the condition, depending on how it's structured. But I tried removing it and the diff looks a little cleaner, so I'm fine with that.

In terms of adding more in the future, we have a few internal annotations around mods that take arguments and might want to go here. Though I can also see an argument for adding those in a separate Set JSDOC_TAGS_WITH_NON_TYPE_ARGS (or just ..._WITH_ARGS).

[mprobst]

No, if you look at the jsdoc.js test case, you'll see the badConstThing property declared as @type {string} instead of @const {string}. It doesn't matter if warnings are promoted to errors, but if folks are running this without promoting warnings to errors then this could be unfortunate.

Yes, tsickle behaves like TS in here (and like JSCompiler) in that it produces output even for incorrect user code. The question is how much effort we should put into emitting something that'll still likely work. With tsickle in general and @define in particular, I'd wager that essentially all users are in bazel or google3, and thus always have these warnings emit errors. So effectively, users should be safe.

There's a question whether we should have ever had this warnings mechanism, I'm not convinced it's useful for anyone. Then again we apparently have some users who use tsickle as a preprocessor to produce JSDoc, they don't care about the effects on JSCompiler at all. Can't really pick your use cases ...

I'll take this over and land it.

[shicks]

Thanks!