Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected warning when implementing an interface without @extends #3137

Closed
rictic opened this issue Nov 4, 2018 · 4 comments
Closed

Unexpected warning when implementing an interface without @extends #3137

rictic opened this issue Nov 4, 2018 · 4 comments

Comments

@rictic
Copy link
Member

rictic commented Nov 4, 2018

The following code:

/** @interface */
class FooInterface {
  /** @return {string} */
  foo() {};
}


/**
 * @implements {FooInterface}
 */
class Foo {
  /** @return {string} */
  foo() {
    return '';
  }
}

Produces the warning:

 WARNING - property foo already defined on interface FooInterface; use @override to override it
  foo() {
  ^^^^^^^

The documentation at https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler#override seems to suggest that this shouldn't be necessary:

[The @override tag] is not required when a class extends a superclass or implements an interface

This is with the internal flag set RECOMMENDED_FLAGS. I didn't see a way to repro it on the closure compiler service. Googlers can see cl/220008730 internally, or repro with

  blaze build //experimental/users/rictic/override-implement-interface:bin
@rictic
Copy link
Member Author

rictic commented Nov 4, 2018

Things which don't affect this warning:

  • class syntax vs function tagged @constructor
  • @interface vs @record
  • local interface vs one defined in an externs file

rictic added a commit to Polymer/polymer that referenced this issue Nov 4, 2018
With these changes we have zero errors and zero warnings with `RECOMMENDED_FLAGS`!

Most of the changes were adding `@override` for methods and properties in mixins. Apparently if you implement an interface you need to say `@override` for each method or property on the interface. This combines with our mixin strategy to the tune of needing to add `@override` on every non-private method and property.

I'm not sure this is intended behavior of the compiler. Filed google/closure-compiler#3137 to see if it is.
@lauraharker
Copy link
Contributor

This is WAI, but I agree the documentation is misleading.

This tag is not required when a class extends a superclass or implements an interface - in those cases the documentation is automatically inherited. This tag is mainly useful to catch typos such as a method named foobar1 in the superclass and mistakenly named foobar2 in the subclass.

The "not required" part really means "not required for type inference to do its job." The wiki should say something like:

Adding or omitting the @override tag does not change what type the compiler infers for a method in a class that extends a superclass or implements an interface. The method's documentation is always automatically inherited from the overridden method in those cases. However, the recommended compiler flagset includes warnings for missing and spurious @override annotations. These warnings are mainly useful to catch typos such as a method named foobar1 in the superclass and mistakenly named foobar2 in the subclass

Does that wording make more sense to you? I can update the wiki.

@rictic
Copy link
Member Author

rictic commented Nov 6, 2018

Ah, your rewording is much better!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants