Fix missing error for accessing type-only export * member through namespace
#57176
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
| import * as intermediate from './intermediate' | ||
| const ghost: intermediate.Ghost = new intermediate.Ghost() | ||
| ~~~~~ | ||
| !!! error TS2339: Property 'Ghost' does not exist on type 'typeof import("intermediate")'. |
There was a problem hiding this comment.
It would be nice to be able to give a better type-only-specific error message here, but this is consistent with existing error messages you can get with other ways of doing property access on a module with type-only exports. (see exportNamespace3.ts, for example.)
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
This code is likely right, but I realised I'm missing background knowledge on how type-only imports work in the first place, so I asked a couple of questions.
| if (type.flags & TypeFlags.Object) { | ||
| const resolved = resolveStructuredTypeMembers(type as ObjectType); | ||
| const symbol = resolved.members.get(name); | ||
| if (symbol && !includeTypeOnlyMembers && type.symbol?.flags & SymbolFlags.ValueModule && getSymbolLinks(type.symbol).typeOnlyExportStarMap?.has(name)) { |
There was a problem hiding this comment.
I don't understand the check for !includeTypeOnlyMembers. If this is from a import * as intermediate from './x.js', the type doesn't indicate that type-only members should be skipped. Where does this come from?
Actually, the overall intent is to skip values that show up in the type-only exports map, right? How does this code distinguish a value (which shouldn't be included) from a type (which should (I think?))? Maybe that waits until the check just below the new one.
There was a problem hiding this comment.
This function gets properties which need to be values by definition. The question is whether the value has to exist in the emitted JavaScript or not. The only callers that pass includeTypeOnlyMembers: true are type queries and import type nodes:
type T = typeof x.y.z; // 'y' may be a namespace made by `export type * from "y"`
type U = typeof import("x").y.z;These are constructs that query values, but don’t rely on those values actually existing, so they can safely return value symbols that are only accessible via type-only imports/exports.
How does this code distinguish a value (which shouldn't be included) from a type (which should (I think?))? Maybe that waits until the check just below the new one.
Your parentheticals are incorrect, but the top level sentence is correct—that’s handled by symbolIsValue below.
There was a problem hiding this comment.
Remember, “type-only” is not the same as “is a type” or “is not a value.” A type-only import/export is like a like a lens that can be applied to any symbol. Applying it doesn’t change the kind/meaning of the symbol; it just means that path to the symbol doesn’t exist in JS, so you have to be careful with how you reference it in any code that emits to JS.
There was a problem hiding this comment.
Thanks, that example helped a lot. So the namespace 'y' can be, almost, a synthetic property that should only be visible from type space?
There was a problem hiding this comment.
I didn’t show where x/y/z were declared, but the way I intended the example was that y itself is real (it’s the module namespace object of a file that does export type * but maybe has no real exports—so it would be an empty object), while z is the thing that acts like a “synthetic property” on y. But I think you understood the idea.
|
@typescript-bot perf test |
|
Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at 281b668. You can monitor the build here. Update: The results are in! |
|
Heya @andrewbranch, I've started to run the regular perf test suite on this PR at 281b668. You can monitor the build here. Update: The results are in! |
|
Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at 281b668. You can monitor the build here. Update: The results are in! |
|
@andrewbranch Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @andrewbranch, the results of running the DT tests are ready. |
|
@andrewbranch Here are the results of running the top-repos suite comparing Everything looks good! |
3 participants
Fixes #56648