imported symbol that is shadowed with a local symbol does not error

[bradzacher]

🔎 Search Terms

imported symbol shadowed

🕗 Version & Regression Information

• This is the behavior in every version I tried

⏯ Playground Link

https://www.typescriptlang.org/play?isolatedModules=true#code/JYWwDg9gTgLgBAbzgYQuCA7Aph+BfOAMyjTgHIosBDAYxjIG44B6ZuAFQGU4soSoAznExwYAC2BCqQ4PSlwAblQA2AVywAoZVnip02XAC4UaSAfgAfOBlXLlcALzXbyhho2hIsRHABiyOAJiUgpqOkYWNi44FWUIAHchcUkYmTkY0QBPME1teH9jfzgrGztHZzs3IA

💻 Code

import { Component } from 'react';
let Component: Component | null = null;

import { FC } from 'react';
let FC: FC | null = null;

🙁 Actual behavior

The first case correctly errors:

Import declaration conflicts with local declaration of 'Component'. (2440)

The second case is allowed, even under isolatedModules.

🙂 Expected behavior

Both cases should error

Additional information about the issue

Looking at this from two perspectives - isolatedModules: true and false

For isolatedModules: false - this looks like it's working as expected. In a fully type-aware build TS can tell that the local value symbol shadows an imported type symbol - which is valid - and it can handle it just fine and knows it 100% can elide the import.

However for isolatedModules: true - I'd expect that TS acts stricter here. Without type information it's not really safe to assume that the imported name is a type.

From TS's POV I believe that when it transpiles the code it sees the imported symbol is never used as a value (due to the local symbol), so it elides the import.
This accidentally makes the code runtime-valid so it won't crash.

I'm of the opinion here that TS should always error on this case or else it's possible to write some really cooked looking code like

import { FC } from 'react';
let FC: FC | null = null;
   FC = null;
// ^^ assigning to an import... but also not technically

[andrewbranch]

I guess if the transpiler assumes there were no TS errors, there’s no real conflict here. If a local declaration has the same name as an import, the transpiler can assume the import had to be a type, and any value references to that name won’t count as a value usage of the import.

I agree this is sketchy though, and I’m open to adding a rule in isolateModules.

[andrewbranch]

If swc can’t easily work around this, that raises the importance.

[bradzacher]

FWIW eslint rules also struggle with this.

I didn't know this case was possible when I wrote the scope analyser - so this leads to some bad cases that slip through.

For example

import {Foo} from 'foo';
// import is never used - should be reported by no-unused-vars, but isn't

function Foo() {}
Foo();

These are both cases I found in the codebase at work because swc errored on them (I've just started playing around with swc instead of TS in our builds).

There's some complicated things with imports because they're always technically valid in either type or value locations - so for external tools it's hard to reason about this case of local declaration shadowing.

It does feel a bit weird that TS just makes the assumption that the code is correct and silently elides the import.
I guess this is the sort of thing that verbatimModuleSyntax does report on - though most people don't use (or want) that flag due to its other side-effects.

[andrewbranch]

It does feel a bit weird that TS just makes the assumption that the code is correct and silently elides the import.

Not totally sure what you mean by this. A lot of people seem to be under the impression that isolatedModules makes TS not use whole-program info during emit, but that’s not the case. All the flag is supposed to do is error on cases that would be problematic without whole-program info. Those cases are found by reasoning about stuff during checking, not actually blindfolding ourselves during emit and seeing what we bump into. So, TS elides the import here due to type-directed emit even in isolatedModules. I’m just speculating that we, or anyone else, could accomplish the same thing without whole-program info if we squint enough. Not sure if that’s what you meant.