Fix js missing type arguments on existing nodes and jsdoc object literal declaration emit #38368

weswigham · 2020-05-06T19:53:00Z

…ral declaration emit

DanielRosenwasser · 2020-05-06T19:54:34Z

Not sure if we baseline outputs for .d.ts files but

@typescript-bot test this
@typescript-bot user test this

typescript-bot · 2020-05-06T19:54:39Z

Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at c827007. You can monitor the build here.

typescript-bot · 2020-05-06T19:54:39Z

Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at c827007. You can monitor the build here.

weswigham · 2020-05-06T19:55:26Z

Not sure if we baseline outputs for .d.ts files

We do not. Extended test suites are entirely useless for observing changes in jsdoc declaration emit~

DanielRosenwasser · 2020-05-06T19:55:30Z

@typescript-bot pack this

typescript-bot · 2020-05-06T19:55:33Z

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at c827007. You can monitor the build here.

typescript-bot · 2020-05-06T20:20:48Z

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

weswigham · 2020-05-06T21:08:35Z

@typescript-bot cherry-pick this to release-3.9

typescript-bot · 2020-05-06T21:08:37Z

Heya @weswigham, I've started to run the task to cherry-pick this into release-3.9 on this PR at c827007. You can monitor the build here.

typescript-bot · 2020-05-06T21:13:01Z

Hey @weswigham, I've opened #38372 for you.

Component commits: c827007 Fix js missing type arguments on existing nodes and jsdoc object literal declaration emit

DanielRosenwasser · 2020-05-06T23:20:29Z

src/compiler/checker.ts

@@ -5503,6 +5503,10 @@ namespace ts {
                return symbol.declarations && find(symbol.declarations, s => !!getEffectiveTypeAnnotationNode(s) && (!enclosingDeclaration || !!findAncestor(s, n => n === enclosingDeclaration)));
            }

+            function getExistingNodeHasNoTypeParametersOrMatchingTypeParameters(existing: TypeNode, type: Type) {
+                return !(getObjectFlags(type) & ObjectFlags.Reference) || !isTypeReferenceNode(existing) || length(existing.typeArguments) >= getMinTypeArgumentCount((type as TypeReference).target.typeParameters);


This is kinda surprising to me - this checks whether the existing type has more type arguments which I'd assume to be the opposite condition.

Yeah, if the existing declaration provides the minimum required or more, it's good to reuse; how's that surprising?

Maybe it's a naming issue? What is the meaning of hasNoTypeParameters in this case?

DanielRosenwasser · 2020-05-06T23:20:42Z

src/compiler/checker.ts

@@ -5503,6 +5503,10 @@ namespace ts {
                return symbol.declarations && find(symbol.declarations, s => !!getEffectiveTypeAnnotationNode(s) && (!enclosingDeclaration || !!findAncestor(s, n => n === enclosingDeclaration)));
            }

+            function getExistingNodeHasNoTypeParametersOrMatchingTypeParameters(existing: TypeNode, type: Type) {


Should this start with the word get?

By convention, yes - pretty much all of our complex internal functions start with verbs, get, create, serialize, and so on.

It makes completions easier to trigger, tbh.

What about predicate functions like typeHasCallOrConstructSignatures? Doesn't seem appropriate to start with a get here IMO

but emacs doesn't show completions until I've typed 3 characters!

I renamed it to existingTypeNodeIsNotReferenceOrIsReferenceWithCompatibleTypeArgumentCount which surely isn't a mouthful. I hope the removal of the verb is to your liking?

named perfectly!

Jack-Works · 2020-05-07T01:40:35Z

Hi! @weswigham There're also some invalid JSDoc basic type generation, can you also check this out?

/** @type {Boolean} */const bool = true;
/** @type {bool} */const bool2 = true;

/** @type {Object} */const obj = true;

/** @type {String} */const str = true;

/** @type {Function} */const f = true;
/** @type {function} */const f2 = true;

/** @type {class} */const clz = true;

/** @type {int} */const x3 = true;
/** @type {float} */const x4 = true;
/** @type {Number} */const x5 = true;
/** @type {integer} */const x6 = true;

Code

/** @type {Boolean} */ declare const bool: Boolean; // expect boolean
/** @type {bool} */ declare const bool2: bool; // expect boolean

/** @type {Object} */ declare const obj: Object; // expect object

/** @type {String} */ declare const str: String; // expect string

/** @type {Function} */ declare const f: Function; // expect (...args:  any[]) => any
/** @type {function} */ declare const f2: function; // ! Invalid output ! expect (...args:  any[]) => any

/** @type {class} */ declare const clz: any; // expect { new (...args: any[]): any }

/** @type {int} */ declare const x3: any; // expect number
/** @type {float} */ declare const x4: any; // expect number
/** @type {Number} */ declare const x5: Number; // expect number
/** @type {integer} */ declare const x6: any; // expect number

I found this when I generate types for a real world JSDoc project, it's nice if TypeScript can handle this.

(My code generation project)

weswigham · 2020-05-07T02:24:55Z

I... Don't think we recognize int or float. But the others... Yeah, ok.

weswigham · 2020-05-08T01:10:13Z

@Jack-Works I've added an explicit testcase for our declaration emit for all the special lookups (as enumerated in our codebase), and included extra entries for the types you mention which we don't currently recognize, including class, bool, int, integer, float, and event (all of which are lookup failures, except event which is coincidentally the name of a global we return the type of, Event | undefined).

Jack-Works · 2020-05-08T01:19:16Z

tests/baselines/reference/jsDeclarationsJSDocRedirectedLookups.js

+/** @type {function} */ declare const h: Function;
+/** @type {array} */ declare const i: any[];
+/** @type {promise} */ declare const j: Promise<any>;
+/** @type {Object<string, string>} */ declare const k: {


What about add a Object => object?

That's actually not a thing we interpret - Object with type arguments is interpreted as an index signature, without isn't recognized special (so it looks up the global Object constructor type, which is the apparent type of... pretty much everything except null and undefined) except when noImplicitAny is false, where it is any.

getIntendedTypeFromJSDocTypeReference in the checker has the special jsdoc types that we lookup in it.

Hmm, but the handbook says

Don’t ever use the types Number, String, Boolean, Symbol, or Object These types refer to non-primitive boxed objects that are almost never used appropriately in JavaScript code.

Instead of Object, use the non-primitive object type (added in TypeScript 2.2).

I think that the point is to test out stuff might people write in JSDoc, because people often write terrible stuff in JSDoc because it was terribly specified.

Jack-Works · 2020-05-08T01:22:18Z

tests/baselines/reference/prototypePropertyAssignmentMergeWithInterfaceMethod.types

@@ -40,14 +40,14 @@ lf.Transaction = function() {};
 * @return {!IThenable}
 */
 lf.Transaction.prototype.begin = function(scope) {};
->lf.Transaction.prototype.begin = function(scope) {} : (scope: Array<lf.schema.Table>) => any
+>lf.Transaction.prototype.begin = function(scope) {} : (scope: Array<any>) => any


Is it a regression?

lf.schema.Table doesn't actually exist in the test, so we were preserving a node that failed to resolve anyway. Technically we could keep it (since it's an error anyway), but since i changed the default for jsdoc type which fail to resolve in not-jsdoc to serialize the resolved type, it now prints back as the any the failed resolution returns.

Jack-Works · 2020-05-08T01:22:44Z

tests/baselines/reference/jsdocTypeReferenceToValue.types

@@ -1,7 +1,7 @@
 === tests/cases/conformance/jsdoc/foo.js ===
 /** @param {Image} image */
 function process(image) {
->process : (image: Image) => HTMLImageElement
+>process : (image: new (width?: number, height?: number) => HTMLImageElement) => HTMLImageElement


Yeah, Image is a constructor with no associated interface, and jsdoc lookup is strange. So, in TS, (image: Image) => HTMLImageElement will issue a Image is not a type error (because it's not). So we gotta serialize the type the jsdoc resolver resolved to instead of keeping the input node.

DanielRosenwasser · 2020-05-08T07:01:49Z

@typescript-bot cherry-pick this to release-3.9

typescript-bot · 2020-05-08T07:01:55Z

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-3.9 on this PR at 04e6b6f. You can monitor the build here.

typescript-bot · 2020-05-08T07:06:12Z

Hey @DanielRosenwasser, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-3.9 manually.

Component commits: c827007 Fix js missing type arguments on existing nodes and jsdoc object literal declaration emit 128ef93 Merge branch 'master' into js-declaration-fixes-mk2 a90f97c Add special lookups test case, rename helper 04e6b6f Accept slightly modified baselines

🤖 Pick PR #38368 (Fix js missing type arguments on ex...) into release-3.9

@type

* upstream/master: (54 commits) LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. Fix for jsdoc modifiers on constructor params (microsoft#38403) Improve assert message in binder (microsoft#38270) fix broken regex on "src/services/completions.ts#getCompletionData" (microsoft#37546) report error for duplicate @type declaration (microsoft#38340) fix(38073): hide 'Extract to function in global scope' action for arrow functions which use 'this' (microsoft#38107) Update user baselines (microsoft#38472) Update user baselines (microsoft#38405) Changed template strings to emit void 0 instead of undefined (microsoft#38430) Fix js missing type arguments on existing nodes and jsdoc object literal declaration emit (microsoft#38368) LEGO: check in for master to temporary branch. Make isDynamicFileName available publicly (microsoft#38269) LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. Exclude arrays and tuples from full intersection property check (microsoft#38395) Fix crash caused by assertion with evolving array type (microsoft#38398) Update user baselines (microsoft#38128) LEGO: check in for master to temporary branch. moveToNewFile: handle namespace imports too ... # Conflicts: # src/compiler/types.ts # src/compiler/utilities.ts

Fix js missing type arguments on existing nodes and jsdoc object lite…

c827007

…ral declaration emit

typescript-bot assigned weswigham May 6, 2020

typescript-bot added the Author: Team label May 6, 2020

weswigham mentioned this pull request May 6, 2020

Regression: DTS emit #38356

Closed

RyanCavanaugh added this to the TypeScript 3.9.2 milestone May 6, 2020

typescript-bot mentioned this pull request May 6, 2020

🤖 User test baselines have changed for js-declaration-fixes-mk2 typescript-bot/TypeScript#31

Open

typescript-bot mentioned this pull request May 6, 2020

🤖 Pick PR #38368 (Fix js missing type arguments on ex...) into release-3.9 #38372

Merged

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request May 6, 2020

Cherry-pick PR microsoft#38368 into release-3.9

557f2b0

Component commits: c827007 Fix js missing type arguments on existing nodes and jsdoc object literal declaration emit

DanielRosenwasser requested review from sandersn and sheetalkamat May 6, 2020 23:13

DanielRosenwasser reviewed May 6, 2020

View reviewed changes

weswigham added 3 commits May 7, 2020 17:35

Merge branch 'master' into js-declaration-fixes-mk2

128ef93

Add special lookups test case, rename helper

a90f97c

Accept slightly modified baselines

04e6b6f

Jack-Works reviewed May 8, 2020

View reviewed changes

sheetalkamat approved these changes May 8, 2020

View reviewed changes

weswigham merged commit 7b03835 into microsoft:master May 8, 2020

DanielRosenwasser added a commit that referenced this pull request May 8, 2020

Merge pull request #38372 from typescript-bot/pick/38368/release-3.9

a073560

🤖 Pick PR #38368 (Fix js missing type arguments on ex...) into release-3.9

Jack-Works mentioned this pull request May 18, 2020

More invalid JSDoc generate cases #38626

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix js missing type arguments on existing nodes and jsdoc object literal declaration emit #38368

Fix js missing type arguments on existing nodes and jsdoc object literal declaration emit #38368

weswigham commented May 6, 2020

DanielRosenwasser commented May 6, 2020

typescript-bot commented May 6, 2020 •

edited

Loading

typescript-bot commented May 6, 2020 •

edited

Loading

weswigham commented May 6, 2020

DanielRosenwasser commented May 6, 2020

typescript-bot commented May 6, 2020 •

edited

Loading

typescript-bot commented May 6, 2020

weswigham commented May 6, 2020

typescript-bot commented May 6, 2020 •

edited

Loading

typescript-bot commented May 6, 2020

DanielRosenwasser May 6, 2020

weswigham May 7, 2020

DanielRosenwasser May 7, 2020

DanielRosenwasser May 6, 2020

weswigham May 7, 2020 •

edited

Loading

DanielRosenwasser May 7, 2020 •

edited

Loading

sandersn May 7, 2020

weswigham May 8, 2020

DanielRosenwasser May 8, 2020

Jack-Works commented May 7, 2020

weswigham commented May 7, 2020

weswigham commented May 8, 2020

Jack-Works May 8, 2020 •

edited

Loading

weswigham May 8, 2020

Jack-Works May 8, 2020

DanielRosenwasser May 8, 2020 •

edited

Loading

Jack-Works May 8, 2020

weswigham May 8, 2020

Jack-Works May 8, 2020

weswigham May 8, 2020

DanielRosenwasser commented May 8, 2020

typescript-bot commented May 8, 2020 •

edited

Loading

typescript-bot commented May 8, 2020

Fix js missing type arguments on existing nodes and jsdoc object literal declaration emit #38368

Fix js missing type arguments on existing nodes and jsdoc object literal declaration emit #38368

Conversation

weswigham commented May 6, 2020

DanielRosenwasser commented May 6, 2020

typescript-bot commented May 6, 2020 • edited Loading

typescript-bot commented May 6, 2020 • edited Loading

weswigham commented May 6, 2020

DanielRosenwasser commented May 6, 2020

typescript-bot commented May 6, 2020 • edited Loading

typescript-bot commented May 6, 2020

weswigham commented May 6, 2020

typescript-bot commented May 6, 2020 • edited Loading

typescript-bot commented May 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham May 7, 2020 • edited Loading

Choose a reason for hiding this comment

DanielRosenwasser May 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jack-Works commented May 7, 2020

weswigham commented May 7, 2020

weswigham commented May 8, 2020

Jack-Works May 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser May 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser commented May 8, 2020

typescript-bot commented May 8, 2020 • edited Loading

typescript-bot commented May 8, 2020

typescript-bot commented May 6, 2020 •

edited

Loading

typescript-bot commented May 6, 2020 •

edited

Loading

typescript-bot commented May 6, 2020 •

edited

Loading

typescript-bot commented May 6, 2020 •

edited

Loading

weswigham May 7, 2020 •

edited

Loading

DanielRosenwasser May 7, 2020 •

edited

Loading

Jack-Works May 8, 2020 •

edited

Loading

DanielRosenwasser May 8, 2020 •

edited

Loading

typescript-bot commented May 8, 2020 •

edited

Loading