-
Notifications
You must be signed in to change notification settings - Fork 645
Add this/satisfies tag #1157
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
Add this/satisfies tag #1157
Conversation
This is the last hosted tag. Just callback is left.
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
Adds support for JSDoc’s @satisfies
and @this
tags by introducing a unified cast helper and extending the parser’s reparse logic
- Refactor existing type-assertion helper into
makeNewCast
with anisAssertion
flag - Extend
reparseHosted
to handle@satisfies
tags on parenthesized expressions - Add logic in
reparseHosted
to inject athis
parameter for functions annotated with@this
Reviewed Changes
Copilot reviewed 80 out of 80 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
internal/parser/reparser.go | Replace makeNewTypeAssertion calls with makeNewCast , add cases for JSDoc @satisfies and @this |
testdata/baselines/** | Update expected types, symbols, and errors to reflect new @satisfies /@this behavior |
testdata/baselines/reference/submodule/conformance/checkJsdocSatisfiesTag14.symbols.diff
Outdated
Show resolved
Hide resolved
@@ -7,8 +7,9 @@ | |||
->p : { isEven: (n: number) => boolean; isOdd: (n: number) => boolean; } | |||
->({ isEven: n => n % 2 === 0, isOdd: n => n % 2 === 1}) : { isEven: (n: number) => boolean; isOdd: (n: number) => boolean; } | |||
->{ isEven: n => n % 2 === 0, isOdd: n => n % 2 === 1} : { isEven: (n: number) => boolean; isOdd: (n: number) => boolean; } | |||
+>p : { isEven: (n: any) => boolean; isOdd: (n: any) => boolean; } | |||
+>({ isEven: n => n % 2 === 0, isOdd: n => n % 2 === 1}) : { isEven: (n: any) => boolean; isOdd: (n: any) => boolean; } | |||
+>p : any |
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 is now any
because Corsa no longer supports index signature syntax like Object.<string, T>
testdata/baselines/reference/submoduleAccepted/conformance/checkJsdocSatisfiesTag3.types.diff
Show resolved
Hide resolved
@@ -7,8 +7,9 @@ | |||
->x : { m: true; s: string; } | |||
->({ m: true, s: "false"}) : { m: true; s: string; } | |||
->{ m: true, s: "false"} : { m: true; s: string; } | |||
+>x : { m: boolean; s: string; } | |||
+>({ m: true, s: "false"}) : { m: boolean; s: string; } | |||
+>x : any |
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.
same; more unsupported syntax
testdata/baselines/reference/submoduleAccepted/conformance/checkJsdocSatisfiesTag3.types.diff
Show resolved
Hide resolved
testdata/baselines/reference/submoduleAccepted/conformance/checkJsdocSatisfiesTag9.types.diff
Outdated
Show resolved
Hide resolved
print their children but not the nodes themselves. This makes the baselines basically identical to the Strada ones.
->this : Symbol(this) | ||
+>this : Symbol(C, Decl(a.js, 0, 0)) | ||
+>this : Symbol((Missing), Decl(a.js, 3, 8)) |
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 am curious what the deal is with (Missing)
here, hopefully not a problem.
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.
iirc, it means the Name
node of the symbol is missing/undefined, (and this isn't a class (it's the class static side), so it isn't (anonymous class)
). Probably a bug somewhere, since this
is jsdoc assigned to type T
, so it really aughta be T
, if not the generic unresolved this
symbol. Perhaps lookup of the names of jsdoc template type params is broken?
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.
#731 potentially
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.
yes, lookup of template types isbroken. I'm working on a fix right now as part of adding @callback
and refactoring @typedef
support.
@@ -266,6 +271,31 @@ func (p *Parser) reparseHosted(tag *ast.Node, parent *ast.Node, jsDoc *ast.Node) | |||
} | |||
} | |||
} | |||
case ast.KindJSDocThisTag: | |||
if fun, ok := getFunctionLikeHost(parent); ok { |
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.
What's this supposed to do for function-like-expressions that don't have a this
, like arrow functions?
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.
Copilot already noticed this and I replied that there's already an error logged so it's OK with me to have wonky semantics in a file with errors
These are the last hosted tags!
I'm not sure about the name
makeNewCast
; copilot originally suggested two separate functions, which felt a little repetitive to me.