google / google/closure-compiler

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

Open
#4,038 1 comment 0 reactions 0 assignees View on GitHub
triage-done
Dominant language
JavaScript
Stars
7.7k
Forks
1.2k
Avg merge
2d 12h
Merged PRs (30d)
6

Description

Looking at the [documentation of compiler flags](https://github.com/google/closure-compiler/wiki/Warnings), 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`
```js
class Car {
constructor() {
/**
* @private
*/
this.color = 'red'; // should raise error, but doesn't
}
}
```
- `strictPrimitiveOperators`
```js
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.
```js
/**
* @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:

```bash
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
```

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.