google / closure-compiler

A JavaScript checker and optimizer.
https://developers.google.com/closure/compiler/
Apache License 2.0
7.4k stars 1.15k forks source link

False positive "property update already defined on interface" when extern contains untyped property called "update" #3277

Open timdawborn opened 5 years ago

timdawborn commented 5 years ago

Mailing list link to thread that spawned this bug report: https://groups.google.com/d/topic/closure-compiler-discuss/0JAsjIFc5Rs/discussion

Closure outputs false positive messages WARNING - property update already defined on interface <name>; use @override to override it when compiling with an extern that defines an untyped symbol called update. Changing the name of the symbol from update to anything else seems to fix the false positive. Adding type annotations to the symbol also seems to fix the false positive.

Compiler version latest:

$ cat /usr/local/bin/closure-compiler
#!/bin/bash
exec java -jar /usr/local/lib/closure/latest/compiler.jar "$@"
$ /usr/local/bin/closure-compiler --version
Closure Compiler (http://github.com/google/closure-compiler)
Version: v20190301
Built on: 2019-03-05 22:03

Repro:

$ cat externs.js
/** @type {!Object} */
let SomeLibrary;

/** @type {!Object} */
SomeLibrary.someThing;

SomeLibrary.someThing.update;

$ cat empty.js

$ /usr/local/bin/closure-compiler --externs externs.js --js empty.js --jscomp_warning '*'
externs.zip//html5.js:1268: WARNING - property update already defined on interface EventTarget; use @override to override it
DOMApplicationCache.prototype.update = function() {};
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

0 error(s), 1 warning(s)

This is a false positive as EventTarget does not define any symbol called update.

In the above example, either changing SomeLibrary.someThing.update to be called anything else (e.g. SomeLibrary.someThing.somethingElse) or adding a type annotation to it (e.g. /** @type {!Object} */) causes this false positive to go away.

These false positive errors continue to seemingly any interface used in code:

$ cat example.js
/**
 * @interface
 */
class MyInterface {
}

/**
 * @implements {MyInterface}
 */
class MyClass {
  /**
   * Does something.
   */
  update() {
  }
}

$ /usr/local/bin/closure-compiler --externs externs.js --js example.js --jscomp_warning '*'
externs.zip//html5.js:1268: WARNING - property update already defined on interface EventTarget; use @override to override it
DOMApplicationCache.prototype.update = function() {};
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

example.js:14: WARNING - property update already defined on interface MyInterface; use @override to override it
  update() {
  ^^^^^^^^^^

0 error(s), 2 warning(s), 100.0% typed

The MyInterface interface clearly does not define a method called update.

timdawborn commented 5 years ago

Laura Hacker on the mailing list has pointed out that extern namespaces should be annotated with /** @const */ instead of /** @type {!Object} */ (as per https://github.com/google/closure-compiler/wiki/A-word-about-the-type-Object#namespaces). This does fix the false positive, so perhaps that incorrect annotation is somehow related to the cause of the false positive.

$ cat externs.js
/** @type {!Object} */
let SomeLibrary;

/** @const */
SomeLibrary.someThing;

SomeLibrary.someThing.update;

$ /usr/local/bin/closure-compiler --externs externs.js --js example.js --jscomp_warning '*'
var MyInterface=function(){},MyClass=function(){};MyClass.prototype.update=function(){};

$ /usr/local/bin/closure-compiler --externs externs.js --js empty.js --jscomp_warning '*'

$
EatingW commented 5 years ago

Internal Google issue http://b/128990360 created.