closure-compiler icon indicating copy to clipboard operation
closure-compiler copied to clipboard

Union types need brackets

Open fingerartur opened this issue 2 years ago • 8 comments
trafficstars

Even simple union types require brackets (e.g. (number|undefined)) without brackets there is a warning.

/**
* @type {{
*  age: number|undefined, // WARNING - [JSC_TYPE_PARSE_ERROR] Bad type annotation.
* }}
*/
const person = {}

It would be nice to have support for the good old simple bracketless syntaxnumber|undefined.

Compiler Version: v20221102

Build command:

java -jar ./scripts/closureCompiler.jar \
  --entry_point=./src/js/index.js \
  --js=./src/**.js \
  --dependency_mode=PRUNE \
  --warning_level=VERBOSE \
  --js_output_file=./dist/bundle.js \
  --module_resolution=WEBPACK \
  --compilation_level=ADVANCED \
  --jscomp_error=checkDebuggerStatement \
  --jscomp_error=unusedLocalVariables \
  --jscomp_error=reportUnknownTypes \
  --jscomp_error=strictCheckTypes;

fingerartur avatar Jan 09 '23 21:01 fingerartur

I recall this was discussed inconclusively in the past due to type precedence not being established.

An example from a previous discussion:
A function return type like (number | string) expressed without parens would look like this:

function(): number | string

which without clear type operator precedence can also parse as

  (function(): number) | string

rishipal avatar Jan 10 '23 17:01 rishipal

Hi @fingerartur, I would like to contribute to this issue. Can you assign it to me? Thanks!

ashmichheda avatar Nov 13 '23 01:11 ashmichheda

Hey @ashmichheda , could you provide some more details on what sort of contribution you have in mind?

As Rishi commented previously, the underlying issue here is unclear rules around operator precedence in the Closure type syntax, so I think we'd need to tackle that before accepting any PRs. Is that something you'd be interested in, given that it's not as clear where to start?

lauraharker avatar Nov 14 '23 15:11 lauraharker

(I'll also note that we don't have the headcount to plan future investments in the Closure type system, so unfortunately we wouldn't have a lot of time to help)

lauraharker avatar Nov 14 '23 15:11 lauraharker

Hi @lauraharker I don't have any approach as of yet, since I too have the questions about it given the issue seems a bit broad. I was assuming if it's assigned to someone, then one can start a discussion on that.

ashmichheda avatar Nov 15 '23 22:11 ashmichheda

Sounds good, assigning to @ashmichheda.

(Please also feel free to start discussions on or send PRs for unassigned issues, the rate of incoming PRs is relatively low so you're unlikely to conflict with someone else. Most of our commits are from Google's internal repo)

lauraharker avatar Nov 16 '23 20:11 lauraharker

@fingerartur - Could you kindly explain more on the issue? I am a bit unclear on the ask here.

ashmichheda avatar Nov 19 '23 01:11 ashmichheda

@ashmichheda Sure thing, it's simple really. I just checked the behavior of the latest version on NPM - version 20230802.0.0 and it's still the same as with v20221102 so this issue is still relevant. The compiler behaves as follows:

(1) This union type syntax is accepted and works as expected:

/**
 * @type {string|undefined}
 */
var x = 'hello world';

(2) However, a similar case of a typedef with attributes typed with a union type is not accepted by the compiler. e.g.:

/**
 * @type {{
 *  name: string|undefined
 * }}
 */
var Type1;

I'm getting the following error:

./src/js/index.js:8:16: WARNING - [JSC_TYPE_PARSE_ERROR] Bad type annotation. expected closing } See https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler for more information.
  8|  *  name: string|undefined
                     ^

If I add brackets, it works as expected:

/**
 * @type {{
 *  name: (string|undefined)
 * }}
 */
var Type1;

So the what to do about it? The assignment could be: make sure that @typedef attributes support union types without brackets, at least in the simple case of e.g. privimite type|undefined|null, if there is a problem with supporting function types in the union type, then that can be left unsolved, that's fine.

Hint: Since case (1) works fine without brackets why not simply treat case (2) with the same logic as case (1)? It seems that that would solve this problem easily. But I've never contributed to this repo, so I'm not gonna make assumptions.

As to @rishipal, yeah I completely get that functions in the union type may cause trouble, that's very understandable. I'm just wondering why case (1) is treated differently from case (2). Note, that in both of those cases, I could put functions in the union type, so in both those cases functions could still cause trouble there. My view as a user of this compiler is: I'm completely fine with the requirement that if you are using union types of functions then you must use brackets to make sure it is not ambiguous, I think that is a fair requirement.

fingerartur avatar Nov 19 '23 13:11 fingerartur