Fixed types of parenthesized expressions at locations with @satisfies and @type mix
#61852
Conversation
…s` and `@type` mix
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
1 similar comment
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
| if ( | ||
| (isJSDocTypeOrSatisfiesTag(tag)) && some(result, t => { | ||
| return isJSDoc(t) ? some(t.tags, isJSDocTypeOrSatisfiesTag) : isJSDocTypeOrSatisfiesTag(t); | ||
| }) | ||
| ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
the rationale is that in TS files a single node can't own both at the same time
There was a problem hiding this comment.
Pull Request Overview
This PR fixes how JSDoc @satisfies and @type tags are associated with parenthesized expressions by tracking previously collected tags and preventing duplicates. It also adds a helper to detect both tag types and updates tests and baselines for the new behavior.
- Track existing JSDoc tags in
getJSDocCommentsAndTagsandfilterOwnedJSDocTagsto avoid reassigning type/satisfies tags. - Introduce
isJSDocTypeOrSatisfiesTaghelper to unify tag checks. - Add new test
checkJsdocSatisfiesTag16.tsand update baselines across severalcheckJsdocSatisfiesTagcases.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/compiler/utilities.ts | Updated JSDoc filtering logic, added result parameter and helper function |
| tests/cases/conformance/jsdoc/checkJsdocSatisfiesTag16.ts | New test for parenthesized expressions with @satisfies tags |
| tests/baselines/reference/* | Updated baselines to reflect the new output and error counts |
Comments suppressed due to low confidence (2)
src/compiler/utilities.ts:4613
- [nitpick] The parameter name
resultis ambiguous. Consider renaming it to something more descriptive likeexistingTagsorcollectedTagsto clarify its purpose.
function filterOwnedJSDocTags(hostNode: Node, comments: JSDocArray, result: (JSDoc | JSDocTag)[] | undefined) {
src/compiler/utilities.ts:4644
- [nitpick] Consider adding a brief JSDoc comment for
isJSDocTypeOrSatisfiesTagto explain what conditions it checks, improving maintainability.
function isJSDocTypeOrSatisfiesTag(tag: JSDocTag) {
| @@ -23,8 +22,6 @@ | |||
|
|
|||
| /** | |||
| * @satisfies {number} | |||
There was a problem hiding this comment.
as shown by the new tests/cases/conformance/jsdoc/checkJsdocSatisfiesTag16.ts both are still checked - but they are no longer treated as duplicates. I think this is fine but maybe @sandersn would have a different opinion
There was a problem hiding this comment.
It's correct to drop the error, but the first tag shouldn't even be checked--the comment on ownsJSDocTag says that satisfies tags not on parentheses don't apply to anything. I think the real bug is that type tags shouldn't apply to the parenthesized expression in this case either, but fails because @type usage is ambiguous.
| @@ -5,8 +5,8 @@ | |||
| let obj = /** @satisfies {{ g(s: string): void } & Record<string, unknown>} */ ({ | |||
| >obj : { f(s: string): void; } & Record<string, unknown> | |||
| > : ^^^^ ^^ ^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | |||
| >({ f(s) { }, // "incorrect" implicit any on 's' g(s) { }}) : { f(s: string): void; } & Record<string, unknown> | |||
| > : ^^^^ ^^ ^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | |||
| >({ f(s) { }, // "incorrect" implicit any on 's' g(s) { }}) : { f(s: any): void; g(s: string): void; } | |||
There was a problem hiding this comment.
as we can see here - type of this parenthesized expression is now the same as the inner's expression, which I think is the actual desired outcome (and the actual result as implemented by microsoft/typescript-go#1157 )
| @@ -74,7 +74,7 @@ const t5 = /** @satisfies {T3} */((m) => m.substring(0)); | |||
| >t5 : (m: string) => string | |||
| > : ^ ^^ ^^^^^ | |||
| >((m) => m.substring(0)) : (m: string) => string | |||
| > : ^ ^^ ^^^^^ | |||
There was a problem hiding this comment.
Sort of interesting, given the old one showed reuse here?
There was a problem hiding this comment.
It makes some sense - this is now the function expression's type that is contextually-typed by the @satisfies tag and not the annotation type coming from @type. @type's content can be reused (I guess) but the expression's type is "unique" so its parts can't be reused as easily
There was a problem hiding this comment.
I don't think the problem is that no more than one tag should apply, but that @type tags shouldn't apply like a caat when they aren't actually on the parenthesized expression.
I also don't think this is worth fixing in Strada. JS support is a known difference for Corsa, and will be better but stricter in many cases.
| @@ -23,8 +22,6 @@ | |||
|
|
|||
| /** | |||
| * @satisfies {number} | |||
There was a problem hiding this comment.
It's correct to drop the error, but the first tag shouldn't even be checked--the comment on ownsJSDocTag says that satisfies tags not on parentheses don't apply to anything. I think the real bug is that type tags shouldn't apply to the parenthesized expression in this case either, but fails because @type usage is ambiguous.
github-project-automation
bot
moved this from Not started
to Waiting on author
in PR Backlog
see https://github.com/microsoft/typescript-go/pull/1157/files/e9100512827813e64556ef3d1fadd36e606d8677#r2139735507
cc @sandersn @jakebailey