Skip to content
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.

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

Fix type-only imports in interface 'extends' and import=/export= #36496

Merged
merged 15 commits into from
Jan 29, 2020

Conversation

andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Jan 29, 2020

  • Fixes a bug I discovered while working on this where errors for using a transitively type-only alias didn’t appear in editors depending on what files are open (because alias resolution happened in a different order than in tsc)—tested in the added unit test.
  • Fixes import type can't be used with interface extends #36478
  • Fixes Type-only import can be used as value after 'import =' #36466
    • Disallows import aliases referencing type-only names
    • Fixes ImportEqualsDeclarations and ExportAssignments referencing type-only names (adds appropriate error for usage of alias in JS-emitting positions)

@andrewbranch andrewbranch marked this pull request as ready for review January 29, 2020 17:47
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments. I'm still reading the checker.ts changes.

src/compiler/core.ts Show resolved Hide resolved
src/compiler/diagnosticMessages.json Show resolved Hide resolved
src/compiler/utilities.ts Outdated Show resolved Hide resolved
src/compiler/utilities.ts Outdated Show resolved Hide resolved
src/compiler/utilities.ts Outdated Show resolved Hide resolved
if (links.typeOnlyDeclaration === undefined) {
const typeOnly = find(resolvesToSymbol.declarations, isTypeOnlyImportOrExportDeclaration);
if (links.typeOnlyDeclaration === undefined || overwriteEmpty && links.typeOnlyDeclaration === false) {
resolvesToSymbol = resolvesToSymbol.exports?.get(InternalSymbolName.ExportEquals) ?? resolvesToSymbol;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would declare a new variable just because I hate mutation so much:

Suggested change
resolvesToSymbol = resolvesToSymbol.exports?.get(InternalSymbolName.ExportEquals) ?? resolvesToSymbol;
const resolved = resolvesToSymbol.exports?.get(InternalSymbolName.ExportEquals) ?? resolvesToSymbol;

Not sure about the name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other kinds of aliases we might have to manually follow the same way as import =? This seems like a slightly odd place to manually get ExportEquals out of the exports, although I could be wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other kinds of aliases we might have to manually follow the same way as import =?

The answer is almost definitely “no,” but I’ve been struggling to concisely explain why for too long at this point, which makes me realize that the reason I’m confident that I’m right is mostly because of thorough test coverage.

src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of questions and comments. I can't tell how multi-part chains are getting followed from reading the code.

markSymbolOfAliasDeclarationIfResolvesToTypeOnly(specifier, exportSymbol);
return resolveSymbol(exportSymbol, dontResolveAlias);
const resolved = resolveSymbol(exportSymbol, dontResolveAlias);
if (!markSymbolOfAliasDeclarationIfResolvesToTypeOnly(specifier, exportSymbol)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this call pair happens quite a few times. I think it should be extracted, perhaps into markSymbolOfAliasDeclarationIfResolvesToTypeOnly itself. I can't tell if it's possible and easy to regularize the other calls to accommodate the 1-export 2-resolved pattern.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought when reviewing my own PR this morning, but stopped when I couldn’t come up with a satisfying name to add more logic to a function that already has a monstrous name. Any suggestions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combined all three type-only checks (1. declaration is marked type-only; 2. immediate alias is marked type-only; 3. final alias is marked type-only) into one markSymbolOfAliasDeclarationIfTypeOnly call

src/compiler/checker.ts Show resolved Hide resolved
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export default <expr> behaves a lot like export = <expr> and probably also needs a test.

@andrewbranch
Copy link
Member Author

@weswigham like this one? The large majority of type-only tests already existed. There are also some new export default tests covered in the unit test.

@weswigham
Copy link
Member

I was thinking like this one but with export default in place of export = (and default imports in place of namespace imports)

@andrewbranch andrewbranch merged commit 2fac535 into microsoft:master Jan 29, 2020
@andrewbranch andrewbranch deleted the bug/type-only branch January 29, 2020 23:00
@bolinfest
Copy link

I just commented on #36478 as I don't think this fixes the issue when the type is another level deep because it appears I can do this:

import type * as monaco from "monaco-editor";

interface MyViewZone extends monaco.IRange {
  marginDomNode: HTMLElement;
}

but not this:

import type * as monaco from "monaco-editor";

interface MyViewZone extends monaco.editor.IViewZone {
  marginDomNode: HTMLElement;
}

@bolinfest
Copy link

Ah, though as a workaround I can do:

import type * as monaco from "monaco-editor";
type IViewZone = monaco.editor.IViewZone;

interface MyViewZone extends IViewZone {
  marginDomNode: HTMLElement;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

import type can't be used with interface extends Type-only import can be used as value after 'import ='
6 participants