Supporting @suppress annotation on class nodes

[supersteves]

SuppressDocWarningsGuard wasn't observing class nodes when looking for
@Suppress, as needed for missingRequire on class X extends Y:

/** @suppress {missingRequire} */
class X extends Y.Z { // where Y.Z is in externs
}

Without this:

WARNING - 'Y.Z' used but not goog.require'd

See #1143
and #1145

[MatrixFrog]

It's always been true that @suppress only works at the @fileoverview level or the function level. I think it's probably reasonable to add "class" to that list but there is likely documentation in N different places that needs to be updated. Let's try to find at least some of it before merging this.

[supersteves]

You mean documentation in the code / javadoc, or wiki / elsewhere?

[blickly]

We'll also want to add some tests to make sure that we handle

var X = class extends Y.Z { ... }

and
ns.qualfiiedname.whatever = class extends Y.Z { ... }
in addition to

class X extends Y.Z { ... }

On Wed, Sep 16, 2015 at 9:27 AM, Tyler Breisacher [email protected]
wrote:

It's always been true that @Suppress only works at the @fileoverview
level or the function level. I think it's probably reasonable to add
"class" to that list but there is likely documentation in N different
places that needs to be updated. Let's try to find at least some of it
before merging this.

—
Reply to this email directly or view it on GitHub
#1147 (comment)
.

[supersteves]

SuppressDocWarningsGuard is devoid of tests, I think

[MatrixFrog]

If we find anything in the Javadoc or code comments then we should fix it as part of this PR. There's also the Github wiki (which anyone can edit), https://developers.google.com/closure/compiler/ (which only Googlers can edit, unfortunately), and probably one or two other spots I'm forgetting.

[supersteves]

I see @blickly has revealed the test cases, so I'll look at adding one. But shouldn't @suppress take effect at whatever scope you choose to place it? Currently there are some quite precise and unexplained restrictions for @suppress; as a developer I'd like to be able to suppress an entire block and everything inside it, depending on placement:

// Suppress just this warning precisely (on NAME or GETPROP):
class X extends /** @suppress {missingRequires} */ React.Component {}

// Suppress just this warning anywhere on or in the class definition
/** @suppress {missingRequires} */
class X extends React.Component {}

The same goes for other suppress options, you should be able to disable any of the non-mutating checks at any level in the AST. Except for the Rhino ones which presumably could at some point become Node aware. You might want to disable all warnings in particular files. A bit like you can for js/eslint. Perhaps this is dangerous and could be done using a @suppresswithin new annotation, or js/eslint-style comments.

I'm getting off track, right here all I need is to fix the warnings for extends React.Component. If you prefer I'll remove my class level suppression and replace it with a suppression exclusively on the NAME or GETPROP of the extends clause (first example in code snippet above). This should be easier to apply precisely. With a test. Let me know.

[supersteves]

In terms of docs:

• javadoc / code:
  • SuppressWarningsGuard has some sparse docs
• wiki:
  • https://github.com/google/closure-compiler/wiki/Warnings
• google-editable:
  • https://developers.google.com/closure/compiler/docs/error-ref?hl=en
  • this page probably should mention @suppress but doesn't: https://developers.google.com/closure/compiler/docs/js-for-compiler

I think that's all relevant places needing an update. The wiki page is pretty good, it's a shame I missed it and had to look at the source code to figure out how it works :-)

Really it could do with a better and auto-generated list of warnings and errors, a table that matches up command line options with suppress values and Java API properties etc.. Some other time...

[supersteves]

Also, question: where is source for the debugger (https://closure-compiler-debugger.appspot.com)? How do options in the debugger match up to options in the Java API? I am often hunting through the debugger to find the equivalent of things like language-in.

[MatrixFrog]

We tried to open-source the debugger but it turned out to be hard to do, unfortunately. We will probably manage to get it done someday (or we'll just write a new one that is open from the beginning) but I can't promise any ETA, unfortunately. As for language-in in particular, the checkbox is called LANG_IN_IS_ES6. Checked is ES6, unchecked is ES5.

[ChadKillingsworth]

@supersteves It's hard to discern intent for @suppress annotations except for the file and function level. I've asked this question myself before. Without clear guidance that's intuitive to developers, support at other levels is more difficult.

For instance, on an ES6 class:

/** @suppress {uselessCode} */
class foo {
    constructor() {
        document.body.offsetWidth;
    }
}

foo.prototype.bar = function() {
  document.body.offsetWidth; //should the useless code warning be suppressed here?
};

[supersteves]

@ChadKillingsworth How about if I rework it as specifically just:

// Suppress just this warning precisely (on NAME or GETPROP):
class X extends /** @suppress {missingRequires} */ React.Component {}

?

[supersteves]

Closing in anticipation of #1192