google / closure-compiler

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

Optional argument can appear before non-optional parameters #2314

Open alexeagle opened 7 years ago

alexeagle commented 7 years ago

This code produces 2 warnings:

function f(a=1,b) {}
f(undefined, 3);
JSC_OPTIONAL_ARG_AT_END: optional arguments must be at the end at line 1 character 0
function f(a=1,b) {}
^
JSC_WRONG_ARGUMENT_COUNT: Function f: called with 2 argument(s). Function requires at least 0 argument(s) and no more than 1 argument(s). at line 2 character 0
f(undefined, 3);
^

https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%2540output_file_name%2520default.js%250A%252F%252F%2520%2540formatting%2520pretty_print%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250Afunction%2520f(a%253D1%252Cb)%2520%257B%257D%250Af(undefined%252C%25203)%253B

And it cannot be suppressed with @suppress {functionParams}.

It is legal ES6 for a parameter with a default value to be followed by a required one, it should not be considered optional.

MatrixFrog commented 7 years ago

ISTR that we intentionally made the decision to be more restrictive than what the spec allows. If anyone remembers the discussion on that and could summarize it in a wiki page that would be great.

supersteves commented 7 years ago

@MatrixFrog advised it could be suppressed with @suppress {functionParams} but this is not the case:

input0:4: WARNING - Parse error. unknown @suppress parameter: functionParams
 * @suppress {functionParams}
              ^

https://closure-compiler-debugger.appspot.com/#input0%3D%252F**%250A%2520*%2520%2540param%2520%257Bnumber%253D%257D%2520opt_x%250A%2520*%2520%2540param%2520%257Bnumber%257D%2520y%250A%2520*%2520%2540suppress%2520%257BfunctionParams%257D%250A%2520*%252F%250Afunction%2520a(opt_x%252C%2520y)%2520%257B%257D%253B%26input1%26conformanceConfig%26externs%26refasterjs-template%26includeDefaultExterns%3D1%26CHECK_SYMBOLS%3D1%26MISSING_PROPERTIES%3D1%26TRANSPILE%3D1%26CHECK_TYPES%3D1%26CLOSURE_PASS%3D1%26PRESERVE_TYPE_ANNOTATIONS%3D1%26PRETTY_PRINT%3D1

dimvar commented 7 years ago

It is legal ES6 for a parameter with a default value to be followed by a required one, it should not be considered optional.

@alexeagle are you proposing that both parameters be treated as required? What's the point of using a default value then? WDYT about treating all trailing parameters after a parameter with a default value as optional?

supersteves commented 7 years ago

Not sure if this is his case but for example in a Redux application a reducer is typically defined as function myReducer(state = new AppState(), action) and Redux calls it as myReducer(undefined, someAction) which results in new AppState() being passed to the function body.

dimvar commented 7 years ago

In this case, state is a required parameter whose type includes undefined. That's fine.

When the compiler sees an unannotated function such as function f(a=1,b) {}, it needs to decide whether a is required or optional. I think that in most cases the user wants it to be optional, so we would give spurious warnings when it was meant to be required. If we considered it to be required, there would be more spurious warnings when f is called without any arguments. It's a tradeoff. If the user wants something that is not the default compiler assumption, they can just manually annotate their code.

But I do agree that the current situation of early warning is somewhat unsatisfactory, and we should just consider the trailing parameters optional and not warn.

supersteves commented 7 years ago

One possibility is that the default assumption applies when there are no @param jsdoc. But when there are, it would take precedence, and the lack of = in the param type would override this behaviour.

dimvar commented 7 years ago

Right.