Skip to content

Commit

Permalink
fix(check-types): stop regression to unify *all* Object types inclu…
Browse files Browse the repository at this point in the history 
…ding parent objects unless there is `unifyParentAndChildTypeChecks` config (should only unify with arrays); fixes gajus#800
  • Loading branch information
@brettz9
brettz9 committed Feb 3, 2022
1 parent b67e14a commit 875f135
Show file tree
Hide file tree
Showing 4 changed files with 229 additions and 19 deletions.
27 changes: 19 additions & 8 deletions .README/rules/check-types.md
View file
Expand Up @@ -13,7 +13,7 @@ number
bigint
string
symbol
object
object (For TypeScript's sake, however, using `Object` when specifying child types on it like `Object<string, number>`)
Array
Function
Date
Expand Down Expand Up @@ -72,7 +72,7 @@ the `valid-types` rule to report parsing errors.

Why are `boolean`, `number` and `string` exempt from starting with a capital
letter? Let's take `string` as an example. In Javascript, everything is an
object. The string Object has prototypes for string functions such as
object. The `String` object has prototypes for string functions such as
`.toUpperCase()`.

Fortunately we don't have to write `new String()` everywhere in our code.
Expand All @@ -95,17 +95,28 @@ new String('lard') // String {0: "l", 1: "a", 2: "r", 3: "d", length: 4}
new String('lard') === 'lard' // false
```

To make things more confusing, there are also object literals and object
Objects. But object literals are still static Objects and object Objects are
instantiated Objects. So an object primitive is still an object Object.
To make things more confusing, there are also object literals (like `{}`) and
`Object` objects. But object literals are still static `Object`s and `Object`
objects are instantiated objects. So an object primitive is still an `Object`
object.

However, `Object.create(null)` objects are not `instanceof Object`, however, so
in the case of this Object we lower-case to indicate possible support for
these objects.
in the case of such a plain object we lower-case to indicate possible support
for these objects. Also, nowadays, TypeScript also discourages use of `Object`
as a lone type. However, one additional complexity is that TypeScript allows and
actually [currently requires](https://github.com/microsoft/TypeScript/issues/20555)
`Object` (with the initial upper-case) if used in the syntax
`Object.<keyType, valueType>` or `Object<keyType, valueType`, perhaps to
adhere to what [JSDoc documents](https://jsdoc.app/tags-type.html).

So, for optimal compatibility with TypeScript (especially since TypeScript
tools can be used on plain JavaScript with JSDoc), we are now allowing this
TypeScript approach by default.

Basically, for primitives, we want to define the type as a primitive, because
that's what we use in 99.9% of cases. For everything else, we use the type
rather than the primitive. Otherwise it would all just be `{object}`.
rather than the primitive. Otherwise it would all just be `{object}` (with the
additional exception of the special case of `Object.<>` just mentioned).

In short: It's not about consistency, rather about the 99.9% use case. (And
some functions might not even support the objects if they are checking for
Expand Down
63 changes: 55 additions & 8 deletions README.md
View file
Expand Up @@ -4728,7 +4728,7 @@ number
bigint
string
symbol
object
object (For TypeScript's sake, however, using `Object` when specifying child types on it like `Object<string, number>`)
Array
Function
Date
Expand Down Expand Up @@ -4789,7 +4789,7 @@ the `valid-types` rule to report parsing errors.

Why are `boolean`, `number` and `string` exempt from starting with a capital
letter? Let's take `string` as an example. In Javascript, everything is an
object. The string Object has prototypes for string functions such as
object. The `String` object has prototypes for string functions such as
`.toUpperCase()`.

Fortunately we don't have to write `new String()` everywhere in our code.
Expand All @@ -4812,17 +4812,28 @@ new String('lard') // String {0: "l", 1: "a", 2: "r", 3: "d", length: 4}
new String('lard') === 'lard' // false
```

To make things more confusing, there are also object literals and object
Objects. But object literals are still static Objects and object Objects are
instantiated Objects. So an object primitive is still an object Object.
To make things more confusing, there are also object literals (like `{}`) and
`Object` objects. But object literals are still static `Object`s and `Object`
objects are instantiated objects. So an object primitive is still an `Object`
object.

However, `Object.create(null)` objects are not `instanceof Object`, however, so
in the case of this Object we lower-case to indicate possible support for
these objects.
in the case of such a plain object we lower-case to indicate possible support
for these objects. Also, nowadays, TypeScript also discourages use of `Object`
as a lone type. However, one additional complexity is that TypeScript allows and
actually [currently requires](https://github.com/microsoft/TypeScript/issues/20555)
`Object` (with the initial upper-case) if used in the syntax
`Object.<keyType, valueType>` or `Object<keyType, valueType`, perhaps to
adhere to what [JSDoc documents](https://jsdoc.app/tags-type.html).

So, for optimal compatibility with TypeScript (especially since TypeScript
tools can be used on plain JavaScript with JSDoc), we are now allowing this
TypeScript approach by default.

Basically, for primitives, we want to define the type as a primitive, because
that's what we use in 99.9% of cases. For everything else, we use the type
rather than the primitive. Otherwise it would all just be `{object}`.
rather than the primitive. Otherwise it would all just be `{object}` (with the
additional exception of the special case of `Object.<>` just mentioned).

In short: It's not about consistency, rather about the 99.9% use case. (And
some functions might not even support the objects if they are checking for
Expand Down Expand Up @@ -5456,6 +5467,30 @@ function quux (foo) {
}
// Settings: {"jsdoc":{"mode":"typescript","preferredTypes":{"object.<>":"Object"}}}
// Message: Invalid JSDoc @param "foo" type "object"; prefer: "Object".

/**
* @param {object.<string, number>} foo
*/
function quux (foo) {
}
// Settings: {"jsdoc":{"mode":"typescript","preferredTypes":{"Object":"object","object.<>":"Object<>","Object.<>":"Object<>","object<>":"Object<>"}}}
// Message: Invalid JSDoc @param "foo" type "object"; prefer: "Object<>".

/**
* @param {Object.<string, number>} foo
*/
function quux (foo) {
}
// Settings: {"jsdoc":{"mode":"typescript","preferredTypes":{"Object":"object","object.<>":"Object<>","Object.<>":"Object<>","object<>":"Object<>"}}}
// Message: Invalid JSDoc @param "foo" type "Object"; prefer: "Object<>".

/**
* @param {object<string, number>} foo
*/
function quux (foo) {
}
// Settings: {"jsdoc":{"mode":"typescript","preferredTypes":{"Object":"object","object.<>":"Object<>","Object.<>":"Object<>","object<>":"Object<>"}}}
// Message: Invalid JSDoc @param "foo" type "object"; prefer: "Object<>".
````

The following patterns are not considered problems:
Expand Down Expand Up @@ -5751,6 +5786,18 @@ function quux (foo) {

}
// Settings: {"jsdoc":{"mode":"typescript"}}

/**
* @typedef {object} foo
*/
function a () {}
// Settings: {"jsdoc":{"mode":"typescript","preferredTypes":{"Object":"object","object.<>":"Object<>","object<>":"Object<>"}}}

/**
* @typedef {Object<string, number>} foo
*/
function a () {}
// Settings: {"jsdoc":{"mode":"typescript","preferredTypes":{"Object":"object","object.<>":"Object<>","object<>":"Object<>"}}}
````


Expand Down
23 changes: 20 additions & 3 deletions src/rules/checkTypes.js
View file
Expand Up @@ -156,6 +156,7 @@ export default iterateJsdoc(({

const tagName = jsdocTag.tag;

// eslint-disable-next-line complexity -- To refactor
traverse(typeAst, (node, parentNode, property) => {
const {
type,
Expand Down Expand Up @@ -221,12 +222,28 @@ export default iterateJsdoc(({
]);
} else if (!noDefaults && type === 'JsdocTypeName') {
for (const strictNativeType of strictNativeTypes) {
if (strictNativeType === 'object' && mode === 'typescript' && !preferredTypes?.[nodeName]) {
if (
// Todo: Avoid typescript condition if moving to default typescript
strictNativeType === 'object' && mode === 'typescript' &&
(
// This is not set to remap with exact type match (e.g.,
// `object: 'Object'`), so can ignore (including if circular)
!preferredTypes?.[nodeName] ||
// Although present on `preferredTypes` for remapping, this is a
// parent object without a parent match (and not
// `unifyParentAndChildTypeChecks`) and we don't want
// `object<>` given TypeScript issue https://github.com/microsoft/TypeScript/issues/20555
parentNode?.elements.length && (
parentNode?.left.type === 'JsdocTypeName' &&
parentNode?.left.value === 'Object'
)
)
) {
continue;
}

if (strictNativeType.toLowerCase() === nodeName.toLowerCase() &&
strictNativeType !== nodeName &&
if (strictNativeType !== nodeName &&
strictNativeType.toLowerCase() === nodeName.toLowerCase() &&

// Don't report if user has own map for a strict native type
(!preferredTypes || preferredTypes?.[strictNativeType] === undefined)
Expand Down
135 changes: 135 additions & 0 deletions test/rules/assertions/checkTypes.js
View file
Expand Up @@ -2193,6 +2193,105 @@ export default {
},
},
},
{
code: `
/**
* @param {object.<string, number>} foo
*/
function quux (foo) {
}
`,
errors: [
{
line: 3,
message: 'Invalid JSDoc @param "foo" type "object"; prefer: "Object<>".',
},
],
output: `
/**
* @param {Object<string, number>} foo
*/
function quux (foo) {
}
`,
settings: {
jsdoc: {
mode: 'typescript',
preferredTypes: {
Object: 'object',
'object.<>': 'Object<>',
'Object.<>': 'Object<>',
'object<>': 'Object<>',
},
},
},
},
{
code: `
/**
* @param {Object.<string, number>} foo
*/
function quux (foo) {
}
`,
errors: [
{
line: 3,
message: 'Invalid JSDoc @param "foo" type "Object"; prefer: "Object<>".',
},
],
output: `
/**
* @param {Object<string, number>} foo
*/
function quux (foo) {
}
`,
settings: {
jsdoc: {
mode: 'typescript',
preferredTypes: {
Object: 'object',
'object.<>': 'Object<>',
'Object.<>': 'Object<>',
'object<>': 'Object<>',
},
},
},
},
{
code: `
/**
* @param {object<string, number>} foo
*/
function quux (foo) {
}
`,
errors: [
{
line: 3,
message: 'Invalid JSDoc @param "foo" type "object"; prefer: "Object<>".',
},
],
output: `
/**
* @param {Object<string, number>} foo
*/
function quux (foo) {
}
`,
settings: {
jsdoc: {
mode: 'typescript',
preferredTypes: {
Object: 'object',
'object.<>': 'Object<>',
'Object.<>': 'Object<>',
'object<>': 'Object<>',
},
},
},
},
],
valid: [
{
Expand Down Expand Up @@ -2804,5 +2903,41 @@ export default {
},
},
},
{
code: `
/**
* @typedef {object} foo
*/
function a () {}
`,
settings: {
jsdoc: {
mode: 'typescript',
preferredTypes: {
Object: 'object',
'object.<>': 'Object<>',
'object<>': 'Object<>',
},
},
},
},
{
code: `
/**
* @typedef {Object<string, number>} foo
*/
function a () {}
`,
settings: {
jsdoc: {
mode: 'typescript',
preferredTypes: {
Object: 'object',
'object.<>': 'Object<>',
'object<>': 'Object<>',
},
},
},
},
],
};

0 comments on commit 875f135

Please sign in to comment.