Skip to content

fix(check-types): prevent parent objects from being reported in "typescript" mode #835

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

Merged
merged 1 commit into from
Feb 3, 2022
Merged
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
27 changes: 19 additions & 8 deletions .README/rules/check-types.md
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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<>',
},
},
},
},
],
};