Accept and document jsdoc diffs, round 1 #1426

sandersn · 2025-07-21T13:10:51Z

I started adding precise differences for JS semantics to CHANGES.md. As you can see, I accepted 25 tests--and looked at around 40--before I got bored and switched to compiling well-known JSDoc codebases (webpack and svelte). The changes include a couple of diffs from Svelte as well.

I'm not 100% sure that this is worth generating. At some point we may instead want to declare legacy test bankruptcy, if people appear to be using JS features with no complaint. However, the first 2.5% of accepted tests probably has 25% of the semantics changes, so this list is at least worth reviewing by the team for surprises.

A good number of the diffs are due to the reparsing strategy, which gives JS exactly the semantics of TS. The rest are due to cut features, both whole features (eg type annotations on functions) and partial (eg use of @class to mark an otherwise-normal function as a constructor).

Copilot

Pull Request Overview

This PR documents and accepts 25 differences in JSDoc test outputs between the legacy TypeScript implementation (Strada) and the new native port (Corsa). The changes primarily reflect semantic differences in JavaScript handling, with many arising from the new reparsing strategy that gives JS exactly the same semantics as TypeScript.

Accepts 25 test diffs by adding them to the submoduleAccepted.txt file
Documents comprehensive semantic changes in CHANGES.md covering checker behavior, JSDoc types/tags, expandos, and CommonJS modules
Includes differences discovered from compiling real-world JSDoc codebases like webpack and svelte

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
testdata/submoduleAccepted.txt	Adds 25 conformance test diffs to the accepted changes list
CHANGES.md	Extensive documentation of semantic differences between Strada and Corsa implementations

CHANGES.md

Co-authored-by: Copilot <[email protected]>

jakebailey

Mostly my comments are not about the text but about the actual differences themselves ☹️

jakebailey · 2025-07-21T16:39:19Z

CHANGES.md

+
+### JSDoc Tags
+
+1. `@type` tags no longer apply to function declarations, and now contextually type function expressions instead of applying directly. So this annotation no longer does anything:


I still really really think we should reconsider this and come up with some sort of way to do it

The right way is to make a WholeType property for functions and check that if the function's Type and parameter types are nil, then only set WholeType in JS. It's possible but I'd put it even below constructor functions and maybe even Object.defineProperty expandos in importance, since contextual typing occurs in nearly all uses of it.

Related sveltejs/svelte#16485 (comment)

CHANGES.md

jakebailey · 2025-07-21T16:40:32Z

CHANGES.md

+function sum(...ns) {}
+```
+
+3. The postfix `=` type no longer adds `undefined` even when `strictNullChecks` is off:


Isn't this one just visual? Like, in a parameter position, missing vs undefined is not a real thing that matters.

Pretty much. It shows up in the type baselines so I noted it.

Right; who is the target of this doc? I had figured this was for end users to read when they find something that doesn't work, so this particular one seemed not enough to mention.

Should this be "even when ... is on"

No, it is supposed to add undefined when it is on. The Strada bug is that it adds it even when strict is off. I will make this note explain it better.

CHANGES.md

jakebailey · 2025-07-21T16:45:39Z

CHANGES.md

+
+### CommonJS
+
+1. Chained exports no longer work:


Is this one a cleanup? Or just impossible?

Simplification -- it's possible, but adds a bunch of code.

CHANGES.md

jakebailey · 2025-07-21T16:47:30Z

CHANGES.md

+
+This exports `x: undefined` not `x: typeof theRealExport`.
+
+3. Type info for `module` shows a property with name of the export instead of `exports`:


I'm not sure what this one means. Are you talking about hover? or something else?

module, and exports are different symbols entirely, right?

It's for hover, yes. module and exports are different symbols.

Probably this is one we can avoid mentioning for end users. But the doc doesn't structure it like that of course so it's okay.

jakebailey · 2025-07-21T16:48:04Z

CHANGES.md

+
+results in `module: { singleIdentifier: any }`
+
+4. Property access on `require` no longer imports a single property from a module:


Why are we disallowing this one? I've definitely seen this before...

It was extremely rare in the survey I did. And it would require significant hacking to the module exports resolution in the checker, too.

jakebailey · 2025-07-21T16:48:22Z

CHANGES.md

+const { x } = require("y")
+```
+
+5. `Object.defineProperty` on `exports` no longer creates an export:


I have a feeling this one will bite us...

weswigham

This looks like a useful summary of where we are right now. A question, though:

A good number of the diffs are due to the reparsing strategy, which gives JS exactly the semantics of TS.

Should we be applying changes to https://github.com/microsoft/TypeScript/tree/tsgo-port to have a strada version with matching semantics for generating matching baselines? Seems like it'd be useful to have a JS version of the compiler that can report the same diagnostics (and maybe provide upgrade fixes? 6.0?).

CHANGES.md

weswigham · 2025-07-21T18:38:12Z

CHANGES.md

+```
+
+In Strada, `cu` incorrectly narrows to `C` inside the `if` block, unlike with TS assertion syntax.
+In Corsa, the behaviour is the same between TS and JS.


...should we change this TS behavior? Seems like a bug, honestly. The presence of the cast doesn't make the truthiness check not present.

I have trouble keeping the precise semantics of narrowing in my head, but if (cu.undeclaredProperty) doesn't seem like a truthiness check compared to if (cu?.undeclaredProperty). I guess implicitly it is--"we are here without a crash, so cu must exist". Also casts are often an escape hatch for when the compiler goes wrong, so maybe that's why that's here.

weswigham · 2025-07-21T18:39:10Z

CHANGES.md

+
+5. `@class` or `@constructor` does not make a function into a constructor function.
+
+Corsa ignores `@class` and `@constructor`.


We'd want it to emit the "convert to class declaration" quickfix on, anyway, right?

weswigham · 2025-07-21T18:41:03Z

CHANGES.md

+They have no other semantics.
+
+
+2. A variadic type on a parameter no longer makes it a rest parameter. The parameter must use standard rest syntax.


This one might need some changes in declaration emit? Or not? ...number still get changed to number[], which is.... still right, I think? Ah, whatever, mental note to check on it.

I'm not sure; the emit changes for sum but unless you ported the special-case check already, the correct emit is function sum(ns: number[]) and that's what the checker should give you if you're generating from a type.

I checked and declaration emit does the right thing already.

sandersn added 4 commits June 25, 2025 11:05

First round of jsdoc diff acceptance/documentation

1618532

Merge branch 'main' into accept-jsdoc-diffs-1

f078fac

conformance/ round 1 done through checkExportsObjectAssignProperty

44018d1

Done through checkParamTag1 + a little svelte

b9f57b4

Copilot AI review requested due to automatic review settings July 21, 2025 13:10

Copilot AI reviewed Jul 21, 2025

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

CHANGES.md Outdated Show resolved Hide resolved

CHANGES.md Outdated Show resolved Hide resolved

sandersn and others added 4 commits July 21, 2025 06:13

Merge branch 'main' into accept-jsdoc-diffs-1

54ccea1

Apply suggestion from @Copilot

509aadc

Co-authored-by: Copilot <[email protected]>

Apply suggestion from @Copilot

257f54d

Co-authored-by: Copilot <[email protected]>

Apply suggestion from @Copilot

227d57f

Co-authored-by: Copilot <[email protected]>

jakebailey reviewed Jul 21, 2025

View reviewed changes

Merge branch 'main' into accept-jsdoc-diffs-1

fe97b6b

weswigham approved these changes Jul 21, 2025

View reviewed changes

sandersn added 4 commits July 22, 2025 06:39

reformat entries, add important entry at start

678d0c9

expand some too-short explanations

665d890

Merge branch 'main' into accept-jsdoc-diffs-1

ceaa9c3

Merge branch 'main' into accept-jsdoc-diffs-1

9291eae


		### JSDoc Tags

		1. `@type` tags no longer apply to function declarations, and now contextually type function expressions instead of applying directly. So this annotation no longer does anything:


		This exports `x: undefined` not `x: typeof theRealExport`.

		3. Type info for `module` shows a property with name of the export instead of `exports`:


		results in `module: { singleIdentifier: any }`

		4. Property access on `require` no longer imports a single property from a module:


		5. `@class` or `@constructor` does not make a function into a constructor function.

		Corsa ignores `@class` and `@constructor`.

		They have no other semantics.


		2. A variadic type on a parameter no longer makes it a rest parameter. The parameter must use standard rest syntax.

Accept and document jsdoc diffs, round 1 #1426

Are you sure you want to change the base?

Accept and document jsdoc diffs, round 1 #1426

Uh oh!

Conversation

sandersn commented Jul 21, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jakebailey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sandersn Jul 23, 2025 •

edited

Loading