Fix js missing type arguments on existing nodes and jsdoc object literal declaration emit#38368
Uh oh!
There was an error while loading. Please reload this page.
Conversation
…ral declaration emit
DanielRosenwasser commented May 6, 2020
Not sure if we baseline outputs for .d.ts files but @typescript-bot test this |
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at c827007. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at c827007. You can monitor the build here. |
weswigham commented May 6, 2020
We do not. Extended test suites are entirely useless for observing changes in jsdoc declaration emit~ |
DanielRosenwasser commented May 6, 2020
@typescript-bot pack this |
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at c827007. You can monitor the build here. |
typescript-bot commented May 6, 2020
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
weswigham commented May 6, 2020
@typescript-bot cherry-pick this to release-3.9 |
Heya @weswigham, I've started to run the task to cherry-pick this into |
typescript-bot commented May 6, 2020
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
| } | ||
| function getExistingNodeHasNoTypeParametersOrMatchingTypeParameters(existing: TypeNode, type: Type){ | ||
| return !(getObjectFlags(type) & ObjectFlags.Reference) || !isTypeReferenceNode(existing) || length(existing.typeArguments) >= getMinTypeArgumentCount((type as TypeReference).target.typeParameters); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, if the existing declaration provides the minimum required or more, it's good to reuse; how's that surprising?
There was a problem hiding this comment.
Maybe it's a naming issue? What is the meaning of hasNoTypeParameters in this case?
src/compiler/checker.ts Outdated
| return symbol.declarations && find(symbol.declarations, s => !!getEffectiveTypeAnnotationNode(s) && (!enclosingDeclaration || !!findAncestor(s, n => n === enclosingDeclaration))); | ||
| } | ||
| function getExistingNodeHasNoTypeParametersOrMatchingTypeParameters(existing: TypeNode, type: Type){ |
There was a problem hiding this comment.
Should this start with the word get?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What about predicate functions like typeHasCallOrConstructSignatures? Doesn't seem appropriate to start with a get here IMO
There was a problem hiding this comment.
but emacs doesn't show completions until I've typed 3 characters!
There was a problem hiding this comment.
I renamed it to existingTypeNodeIsNotReferenceOrIsReferenceWithCompatibleTypeArgumentCount which surely isn't a mouthful. I hope the removal of the verb is to your liking?
Jack-Works commented May 7, 2020
Hi! @weswigham There're also some invalid JSDoc basic type generation, can you also check this out? /** @type{Boolean} */constbool=true;/** @type{bool} */constbool2=true;/** @type{Object} */constobj=true;/** @type{String} */conststr=true;/** @type{Function} */constf=true;/** @type{function} */constf2=true;/** @type{class} */constclz=true;/** @type{int} */constx3=true;/** @type{float} */constx4=true;/** @type{Number} */constx5=true;/** @type{integer} */constx6=true;Code /** @type{Boolean} */declareconstbool: Boolean;// expect boolean/** @type{bool} */declareconstbool2: bool;// expect boolean/** @type{Object} */declareconstobj: Object;// expect object/** @type{String} */declareconststr: String;// expect string/** @type{Function} */declareconstf: Function;// expect (...args: any[]) => any/** @type{function} */declareconstf2: function;// ! Invalid output ! expect (...args: any[]) => any/** @type{class} */declareconstclz: any;// expect{new (...args: any[]): any }/** @type{int} */declareconstx3: any;// expect number/** @type{float} */declareconstx4: any;// expect number/** @type{Number} */declareconstx5: Number;// expect number/** @type{integer} */declareconstx6: any;// expect numberI 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 commented May 7, 2020
I... Don't think we recognize |
weswigham commented May 8, 2020
@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 |
| /** @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:{ |
There was a problem hiding this comment.
What about add a Object => object?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| */ | ||
| 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 |
There was a problem hiding this comment.
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.
| /** @param{Image} image */ | ||
| function process(image){ | ||
| >process : (image: Image) => HTMLImageElement | ||
| >process : (image: new (width?: number, height?: number) => HTMLImageElement) => HTMLImageElement |
There was a problem hiding this comment.
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 commented May 8, 2020
@typescript-bot cherry-pick this to release-3.9 |
Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into |
typescript-bot commented May 8, 2020
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. |
🤖 Pick PR #38368 (Fix js missing type arguments on ex...) into release-3.9
* 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
7 participants
Fixes#38356