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

Compiler warnings are incorrectly documented, some do not work and some do not exist

Open fingerartur opened this issue 2 years ago • 1 comments

Looking at the documentation of compiler flags, specifically those, that are OFF by default, there are many problems. Let's break it down:

Some flags have a wrong default value in the wiki

Some flags are not OFF, so their default value in the wiki should either be WARNING or ERROR (not OFF). These flags are:

  • visibility
  • accessControls
  • const
  • constantProperty
  • missingRequire - Maybe this flag should be documented a little more in detail. What module system does it apply to? For example missing import/export statements in ESM modules are type checked by default

Some flags are not recognised by the compiler

  • undefinedNames

Some flags simply do not work

  • unusedPrivateMembers
class Car {
  constructor() {
    /**
     * @private
     */
    this.color = 'red'; // should raise error, but doesn't
  }
}
  • strictPrimitiveOperators
function go() {
  return 555 + 'hello' + {} + new Element() // should raise error, but desn't
}

Sometimes it is very hard to understand what a flag does

  • missingProperties
Warnings about whether a property will ever be defined on an object. Part of type-checking.

What does that mean?

  • strictModuleDepCheck
Warnings about all references potentially violating module dependencies.

What does that mean? What is a violation of module dependencies?

  • typeInvalidation
Warn about properties that cannot be disambiguated when using type based optimizations

What does that mean?

Some flags need more documentation

  • strictMissingProperties
Warnings for missing properties that forbid accessing subclass props off superclasses

In reality though, this flag does so much more. Only the authors know all the details. From what I have been able to reverse engineer, this flag completely changes the behavior of union types, the all type, Element type, etc. I believe this would be worth mentioning in order to facilitate an easier understanding of all that this flag actually does.

Some flags are wrongly documented

  • reportUnknownTypes
Warnings for any place in the code where type is inferred to ?. NOT RECOMMENDED!

This documentation is not entirely true, if I declare a variable of unknown (?) type - so it is declared, as opposed to inferred - the compiler also raises error. So the behavior I see is that it does not apply only to inferred types but also to declared types.

/**
 * @param {?} z // raises error
 */
function hello(z) {
  console.log('hello', z);
}

This is either a mistake in documentation or a bug in the compiler - that depends on what was the intended behavior.

There is also an interesting question: Why does the documentation say it is "NOT RECOMMENDED!"? This would deserve a little more in-depth explanation. It is not immediately obvious why this flag is not recommended. It seems like a useful flag.

Environment

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;
  
  # Plus whatever flag I was testing at that moment, e.g.:
  # --jscomp_error=checkDebuggerStatement
  # --jscomp_error=unusedLocalVariables
  # --jscomp_error=reportUnknownTypes
  # --jscomp_error=strictCheckTypes

fingerartur avatar Jan 10 '23 14:01 fingerartur

Fixed the title as this refers to the warnings page (not flags). The flags get auto updated with each release. The warnings do not.

rishipal avatar Jan 11 '23 18:01 rishipal