-
Couldn't load subscription status.
- Fork 724
Several fixes to JS typing of functions and methods #1960
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes several issues related to JavaScript function and method typing. The main changes ensure that untyped functions/methods in .js files correctly allow arguments to be omitted, and that @type JSDoc annotations are properly applied with full signature information including parameter names and optionality.
Key changes:
- Properly computes
SignatureFlagsIsUntypedSignatureInJSFileso untyped JS functions/methods treat all parameters as optional - Restricts
@typeJSDoc annotations to only apply to functions and methods (excluding constructors and accessors) - Uses full signature information from
@typeannotations including parameter names and optionality markers
Reviewed Changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/checker/checker.go | Computes untyped signature flag for JS files; restricts @type application to appropriate function types |
| internal/checker/emitresolver.go | Ensures JSDoc-declared signatures aren't treated as overload implementations |
| internal/ast/utilities.go | Exports IsMethodOrAccessor function for external use |
| testdata/baselines/reference/* | Updated test baselines reflecting corrected behavior for untyped JS functions and @type annotations |
Comments suppressed due to low confidence (1)
internal/checker/checker.go:1
- [nitpick] This multi-line boolean expression could be split into smaller named boolean variables to improve readability. For example,
isJSFunctionLike,hasNoTypedParameters, andhasNoContextualSignaturewould make the logic clearer.
package checker
| // return a; | ||
| // } | ||
| if len(signaturesOfSymbol) == 1 { | ||
| declaration := signaturesOfSymbol[0].declaration |
Copilot
AI
Oct 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition declaration.Flags&ast.NodeFlagsJSDoc == 0 checks if the JSDoc flag is NOT set, but there's no comment explaining why JSDoc-flagged declarations should be excluded from being considered overload implementations. Adding a brief comment would clarify this non-obvious logic.
| declaration := signaturesOfSymbol[0].declaration | |
| declaration := signaturesOfSymbol[0].declaration | |
| // JSDoc signatures are not considered overloads, so exclude JSDoc-flagged declarations. |
| const variableWithMoreParameters = function (more) {}; // error | ||
| - ~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| -!!! error TS2322: Type '(more: any) => void' is not assignable to type '() => void'. | ||
| -!!! error TS2322: Target signature provides too few arguments. Expected 1 or more, but got 0. | ||
|
|
||
| /** @type {() => void} */ | ||
| const arrowWithMoreParameters = (more) => {}; // error | ||
| - ~~~~~~~~~~~~~~~~~~~~~~~ | ||
| -!!! error TS2322: Type '(more: any) => void' is not assignable to type '() => void'. | ||
| -!!! error TS2322: Target signature provides too few arguments. Expected 1 or more, but got 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious loss of error; it's maybe fine but I would have expected this to still error given this is contextually typed by the declarations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to be an issue with reparsing. In Corsa, we currently consider the @type annotation to only apply to the variable declaration and not the arrow function. In Strada we consider the annotation to apply to both. I'll look at fixing this, provided we like the Strada approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is something @sandersn changed but I can't remember anymore.
If it's a big change I don't think it's a big deal to ignore for this PR either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And, I should add, in Corsa, because the @type annotation doesn't apply to the arrow function, the arrow function is considered completely untyped and therefore gets the SignatureFlagsIsUntypedSignatureInJSFile flag and the more permissive arity check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And, I should further add, the arrow function is considered completely untyped because the contextual type doesn't have an applicable signature. That's a bit too aggressive, so with the latest commit, any contextual type for a function in a .js file causes us to not consider that function untyped.
In this PR:
SignatureFlagsIsUntypedSignatureInJSFilesuch that calls to untyped functions and methods declared in .js files allow arguments to be omitted.@typeJSDoc annotations to functions and methods (specifically, ignore@typeannotations in constructors and accessors).@typeannotations (similar to Strada).exportmodifier is respected when@typeannotations are present.Fixes #1521.
Fixes #1957.