JSDoc typedef module value type propagation fix #28256
Conversation
This fixes microsoft#26626 and some related implicit any issues.
|
@sandersn any update on this? |
There was a problem hiding this comment.
I think this is likely to break Typescript's ability to import nameless typedefs from Javascript because it treats @typedef as an alias for @type for them. Can you please check that with tests? I suggested what I think is the right approach below.
Sorry I ignored this PR for so long. I understand if you don't have the time or interest to pick it up again.
src/compiler/checker.ts
Outdated
| @@ -4796,6 +4796,10 @@ namespace ts { | |||
| return addOptionality(declaredType, isOptional); | |||
| } | |||
|
|
|||
| if (isNamelessJSDocTypeDef(declaration)) { | |||
| return getDeclaredTypeOfTypeAlias(getSymbolOfNode(declaration)); | |||
There was a problem hiding this comment.
so the type of the variable declaration is the type of the typedef? that seems like it basically treats @typedef as if it were @type and then relies on Javascript's ability to resolve values as types (in resolveEntityName) to then use the variable as a type.
Doesn't that mean this will cause importing the typedef from Typescript to now fail? Can you add a test with a TS file that imports a nameless typedef?
The right way is probably to change resolveEntityName to return the symbol for the variable even when resolving a type (like the current values-as-types code), then make sure that symbol is found correctly through getDeclaredTypeOfSymbol. The symbol might be fine as-is, or might need to be marked with SymbolFlags.TypeAlias.
There was a problem hiding this comment.
I wanted to make sure nameless typedef's didn't throw an implicit any error and assigning the underlying type seemed acceptable at the time.
Importing the typedef takes a slightly different path when resolving the symbol, where it was incorrectly resolving the the value symbol in combineValueAndTypeSymbols. I'll push a fix to that and updated tests shortly.
|
@sandersn I'm still interested in working on this PR. Just some context, my goal is to support closure compiler style I'll try to address the PR comments this week, thanks! |
Also narrow the definition of a `nameless` typedef by not allowing an initializer on the declaration.
|
@sandersn Could you take a look at the latest changes, they should address your comments. The only thing I'm not sure how to best resolve is the concern with resolving the variable decl type as the type of the typedef. Without the resolution, there'd be implicit any errors where there shouldn't be. We could just silence the diagnostic for nameless typedefs, but that didn't seem preferable. |
|
@sandersn just wanted to check if there's still interest on your side for this. |
|
@jameskeane Sorry I ignored this for so long. It looks like the original bug is fixed in 3.8. I'm going to close this, but let me know if any more fixes are needed to 3.8. |
This PR is a lower impact follow up from #28162, focused on fixing #26626.
The underlying issue is caused by the automatic binding of jsdoc typedef as a TypeAlias module member (see
delayedBindJSDocTypedefTagin binder) to the JSDoc node but the type being looked up from the variable decl node.Fixes #26626.
@sandersn