When aliases and local declarations merge, basically nothing works

[andrewbranch]

Investigating #44896, I have found it is just the tip of a giant iceberg of bugs. The most fundamental operations of the compiler are derailed by a fairly simple merging pattern, which approximately nobody must be using, since as far as I know, #44896 was the first to notice any symptom of the problem. I felt like I should document what I’ve found so far, because it’s so big and widespread I feel sure that any fix I submit will miss some cases, and I want to check my interpretation of the problem against others’ before getting much deeper. Let’s explore the root cause with a simplified version of the example given in the bug:

// @Filename: value.ts
export const X = 0;

// @Filename: mergeType.ts
import { X } from "./value";
type X = number;
export { X };

// @Filename: index.ts
import { X } from "./mergeType";
const x: X = X;
//           ^ 'X' only refers to a type, but is being used as a value here.ts (2693)

So, value.ts exports a value X; mergeType.ts imports that value and merges a local type with it, then exports the result of that merge; finally, the merged symbol is imported by index.ts, who tries to access both its type meaning and its value meaning. But, only the type meaning is found.

The code that produces this error does something like

const symbol = resolveSymbol(resolveName(errorLocation, name, SymbolFlags.Type & ~SymbolFlags.Value));
if (!(symbol.flags & SymbolFlags.Value)) {
  // report error
}

1. The resolveName call does file-local resolution of the name X, returning a result as long as the non-file-local resolution would resolve to a symbol that is a type or interface (potentially merged with other stuff too). That call succeeds in finding X in import { X } from "./mergeType", because it peeks out of the file to find the type X = number meaning that satisfies the flags.
2. resolveSymbol resolves the imported symbol to what it points to. Usually, this is recursive and skips over re-exports all the way to where the symbol is originally exported. However, in the case of X, the real symbols being exported are in two different places: a value in value.ts and a type in mergeType.ts. resolveSymbol can’t just skip over the re-export in mergeType.ts because that would miss the type meaning altogether. So it returns the merged symbol in mergeType.ts, which has symbol flags TypeAlias (the local type meaning) and Alias (signifying an imported symbol that can be resolved further).
3. The resulting symbol from (2) doesn’t have SymbolFlags.Value, so the error is issued.

So what exactly went wrong? It seems that the code assumed resolveSymbol would never return something that’s still an alias; that is, a symbol that’s not fully resolved. This code would have to recursively call resolveAlias (close relative of resolveSymbol) and aggregate all the flags it sees until it resolves to something that’s no longer an alias.

Ok, no problem; that’s what I tried first. I ran the test and the error went away. Success! Except, then I looked at the JS emit. index.ts was completely missing an import/require for mergeType.ts. The code would crash at runtime, and now it doesn’t have any errors—a decidedly worse state than before. It turns out that the code controlling import elision in emit is full of this same pattern based on the same faulty assumption.

I updated about 10 more places to do recursively-resolving flag checks and fixed emit. However, while I was there, I noticed a few other checks that also fall prey to the same bad assumption. Imports are also elided if the symbol resolves to a type-only import or export, or if the symbol resolves to a const enum or const-enum-only module, since references to those will be inlined, making the import unreferenced. These things also fail to consider aliases merging with locals:

// @Filename: a.ts
interface A {}
export type { A }

// @Filename: b.ts
import { A } from "./a";
const A = 0;
export { A };

// @Filename: c.ts
import { A } from "./b";
A;

In c.ts, we got an error for using A in an emitting position because it was traced to a type-only export in a.ts, and the import was elided for the same reason. In fact, even the export in b.ts was elided for that reason. But this is all nonsensical, because the type-only export never actually touched the value meaning declared in b.ts. The type-only export, applied to a symbol that is already just a type, should be completely irrelevant.

So I fixed the handling of type-only imports and exports in error reporting and import/export elision next. But then I remembered that some value-having symbols can merge with other value-having symbols—functions and namespaces, for example—and I wondered how a merge between those would work when one of them is exported as type-only. As it turned out, thinking about the type-only behavior for this case may have been jumping the gun, because the normal case can be trivially broken too. I started with a test case like:

// @Filename: a.ts
function A() {}
export { A };

// @Filename: b.ts
import { A } from "./a";
namespace A {
  export const displayName = "A";
}

A();
A.displayName;

Gracefully, this version of the test does put an error on import { A } from "./a": Import declaration conflicts with local declaration of 'A'.. So while a function and namespace can merge locally, they can’t across module boundaries, making things slightly easier to reason about. Except, by now I have learned a fun and nearly foolproof way to merge almost anything I want by foiling resolveAlias. If I just introduce an intervening type merge between a.ts and b.ts...

// @Filename: a.ts
function A() {}
export { A };

// @Filename: b.ts
import { A } from "./a";
type A = 0;
export { A };

// @Filename: c.ts
import { A } from "./b";
namespace A {
  export const displayName = "A";
}

A();
A.displayName;

Yep! Now there are no symbol merging errors; just an error on A() that tells me A is not callable! Which at least is consistent with the related emit bugs, because b.js has no export and c.js has no import, so the function meaning is indeed lost in the emit.

Conclusion...?

There are 32 references to resolveSymbol and 25 references to resolveAlias that probably all need to be audited for this mistake. I also haven’t touched the const-enum-related import elision logic yet, which I think is similarly broken.

I’m a bit concerned about potential fallout from attempting a fix because error reporting and import elision are separate code paths, currently both wrong in a consistent, predictable way, and “fixing” error reporting without fixing emit makes the situation much, much worse than it is now. It’s also a bit hard to think about how to prioritize these bugs since they are very old and have received very little attention, but are pretty fundamental and glaringly wrong. I think the best course of action is to try to be thorough and get stuff in early in the release cycle. I wish we could be a bit more restrictive about alias merges, but that would be too big of a breaking change. I’m also considering whether the import elision logic can be simplified in many places by assuming that if a reference to an import appears in an emitting value position, we should consider the alias referenced, whether we think it resolves to a value or not (related to #48588 and similar things @gabritto is working on).

[fatcerberus]

Investigating #44896, I have found it is just the tip of a giant iceberg of bugs.

There are 32 references to resolveSymbol and 25 references to resolveAlias that probably all need to be audited for this mistake. I also haven’t touched the const-enum-related import elision logic yet, which I think is similarly broken.

So... would you say that it would take several days just to scratch the surface of the tip of that iceberg?
/strongbad🥊

[callionica]

This pattern may not be common because of this section of the docs:

https://www.typescriptlang.org/docs/handbook/declaration-merging.html#module-augmentation

which seems to encourage merging within declare.

There are also numerous examples on the internet of the comment that "types cannot be merged", "declaration merging does not work with type".

In addition, it's really not clear what the user intent would be from the code. Why would users expect a merging behaviour instead of a more-local-scope behaviour? Code review would surely discourage further examples.

[andrewbranch]

I agree, the code is weird and there aren’t a ton of good uses for it. The example in #44896 of building an enum-like symbol up from its constituent parts is one of the only patterns I’ve seen where merging symbols across declaration spaces makes sense, but I’ve usually seen all the meanings declared in the same module.

[andrewbranch]

Here’s another fun one from auditing every use of resolveAlias. In an old-fashioned internal module, you’re supposed to be able to import namespaces and values as the name any, but not types, since that would conflict with the built-in type when you go to reference it:

module a {
  export type A = number;
}

module c {
  import any = a.A;
//       ^^^ Import cannot be 'any'.
}

But the merging trick silences the error:

module a {
  export type A = number;
}

module b {
  export import A = a.A;
  export module A {}
}

module c {
  import any = b.A; // Ok! (But never able to be referenced as a type)
}

[johnnyreilly]

Link back to @andrewbranch's rant poster (purely for the entertainment)

https://twitter.com/atcb/status/1563260205583872000

[mikearnaldi]

Small aside regarding 'approximately nobody must be using', both fp-ts and effect-ts communities have attempted using it to have an importable 'namespace' with a type called in the same way

[andrewbranch]

@mikearnaldi were those attempts abandoned due to this issue?

[mikearnaldi]

@andrewbranch while it's quite hard to scope 'this issue' to a single thing I would say yes, we did not found a good way to do it. Basically those libs export huge modules as independent files and they become hard to use due to extensive amount of imports such as:

import * as Either from "fp-ts/Either"

And the community wanted a way of doing

import { Either } from "fp-ts"

But if we do that via re-exports, at the moment, there is no way to also say that such Either should not only refer to the re-exported module but also to the Either type that is inside and we force users to write Either.Either which is kind of bad to look at (though it looks the same in case of the namespace imports), at the moment we are all pretty much fractured choosing what we prefer I personally opted for

import * as E from "fp-ts/Either"

For a while before developing TS+ where I can use types as namespaces (not too much relevant to this issue)

[Conaclos]

Small aside regarding 'approximately nobody must be using', both fp-ts and effect-ts communities have attempted using it to have an importable 'namespace' with a type called in the same way

I meet the same issue when I investigated fp programming in TypeScript. This is why I opened #44896.

Link back to @andrewbranch's rant poster (purely for the entertainment)

I pretty like the idea of separating types and values and so to allow the use of name for both a value and a type.
This makes fp programming more ergonomics.

For instance, a pattern I like is to have a type and its constructor with the same name:

// @Filename: a.ts
export interface User { ... }
export function User(...) { ... }

// @Filename: b.ts
import { User } from "a.ts"
const u: User = User(...)

The example of #44896 allows to obtain a tree-shakable enum.