Skip to content

Accept and document jsdoc diffs, round 1 #1426

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
306 changes: 305 additions & 1 deletion CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,311 @@ JSDoc types are parsed in normal type annotation position but show a grammar err

Corsa no longer parses the following JSDoc tags with a specific node type. They now parse as generic JSDocTag nodes.

1. `@class`
1. `@class`/`@constructor`
2. `@throws`
3. `@author`
4. `@enum`

## Checker

### Miscellaneous
#### When `"strict": false`, Corsa no longer allows omitting arguments for parameters with type `undefined`, `unknown`, or `any`:


```js
/** @param {unknown} x */
function f(x) { return x; }
f(); // Previously allowed, now an error
```

`void` can still be omitted, regardless of strict mode:

```js
/** @param {void} x */
function f(x) { return x; }
f(); // Still allowed
```

#### Strada's JS-specific rules for inferring type arguments no longer apply in Corsa.

Inferred type arguments may change. For example:

```js
/** @type {any} */
var x = { a: 1, b: 2 };
var entries = Object.entries(x);
```
In Strada, `entries: Array<[string, any]>`.
In Corsa it has type `Array<[string, unknown]>`, the same as in TypeScript.

#### Values are no longer resolved as types in JSDoc type positions.

```js
/** @typedef {FORWARD | BACKWARD} Direction */
const FORWARD = 1, BACKWARD = 2;
```

Must now use `typeof` the same way TS does:

```js
/** @typedef {typeof FORWARD | typeof BACKWARD} Direction */
const FORWARD = 1, BACKWARD = 2;
```

### JSDoc Types

#### JSDoc variadic types are now only synonyms for array types.

```js
/** @param {...number} ns */
function sum(...ns) {}
```

is equivalent to


```js
/** @param {number[]} ns */
function sum(...ns) {}
```

They have no other semantics.


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

```js
/** @param {...number} ns */
function sum(ns) {}
```

Must now be written as

```js
/** @param {...number} ns */
function sum(...ns) {}
```

#### The postfix `=` type no longer adds `undefined` even when `strictNullChecks` is off

This is a bug in Strada: it adds `undefined` to the type even when `strictNullChecks` is off.
This bug is fixed in Corsa.

```js
/** @param {number=} x */
function f(x) {
return x;
}
```

will now have `x?: number` not `x?: number | undefined` with `strictNullChecks` off.
Regardless of strictness, it still makes parameters optional when used in a `@param` tag.


### JSDoc Tags

#### `@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:

```js

/** @type {(x: unknown) => asserts x is string } */
function assertIsString(x) {
if (!(typeof x === "string")) throw new Error();
}
```

Although this one still works via contextual typing:

```js
/** @typedef {(check: boolean) => asserts check} AssertFunc */

/** @type {AssertFunc} */
const assert = check => {
if (!check) throw new Error();
}
```

A number of things change slightly because of differences between type annotation and contextual typing.

#### `asserts` annotation for an arrow function must be on the declaring variable, not on the arrow itself. This no longer works:

```js
/**
* @param {A} a
* @returns { asserts a is B }
*/
const foo = (a) => {
if (/** @type { B } */ (a).y !== 0) throw TypeError();
return undefined;
};
```

And must be written like this:

```js
/**
* @type {(a: A) => asserts a is B}
*/
const foo = (a) => {
if (/** @type { B } */ (a).y !== 0) throw TypeError();
return undefined;
};
```

This is identical to the Typescript rule.

#### Error messages on async functions that incorrectly return non-Promises now use the same error as TS.

#### `@typedef` and `@callback` in a class body are no longer accessible outside the class.
They must be moved outside the class to use them outside the class.

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

Corsa ignores `@class` and `@constructor`.
Copy link
Member

Choose a reason for hiding this comment

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

For some of these, I hope we can emit a suggestion diagnostic or something at some point to point out that the JSDoc isn't taking effect.

Copy link
Member

Choose a reason for hiding this comment

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

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

This makes a difference on a function without this-property assignments or associated prototype-function assignments.

#### `@param` tags now apply to at most one function.

If they're in a place where they could apply to multiple functions, they apply only to the first one.
If you have `"strict": true`, you will see a noImplicitAny error on the now-untyped parameters.

```js
/** @param {number} x */
var f = x => x, g = x => x;
```

#### Optional marking on parameter names now makes the parameter both optional and undefined:

```js
/** @param {number} [x] */
function f(x) {
return x;
}
```

This behaves the same as Typescript's `x?: number` syntax.
Strada makes the parameter optional but does not add `undefined` to the type.

#### Type assertions with `@type` tags now prevent narrowing of the type.

```js
/** @param {C | undefined} cu */
function f(cu) {
if (/** @type {any} */ (cu).undeclaredProperty) {
cu // still has type C | undefined
}
}
```

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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


### Expandos

#### Expando assignments of `void 0` are no longer ignored as a special case:

```js
var o = {}
o.y = void 0
```

creates a property `y: undefined` on `o` (which will widen to `y: any` if strictNullChecks is off).

#### A this-property expression with a type annotation in the constructor no longer creates a property:

```js
class SharedClass {
constructor() {
/** @type {SharedId} */
this.id;
}
}
```

Provide an initializer or use a property declaration in the class body:

```js
class SharedClass1 {
/** @type {SharedId} */
id;
}
class SharedClass2 {
constructor() {
/** @type {SharedId} */
this.id = 1;
}
}
```

#### Assigning an object literal to the `prototype` property of a function no longer makes it a constructor function:

```js
function Foo() {}
Foo.prototype = {
/** @param {number} x */
bar(x) {
return x;
}
};
```

If you still need to use constructor functions instead of classes, you should declare methods individually on the prototype:

```js
function Foo() {}
/** @param {number} x */
Foo.prototype.bar = function(x) {
return x;
};
```

Although classes are a much better way to write this code.

### CommonJS

#### Chained exports no longer work:

```js
exports.x = exports.y = 12
```

Now only exports `x`, not `y` as well.

#### Exporting `void 0` is no longer ignored as a special case:

```js
exports.x = void 0
// several lines later...
exports.x = theRealExport
```

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

#### Hover for `module` shows a property with name of the export instead of `exports`:

```js
module.exports = singleIdentifier
```

shows a hover type like `module: { singleIdentifier: any }`

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

```js
const x = require("y").x
```

If you can't configure your package to use ESM syntax, you can use destructuring instead:

```js
const { x } = require("y")
```

#### `Object.defineProperty` on `exports` no longer creates an export:

```js
Object.defineProperty(exports, "x", { value: 12 })
```

This applies to `module.exports` as well.
Use `exports.x = 12` instead.
27 changes: 26 additions & 1 deletion testdata/submoduleAccepted.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,28 @@
# Diff files to instead write to submoduleAccepted as "accepted" changes.
conformance/assertionsAndNonReturningFunctions.types.diff
conformance/assertionTypePredicates2.errors.txt.diff
conformance/assignmentToVoidZero1.errors.txt.diff
conformance/assignmentToVoidZero1.types.diff
conformance/asyncArrowFunction_allowJs.errors.txt.diff
conformance/asyncArrowFunction_allowJs.types.diff
conformance/asyncFunctionDeclaration16_es5.errors.txt.diff
conformance/asyncFunctionDeclaration16_es5.types.diff
conformance/binderUninitializedModuleExportsAssignment.types.diff
conformance/callbackOnConstructor.errors.txt.diff
conformance/callbackOnConstructor.types.diff
conformance/callbackTag2.errors.txt.diff
conformance/callbackTag2.types.diff
conformance/callbackTag4.types.diff
conformance/callbackTagNamespace.types.diff
conformance/callbackTagVariadicType.types.diff
conformance/callOfPropertylessConstructorFunction.errors.txt.diff
conformance/callOfPropertylessConstructorFunction.types.diff
conformance/callWithMissingVoidUndefinedUnknownAnyInJs(strict=false).errors.txt.diff
conformance/checkExportsObjectAssignProperty.errors.txt.diff
conformance/checkExportsObjectAssignProperty.types.diff
conformance/checkJsdocOnEndOfFile.types.diff
conformance/checkJsdocParamOnVariableDeclaredFunctionExpression.errors.txt.diff
conformance/checkJsdocParamOnVariableDeclaredFunctionExpression.types.diff
conformance/checkParamTag1.types.diff
conformance/node10Alternateresult_noTypes.errors.txt.diff
conformance/node10AlternateResult_noResolution.errors.txt.diff
conformance/node10AlternateResult_noResolution.errors.txt.diff
Loading