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(js_semantic): import namespace handling #2812

Merged
merged 1 commit into from May 13, 2024

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented May 10, 2024

Summary

Fix #2796

This PR improves the handling of import namespace in the JavaScript/TypeScript semantic model.

We now handle import namespace as TypeScript namespace: we consider them as values (variables) and allow their reference in an ambient context only inside a qualified name.
Some examples:

// Unused import
import * as Ns1 from "";
// This doesn't reference the import namespace `Ns1`
type T1 = Ns1; // Undeclared variable `Ns1`

namespace Ns2 {}
type T2 = Ns2; // Undeclared variable `Ns2`

import type * as Ns3 from "";
// `Ns3` references the import namespace because it is a qualified name.
type T3 = Ns3.Inner;

Unfortunately it is not enough because import namespace can be imported as a type.
In this specific case, we consider them as types.
However, it is always invalid of referencing a namespace in an unqualified name.
I added an exception in the name resolution where we report the reference as unresolved if it is not a qualified name and if it is referencing an import namespace.
This allows handling this case:

// Unused import
import type * as Ns from "";
]/ This doesn't reference the import namespace `Ns`
type T = Ns; // Undeclared variable `Ns`

I also fixed an issue where exporting as a type a TypeScript namespace was not correctly resolved.

namespace Ns {}
export type { Ns }; // This was previously reported as an undeclared type. It now refrences the namespace.

Test Plan

I added non-regression tests.

Mote: the new tests for useImportType and useExportType are not non-regression tests. More tests is always good no :)

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages A-Changelog Area: changelog labels May 10, 2024
@Conaclos Conaclos force-pushed the conaclos/semantic-model/2796 branch from 85b0d4e to 5d9e7ea Compare May 10, 2024 19:10
@Conaclos Conaclos requested a review from a team May 10, 2024 19:11
@Conaclos Conaclos changed the title refactor(js_semantic): improve import namespace handling fix(js_semantic): import namespace handling May 10, 2024
Copy link
Contributor

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 49701 49701 0
Passed 48721 48721 0
Failed 980 980 0
Panics 0 0 0
Coverage 98.03% 98.03% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6322 6322 0
Passed 2036 2036 0
Failed 4286 4286 0
Panics 0 0 0
Coverage 32.20% 32.20% 0.00%

ts/babel

Test result main count This PR count Difference
Total 662 662 0
Passed 593 593 0
Failed 69 69 0
Panics 0 0 0
Coverage 89.58% 89.58% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17646 17646 0
Passed 13439 13439 0
Failed 4205 4205 0
Panics 2 2 0
Coverage 76.16% 76.16% 0.00%

@Conaclos Conaclos merged commit 63a71d3 into main May 13, 2024
15 checks passed
@Conaclos Conaclos deleted the conaclos/semantic-model/2796 branch May 13, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Linter Area: linter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

💅 noUnusedImports false negative with TypeScript ambient types
1 participant