Skip to content

stricter jsdoc type resolution causes weird behaviour for value-only declarations #33106

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.

Sign up for GitHub

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

Jump to bottom
Closed
sandersn opened this issue Aug 27, 2019 · 4 comments · Fixed by #34515
Closed

stricter jsdoc type resolution causes weird behaviour for value-only declarations #33106

sandersn opened this issue Aug 27, 2019 · 4 comments · Fixed by #34515
Assignees
@sandersn
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Milestone
TypeScript 3.7.1

Comments

@sandersn
Copy link
Member 

/** @param {Image} image */
function process(image) {
}

Expected behavior:

  1. Error "'Image' refers to a value, but is being used as a type here." and image: any, as TS does.
  2. No error, and image: typeof Image, as we often do in JS.

I think I prefer (2), but after the stricter-jsdoc-type-resolution PR, I expected (1), which is also OK.

Actual behavior:
No error and image: any.
@sandersn sandersn added this to the TypeScript 3.7.0 milestone Aug 27, 2019
@weswigham
Copy link
Member

We still return the global Image symbol in getTypeFromTypeReference, but it's in getTypeReferenceType that we end up returning the errorType. Specifically getTypeFromJSAlias's recursion back into getTypeReferenceType seems to result in an errorType? Unintended sideeffect of the simplifications applied there with the class changes?

@sandersn
Copy link
Member Author

Yep, probably. I thought I got away with a little too much deletion and simplification.

@sandersn sandersn assigned sandersn and unassigned weswigham Aug 27, 2019
@sandersn sandersn mentioned this issue Aug 27, 2019
Merged
@sandersn sandersn added the Bug A bug in TypeScript label Aug 29, 2019
@ahocevar ahocevar mentioned this issue Sep 24, 2019
Closed
@ahocevar
Copy link

@sandersn Might #33575 be related?

@sandersn sandersn mentioned this issue Oct 16, 2019
Merged
@sandersn sandersn added the Fix Available A PR has been opened for this issue label Oct 16, 2019
@sandersn
Copy link
Member Author

sandersn commented Oct 16, 2019 •
edited
Loading

@ahocevar Yes, although the scenario still doesn't work completely, so it still needs work.

Edit: After more investigation, they're not really related, but after my fix #33575 will fail in a different way.

sandersn added a commit that referenced this issue Oct 24, 2019
@sandersn
Restore delayed merge check to getTypeFromJSDocValueReference
056a656 
This is needed when a function merges with a prototype assignment. The
resulting *merged* symbol is a constructor function marked with
SymbolFlags.Class. However, the merge doesn't happen until
getTypeOfFuncClassEnumModule is called, which, in the
getTypeReferenceType code path, doesn't happen until
getTypeFromJSDocValueReference. That means the check for
SymbolFlags.Class is missed.

Previously, getTypeFromJSDocValueReference had a weird check
`symbol !== getTypeOfSymbol(symbol).symbol`, which, if true, ran
getTypeReferenceType again on `getTypeOfSymbol(symbol).symbol`. For
JS "aliases", this had the effect of dereferencing the alias, and for
function-prototype merges, this had the effect of ... just trying again
after the merge had happened.

This is a confusing way to run things. getTypeReferenceType should
instead detect a function-prototype merge, cause it to happen, and
*then* run the rest of its code instead of relying on try-again logic at
the very end. However, for the RC, I want to fix this code by restoring
the old check, with an additional check to make sure that #33106 doesn't
break again:

```ts
const valueType = getTypeOfSymbol(symbol)
symbol !== valueType.symbol && getMergedSymbol(symbol) === valueType.symbol
```

I'll work on the real fix afterwards and put it into 3.8.
@sandersn sandersn mentioned this issue Oct 24, 2019
Merged
sandersn added a commit that referenced this issue Oct 24, 2019
@sandersn
Restore delayed merge check to getTypeFromJSDocValueReference (#34706)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
969634b 
* Restore delayed merge check to getTypeFromJSDocValueReference

This is needed when a function merges with a prototype assignment. The
resulting *merged* symbol is a constructor function marked with
SymbolFlags.Class. However, the merge doesn't happen until
getTypeOfFuncClassEnumModule is called, which, in the
getTypeReferenceType code path, doesn't happen until
getTypeFromJSDocValueReference. That means the check for
SymbolFlags.Class is missed.

Previously, getTypeFromJSDocValueReference had a weird check
`symbol !== getTypeOfSymbol(symbol).symbol`, which, if true, ran
getTypeReferenceType again on `getTypeOfSymbol(symbol).symbol`. For
JS "aliases", this had the effect of dereferencing the alias, and for
function-prototype merges, this had the effect of ... just trying again
after the merge had happened.

This is a confusing way to run things. getTypeReferenceType should
instead detect a function-prototype merge, cause it to happen, and
*then* run the rest of its code instead of relying on try-again logic at
the very end. However, for the RC, I want to fix this code by restoring
the old check, with an additional check to make sure that #33106 doesn't
break again:

```ts
const valueType = getTypeOfSymbol(symbol)
symbol !== valueType.symbol && getMergedSymbol(symbol) === valueType.symbol
```

I'll work on the real fix afterwards and put it into 3.8.

* Add bug number
typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this issue Oct 24, 2019
@sandersn @typescript-bot
Cherry-pick PR microsoft#34706 into release-3.7
e8e6282 
Component commits:
056a656 Restore delayed merge check to getTypeFromJSDocValueReference
This is needed when a function merges with a prototype assignment. The
resulting *merged* symbol is a constructor function marked with
SymbolFlags.Class. However, the merge doesn't happen until
getTypeOfFuncClassEnumModule is called, which, in the
getTypeReferenceType code path, doesn't happen until
getTypeFromJSDocValueReference. That means the check for
SymbolFlags.Class is missed.

Previously, getTypeFromJSDocValueReference had a weird check
`symbol !== getTypeOfSymbol(symbol).symbol`, which, if true, ran
getTypeReferenceType again on `getTypeOfSymbol(symbol).symbol`. For
JS "aliases", this had the effect of dereferencing the alias, and for
function-prototype merges, this had the effect of ... just trying again
after the merge had happened.

This is a confusing way to run things. getTypeReferenceType should
instead detect a function-prototype merge, cause it to happen, and
*then* run the rest of its code instead of relying on try-again logic at
the very end. However, for the RC, I want to fix this code by restoring
the old check, with an additional check to make sure that microsoft#33106 doesn't
break again:

```ts
const valueType = getTypeOfSymbol(symbol)
symbol !== valueType.symbol && getMergedSymbol(symbol) === valueType.symbol
```

I'll work on the real fix afterwards and put it into 3.8.

d1515f4 Add bug number
sandersn pushed a commit that referenced this issue Oct 24, 2019
@sandersn
Cherry-pick PR #34706 into release-3.7 (#34708)
23d1b7b 
Component commits:
056a656 Restore delayed merge check to getTypeFromJSDocValueReference
This is needed when a function merges with a prototype assignment. The
resulting *merged* symbol is a constructor function marked with
SymbolFlags.Class. However, the merge doesn't happen until
getTypeOfFuncClassEnumModule is called, which, in the
getTypeReferenceType code path, doesn't happen until
getTypeFromJSDocValueReference. That means the check for
SymbolFlags.Class is missed.

Previously, getTypeFromJSDocValueReference had a weird check
`symbol !== getTypeOfSymbol(symbol).symbol`, which, if true, ran
getTypeReferenceType again on `getTypeOfSymbol(symbol).symbol`. For
JS "aliases", this had the effect of dereferencing the alias, and for
function-prototype merges, this had the effect of ... just trying again
after the merge had happened.

This is a confusing way to run things. getTypeReferenceType should
instead detect a function-prototype merge, cause it to happen, and
*then* run the rest of its code instead of relying on try-again logic at
the very end. However, for the RC, I want to fix this code by restoring
the old check, with an additional check to make sure that #33106 doesn't
break again:

```ts
const valueType = getTypeOfSymbol(symbol)
symbol !== valueType.symbol && getMergedSymbol(symbol) === valueType.symbol
```

I'll work on the real fix afterwards and put it into 3.8.

d1515f4 Add bug number
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sandersn sandersn

Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
Milestone
TypeScript 3.7.1
Development

Successfully merging a pull request may close this issue.

Resolve more jsdoc value references as types microsoft/TypeScript
4 participants
@ahocevar @sandersn @weswigham @RyanCavanaugh