-
Notifications
You must be signed in to change notification settings - Fork 681
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
1618532
f078fac
44018d1
b9f57b4
54ccea1
509aadc
257f54d
227d57f
fe97b6b
678d0c9
665d890
ceaa9c3
9291eae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,293 @@ 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 | ||
|
||
1. 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 | ||
``` | ||
|
||
2. Strada's JS-specific rules for inferring type arguments no apply in Corsa. | ||
sandersn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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. | ||
|
||
### JSDoc Types | ||
|
||
1. 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. | ||
|
||
|
||
2. A variadic type on a parameter no longer makes it a rest parameter. The parameter must use standard rest syntax. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one might need some changes in declaration emit? Or not? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure; the emit changes for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked and declaration emit does the right thing already. |
||
|
||
```js | ||
/** @param {...number} ns */ | ||
function sum(ns) {} | ||
``` | ||
|
||
Must now be written as | ||
|
||
```js | ||
/** @param {...number} ns */ | ||
function sum(...ns) {} | ||
``` | ||
|
||
3. The postfix `=` type no longer adds `undefined` even when `strictNullChecks` is off: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this one just visual? Like, in a parameter position, missing vs undefined is not a real thing that matters. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pretty much. It shows up in the type baselines so I noted it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be "even when ... is on" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
```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 | ||
|
||
1. `@type` tags no longer apply to function declarations, and now contextually type functionn expressions instead of applying directly. So this annotation no longer does anything: | ||
sandersn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
```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. | ||
|
||
2. `asserts` annotation for an arrow function must be on the declaring variable, not on the arrow itself. This no longer works: | ||
jakebailey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
```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. | ||
jakebailey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
3. Error messages on async functions that incorrectly return non-Promises now use the same error as TS. | ||
|
||
4. `@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. | ||
|
||
5. `@class` or `@constructor` does not make a function into a constructor function. | ||
|
||
Corsa ignores `@class` and `@constructor`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
||
6. `@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; | ||
``` | ||
|
||
7. Optional marking on parameter names now makes the parameter both optional and undefined: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this the opposite of the change made to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The difference is that The biggest distinction is that postfix-= is a type, brackets and question mark are parameter syntax. They were both buggy in different ways in Strada. |
||
|
||
```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. | ||
sandersn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
8. 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
### Expandos | ||
|
||
1. Expando assignments of `void 0` are no longer ignored as a special case: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's extra code, and an extra thing to explain, for a pattern that isn't used anymore. |
||
|
||
```js | ||
var o = {} | ||
o.y = void 0 | ||
``` | ||
|
||
creates a property `y: undefined` on `o` (which will widen to `y: any` if strictNullChecks is off). | ||
|
||
2. A this-property expression with a type annotation in the constructor no longer creates a property: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this just a cruft cleanup? Or a technical limitation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's Closure cleanup, and obsoleted by ES standard syntax. It is basically unused as far as I remember from my survey. |
||
|
||
```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; | ||
} | ||
} | ||
``` | ||
|
||
3. 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 | ||
|
||
1. Chained exports no longer work: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this one a cleanup? Or just impossible? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simplification -- it's possible, but adds a bunch of code. |
||
|
||
```js | ||
exports.x = exports.y = 12 | ||
``` | ||
|
||
Now only exports `x`, not `y` as well. | ||
sandersn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
2. Exporting `void 0` is no longer ignored as a special case: | ||
|
||
```js | ||
exports.x = void 0 | ||
// several lines later... | ||
exports.x = theRealExport | ||
sandersn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
``` | ||
|
||
This exports `x: undefined` not `x: typeof theRealExport`. | ||
|
||
3. Type info for `module` shows a property with name of an instead of `exports`: | ||
sandersn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
```js | ||
module.exports = singleIdentifier | ||
``` | ||
|
||
results in `module: { singleIdentifier: any }` | ||
|
||
4. Property access on `require` no longer imports a single property from a module: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we disallowing this one? I've definitely seen this before... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was extremely rare in the survey I did. And it would require significant hacking to the module exports resolution in the checker, too. |
||
|
||
```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") | ||
``` | ||
|
||
5. `Object.defineProperty` on `exports` no longer creates an export: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a feeling this one will bite us... |
||
|
||
```js | ||
Object.defineProperty(exports, "x", { value: 12 }) | ||
``` | ||
|
||
This applies to `module.exports` as well. | ||
Use `exports.x = 12` instead. |
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 |
Uh oh!
There was an error while loading. Please reload this page.