Skip to content

Fix crash pulling on global types before they're initialized #46471

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

Merged
merged 4 commits into from
Oct 25, 2021
Merged

Fix crash pulling on global types before they're initialized #46471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aa0b014
Add failing test
andrewbranch Oct 21, 2021
0136603
Dumb fix
andrewbranch Oct 21, 2021
da6995d
Compute error message info more lazily
andrewbranch Oct 25, 2021
64961f0
One more laziness
andrewbranch Oct 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 29 additions & 10 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3275,21 +3275,40 @@ namespace ts {
const namespaceName = getFullyQualifiedName(namespace);
const declarationName = declarationNameToString(right);
const suggestionForNonexistentModule = getSuggestedSymbolForNonexistentModule(right, namespace);
const exportedTypeSymbol = getMergedSymbol(getSymbol(getExportsOfSymbol(namespace), right.escapedText, SymbolFlags.Type));
const containingQualifiedName = isQualifiedName(name) && getContainingQualifiedNameNode(name);
const canSuggestTypeof = containingQualifiedName && !isTypeOfExpression(containingQualifiedName.parent) && tryGetQualifiedNameAsValue(containingQualifiedName);
if (suggestionForNonexistentModule) {
Copy link
Member

@DanielRosenwasser DanielRosenwasser Oct 21, 2021

Choose a reason for hiding this comment

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

So the original change also added this. This means that while we're doing symbol merging, we're asking for the merged symbol. Is this also a problem?

Copy link
Member Author

@andrewbranch andrewbranch Oct 21, 2021

Choose a reason for hiding this comment

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

Nope.

error(right, Diagnostics._0_has_no_exported_member_named_1_Did_you_mean_2, namespaceName, declarationName, symbolToString(suggestionForNonexistentModule));
return undefined;
}
Copy link
Member

@DanielRosenwasser DanielRosenwasser Oct 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It's also weird that we never check for the appropriate SymbolFlags being passed in, right? There's no way you would've done this work if we strictly were looking for SymbolFlags.Type. I think this was overlooked in the follow-up code since there is a check for meaning & SymbolFlags.Namespace below.

Copy link
Member Author

@andrewbranch andrewbranch Oct 21, 2021

Choose a reason for hiding this comment

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

Yeah. In order to trigger the bug, the symbol has to not exist by the meaning of the original lookup, but exist in the value meaning, and it has to be part of a qualified name on the right-hand of an import equals, at least in the stack trace we're exclusively seeing in reports. I think the implicit assumption in this code was that if the symbol wasn't found in the lookup but exists by value, we must have been looking for a type, which of course is not true when we’re looking for namespaces exclusively.

There is definitely some logical cleanup that should ideally happen around that, but I’m a little concerned because it would fix this particular repro, but is pretty far removed from the root cause, so I’m not confident it would fix all instances of this crash. There could be some other entrypoint to it that doesn’t come from resolving an import = entity name.

else if (canSuggestTypeof) {
error(containingQualifiedName, Diagnostics._0_refers_to_a_value_but_is_being_used_as_a_type_here_Did_you_mean_typeof_0, entityNameToString(containingQualifiedName));
}
else if (meaning & SymbolFlags.Namespace && exportedTypeSymbol && isQualifiedName(name.parent)) {
error(name.parent.right, Diagnostics.Cannot_access_0_1_because_0_is_a_type_but_not_a_namespace_Did_you_mean_to_retrieve_the_type_of_the_property_1_in_0_with_0_1, symbolToString(exportedTypeSymbol), unescapeLeadingUnderscores(name.parent.right.escapedText));

const containingQualifiedName = isQualifiedName(name) && getContainingQualifiedNameNode(name);
const canSuggestTypeof = globalObjectType // <-- can't pull on types if global types aren't initialized yet
&& (meaning & SymbolFlags.Type)
&& containingQualifiedName
&& !isTypeOfExpression(containingQualifiedName.parent)
&& tryGetQualifiedNameAsValue(containingQualifiedName);
if (canSuggestTypeof) {
error(
containingQualifiedName,
Diagnostics._0_refers_to_a_value_but_is_being_used_as_a_type_here_Did_you_mean_typeof_0,
entityNameToString(containingQualifiedName)
);
return undefined;
}
else {
error(right, Diagnostics.Namespace_0_has_no_exported_member_1, namespaceName, declarationName);

if (meaning & SymbolFlags.Namespace && isQualifiedName(name.parent)) {
const exportedTypeSymbol = getMergedSymbol(getSymbol(getExportsOfSymbol(namespace), right.escapedText, SymbolFlags.Type));
if (exportedTypeSymbol) {
error(
name.parent.right,
Diagnostics.Cannot_access_0_1_because_0_is_a_type_but_not_a_namespace_Did_you_mean_to_retrieve_the_type_of_the_property_1_in_0_with_0_1,
symbolToString(exportedTypeSymbol),
unescapeLeadingUnderscores(name.parent.right.escapedText)
);
return undefined;
}
}

error(right, Diagnostics.Namespace_0_has_no_exported_member_1, namespaceName, declarationName);
}
return undefined;
}
Expand Down
15 changes: 15 additions & 0 deletions tests/baselines/reference/importEqualsError45874.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/globals.ts(5,22): error TS2694: Namespace 'globals' has no exported member 'toString'.


==== /globals.ts (1 errors) ====
namespace globals {
export type Foo = {};
export const Bar = {};
}
import Foo = globals.toString.Blah;
~~~~~~~~
!!! error TS2694: Namespace 'globals' has no exported member 'toString'.

==== /index.ts (0 errors) ====
const Foo = {};

21 changes: 21 additions & 0 deletions tests/baselines/reference/importEqualsError45874.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
//// [tests/cases/compiler/importEqualsError45874.ts] ////

//// [globals.ts]
namespace globals {
export type Foo = {};
export const Bar = {};
}
import Foo = globals.toString.Blah;

//// [index.ts]
const Foo = {};


//// [globals.js]
var globals;
(function (globals) {
globals.Bar = {};
})(globals || (globals = {}));
var Foo = globals.toString.Blah;
//// [index.js]
var Foo = {};
18 changes: 18 additions & 0 deletions tests/baselines/reference/importEqualsError45874.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
=== /globals.ts ===
namespace globals {
>globals : Symbol(globals, Decl(globals.ts, 0, 0))

export type Foo = {};
>Foo : Symbol(Foo, Decl(globals.ts, 0, 19))

export const Bar = {};
>Bar : Symbol(Bar, Decl(globals.ts, 2, 14))
}
import Foo = globals.toString.Blah;
>Foo : Symbol(Foo, Decl(globals.ts, 3, 1))
>globals : Symbol(globals, Decl(globals.ts, 0, 0))

=== /index.ts ===
const Foo = {};
>Foo : Symbol(Foo, Decl(index.ts, 0, 5))

22 changes: 22 additions & 0 deletions tests/baselines/reference/importEqualsError45874.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
=== /globals.ts ===
namespace globals {
>globals : typeof globals

export type Foo = {};
>Foo : Foo

export const Bar = {};
>Bar : {}
>{} : {}
}
import Foo = globals.toString.Blah;
>Foo : any
>globals : typeof globals
>toString : any
>Blah : any

=== /index.ts ===
const Foo = {};
>Foo : {}
>{} : {}

9 changes: 9 additions & 0 deletions tests/cases/compiler/importEqualsError45874.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// @Filename: /globals.ts
namespace globals {
export type Foo = {};
export const Bar = {};
}
import Foo = globals.toString.Blah;

// @Filename: /index.ts
const Foo = {};