Skip to content

Require all parameters in JS #35531

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

Closed
wants to merge 5 commits into from
Closed

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Dec 6, 2019

This is an experiment in fixing #31888. This is the simplest experiment, which requires all parameters to be provided in JS, same as TS. If this doesn't break the user tests that are written in JS too badly, then I won't proceed to the more complex experiments, which are to require all paremeters for contextually typed functions, or to require all parameters when "noImplicitAny": true.

This is an experiment in fixing #31888. This is the simplest experiment,
which requires all parameters to be provided in JS, same as TS. If this
doesn't break the user tests that are written in JS too badly, then I
won't proceed to the more complex experiments, which are to require all
paremeters for contextually typed functions, or to require all
parameters when `"noImplicitAny": true`.
@sandersn
Copy link
Member Author

sandersn commented Dec 6, 2019

@typescript-bot user test this

@weswigham
Copy link
Member

... @typescript-bot user test this ... ?

@weswigham
Copy link
Member

@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 6, 2019

Heya @weswigham, I've started to run the parallelized community code test suite on this PR at 7aa5a80. You can monitor the build here. It should now contribute to this PR's status checks.

@weswigham
Copy link
Member

Looks like azure functions broke their old beta 2.0 version of the function host that we'd never upgraded off of. I had to upgrade the bot to the proper 2.0 GA release to get the bot running again - it was a massive pain to figure that out, since it had just stopped taking or logging requests at all, but it should be good now.

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@sandersn
Copy link
Member Author

sandersn commented Dec 6, 2019

Some brief observations. I haven't made any decisions yet:

  1. uglify and npm have a lot of new errors. They are probably the least sophisticated in terms of JSDoc.
  2. chrome-devtools has lots of new errors, but its jsdoc is aimed at Closure.
  3. worryingly, webpack has 92 new errors, even though they compile with tsc as part of their build.
  4. lodash gets better!

@sandersn
Copy link
Member Author

I looked at the webpack errors. The problem is that Javascript has no built-in way to denote optional parameters. If the average function in a ts-checked codebase is not annotated, which is true of webpack, its signature will look like this: (x: any, opt_y: any) => any. We have no way to tell whether the second parameter should be optional. And assuming that it's not dumps a bunch of errors on otherwise-fine recently-added JS code.

I'm going to look at other options. Using --noImplicitAny is tempting because it's easy, but the effect would be, I think, even more confusing than disabling the Object/object => any rewrite. On the other hand, I'm not convinced that requiring a contextual type will (1) Just Work (2) be feasible. I'll try it next, though.

@sandersn
Copy link
Member Author

Well, a probably-incorrect version is quite easy. It doesn't make any tests fail, but asking for a contextual type in the middle of getSignatureOfDeclaration feels like asking for trouble.

Let's see what the user tests look like:
@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 18, 2019

Heya @sandersn, I've started to run the parallelized community code test suite on this PR at 0e0bff8. You can monitor the build here. It should now contribute to this PR's status checks.

@weswigham
Copy link
Member

Alternatively: Treat any like void - as a type that indicates the argument can be optional, if in a trailing position?

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@sandersn
Copy link
Member Author

@weswigham I don't understand, can you explain more?

@weswigham
Copy link
Member

Today, when we encounter void in an argument position, that argument is considered optional. We could do the same for any, at least in js files.

@sandersn
Copy link
Member Author

Summary of requiring contextual type in order to fix arity:

  1. One correct error from bad type annotations in webpack.
  2. Several cases where a variable is initialised with a simpler function than its intended usual value. Fixable by adding a type annotation.
  3. A few cases where the contextual type is any, which shouldn't make the signature strict in checking number of parameters. I changed this.

Overall I think this is a good compromise. I don't trust the implementation though. @weswigham can you see ways that asking for contextual type in getSignatureFromDeclaration might result in circularities?

@sandersn
Copy link
Member Author

sandersn commented Jan 8, 2020

I'm punting this to 3.9, since (1) it is intentionally breaky (though not badly) (2) I still don't trust the implementation. @weswigham you and I should try to step through an example together and convince ourselves that we can't get caught in a circularity.

@sandersn sandersn added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 1, 2020
@sandersn
Copy link
Member Author

This PR hasn't seen any activity for quite a while, so I'm going to close it to keep the number of open PRs manageable. Feel free to open a fresh PR or continue the discussion here.

(Also, it's redundant with a change @weswigham merged last month.)

@sandersn sandersn closed this Mar 11, 2020
@jakebailey jakebailey deleted the require-all-parameters-in-js branch November 7, 2022 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants