Skip to content

WIP: ignore last void parameters - #20570

Closed
Wenlu Wang (Kingwl) wants to merge 1 commit into
microsoft:masterfrom
Kingwl:ignore-void-parameters
Closed

Wenlu Wang (Kingwl) wants to merge 1 commit into
microsoft:masterfrom
Kingwl:ignore-void-parameters

Conversation

@Kingwl

Copy link
Copy Markdown
Contributor

Fixes #4260

Comment thread src/compiler/checker.ts
// Record a new minimum argument count if this is not an optional parameter
const isOptionalParameter = param.initializer || param.questionToken || param.dotDotDotToken ||
iife && parameters.length > iife.arguments.length && !param.type ||
iife && parameters.length > iife.arguments.length && !param.type || param.type && param.type.kind === SyntaxKind.VoidKeyword ||

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A real fix here is quite a bit more involved, unfortunately.

This change doesn't fix the linked issue because it's only looking for the exact syntactic form of function f(x: void) (which no one would write) instead of actually detecting the type of the parameter during overload resolution and treating it as optional.

@RyanCavanaugh

Copy link
Copy Markdown
Member

At a minimum this needs a testcase that includes the original repro (which uses generics). The code so far isn't in the right place to fix the issue as written.

@DanielRosenwasser

Copy link
Copy Markdown
Member

Do we really have people who want to do this now that we have null and undefined as types?

@Kingwl Wenlu Wang (Kingwl) changed the title ignore last void parameters WIP: ignore last void parameters Dec 11, 2017
@RyanCavanaugh

Copy link
Copy Markdown
Member

Daniel Rosenwasser (@DanielRosenwasser) Promise<void> still needs to behave according to expectation

@Kingwl
Wenlu Wang (Kingwl) deleted the ignore-void-parameters branch December 18, 2017 03:49
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jun 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants