Skip to content

mergeSymbol in checker:Remove block-scoped duplicate declaration errors in plain JS #47825

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 2 commits into from
Feb 10, 2022
Merged

mergeSymbol in checker:Remove block-scoped duplicate declaration errors in plain JS #47825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb13502
Checker:Remove block-scoped duplicate declaration errors in plain JS
sandersn Feb 9, 2022
8e59c50
fix semicolon lint
sandersn Feb 10, 2022
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
19 changes: 10 additions & 9 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1320,13 +1320,14 @@ namespace ts {
else { // error
const isEitherEnum = !!(target.flags & SymbolFlags.Enum || source.flags & SymbolFlags.Enum);
const isEitherBlockScoped = !!(target.flags & SymbolFlags.BlockScopedVariable || source.flags & SymbolFlags.BlockScopedVariable);
const message = isEitherEnum
? Diagnostics.Enum_declarations_can_only_merge_with_namespace_or_other_enum_declarations
: isEitherBlockScoped
? Diagnostics.Cannot_redeclare_block_scoped_variable_0
: Diagnostics.Duplicate_identifier_0;
const message = isEitherEnum ? Diagnostics.Enum_declarations_can_only_merge_with_namespace_or_other_enum_declarations
: isEitherBlockScoped ? Diagnostics.Cannot_redeclare_block_scoped_variable_0
: Diagnostics.Duplicate_identifier_0;
const sourceSymbolFile = source.declarations && getSourceFileOfNode(source.declarations[0]);
const targetSymbolFile = target.declarations && getSourceFileOfNode(target.declarations[0]);

const isSourcePlainJs = isPlainJsFile(sourceSymbolFile, compilerOptions.checkJs);
const isTargetPlainJs = isPlainJsFile(targetSymbolFile, compilerOptions.checkJs);
const symbolName = symbolToString(source);

// Collect top-level duplicate identifier errors into one mapping, so we can then merge their diagnostics if there are a bunch
Expand All @@ -1337,12 +1338,12 @@ namespace ts {
({ firstFile, secondFile, conflictingSymbols: new Map() } as DuplicateInfoForFiles));
const conflictingSymbolInfo = getOrUpdate(filesDuplicates.conflictingSymbols, symbolName, () =>
({ isBlockScoped: isEitherBlockScoped, firstFileLocations: [], secondFileLocations: [] } as DuplicateInfoForSymbol));
addDuplicateLocations(conflictingSymbolInfo.firstFileLocations, source);
addDuplicateLocations(conflictingSymbolInfo.secondFileLocations, target);
if (!isSourcePlainJs) addDuplicateLocations(conflictingSymbolInfo.firstFileLocations, source);
if (!isTargetPlainJs) addDuplicateLocations(conflictingSymbolInfo.secondFileLocations, target);
Comment on lines +1341 to +1342
Copy link
Member

Choose a reason for hiding this comment

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

Definitely weird - if you get one in one file, it's helpful to know where the other is. But you can always find-all-references I suppose.

Copy link
Member Author

@sandersn sandersn Feb 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm trying to imitate the existing post-compile filtration of JS errors, which behaves the same way. It's definitely not ideal.

Edit: Actually, this maintains the pre-4.6 behaviour if you have a global const in a TS file and a global const in a JS file. The latter will not report the error, the former will.

}
else {
addDuplicateDeclarationErrorsForSymbols(source, message, symbolName, target);
addDuplicateDeclarationErrorsForSymbols(target, message, symbolName, source);
if (!isSourcePlainJs) addDuplicateDeclarationErrorsForSymbols(source, message, symbolName, target);
if (!isTargetPlainJs) addDuplicateDeclarationErrorsForSymbols(target, message, symbolName, source);
}
}
return target;
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/program.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2102,7 +2102,7 @@ namespace ts {

const isJs = sourceFile.scriptKind === ScriptKind.JS || sourceFile.scriptKind === ScriptKind.JSX;
const isCheckJs = isJs && isCheckJsEnabledForFile(sourceFile, options);
const isPlainJs = isJs && !sourceFile.checkJsDirective && options.checkJs === undefined;
const isPlainJs = isPlainJsFile(sourceFile, options.checkJs);
const isTsNoCheck = !!sourceFile.checkJsDirective && sourceFile.checkJsDirective.enabled === false;

// By default, only type-check .ts, .tsx, Deferred, plain JS, checked JS and External
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,10 @@ namespace ts {
return getSourceFileOfNode(module.valueDeclaration || getNonAugmentationDeclaration(module));
}

export function isPlainJsFile(file: SourceFile | undefined, checkJs: boolean | undefined): boolean {
return !!file && (file.scriptKind === ScriptKind.JS || file.scriptKind === ScriptKind.JSX) && !file.checkJsDirective && checkJs === undefined;
}

export function isStatementWithLocals(node: Node) {
switch (node.kind) {
case SyntaxKind.Block:
Expand Down
6 changes: 1 addition & 5 deletions tests/baselines/reference/plainJSReservedStrict.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,12 @@
tests/cases/conformance/salsa/plainJSReservedStrict.js(2,7): error TS1100: Invalid use of 'eval' in strict mode.
tests/cases/conformance/salsa/plainJSReservedStrict.js(2,7): error TS2451: Cannot redeclare block-scoped variable 'eval'.
tests/cases/conformance/salsa/plainJSReservedStrict.js(3,7): error TS1100: Invalid use of 'arguments' in strict mode.


==== tests/cases/conformance/salsa/plainJSReservedStrict.js (3 errors) ====
==== tests/cases/conformance/salsa/plainJSReservedStrict.js (2 errors) ====
"use strict"
const eval = 1
~~~~
!!! error TS1100: Invalid use of 'eval' in strict mode.
~~~~
!!! error TS2451: Cannot redeclare block-scoped variable 'eval'.
!!! related TS6203 /.ts/lib.es5.d.ts:32:18: 'eval' was also declared here.
const arguments = 2
~~~~~~~~~
!!! error TS1100: Invalid use of 'arguments' in strict mode.
Expand Down