Skip to content

Fix assertion on duplicate imported identifiers #1124

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 1 commit into from
Feb 27, 2020
Merged

Fix assertion on duplicate imported identifiers #1124

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 22 additions & 21 deletions src/program.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -787,6 +787,7 @@ export class Program extends DiagnosticEmitter {
// queued imports should be resolvable now through traversing exports and queued exports
for (let i = 0, k = queuedImports.length; i < k; ++i) {
let queuedImport = queuedImports[i];
let localIdentifier = queuedImport.localIdentifier;
let foreignIdentifier = queuedImport.foreignIdentifier;
if (foreignIdentifier) { // i.e. import { foo [as bar] } from "./baz"
let element = this.lookupForeign(
Expand All @@ -797,9 +798,9 @@ export class Program extends DiagnosticEmitter {
);
if (element) {
queuedImport.localFile.add(
queuedImport.localIdentifier.text,
localIdentifier.text,
element,
true // isImport
localIdentifier // isImport
);
} else {
// FIXME: file not found is not reported if this happens?
Expand All @@ -812,14 +813,15 @@ export class Program extends DiagnosticEmitter {
let foreignFile = this.lookupForeignFile(queuedImport.foreignPath, queuedImport.foreignPathAlt);
if (foreignFile) {
let localFile = queuedImport.localFile;
let localName = queuedImport.localIdentifier.text;
let localName = localIdentifier.text;
localFile.add(
localName,
foreignFile.asImportedNamespace(
localName,
localFile
localFile,
localIdentifier
),
true // isImport
localIdentifier // isImport
);
} else {
assert(false); // already reported by the parser not finding the file
Expand Down Expand Up @@ -1775,7 +1777,7 @@ export class Program extends DiagnosticEmitter {
// resolve right away if the element exists
var element = this.lookupForeign(declaration.foreignName.text, foreignPath, foreignPathAlt, queuedExports);
if (element) {
parent.add(declaration.name.text, element, true);
parent.add(declaration.name.text, element, declaration.name /* isImport */);
return;
}

Expand Down Expand Up @@ -2161,7 +2163,7 @@ export abstract class Element {
abstract lookup(name: string): Element | null;

/** Adds an element as a member of this one. Reports and returns `false` if a duplicate. */
add(name: string, element: DeclaredElement): bool {
add(name: string, element: DeclaredElement, localIdentifierIfImport: IdentifierExpression | null = null): bool {
var originalDeclaration = element.declaration;
var members = this.members;
if (!members) this.members = members = new Map();
Expand All @@ -2174,17 +2176,18 @@ export abstract class Element {
if (merged) {
element = merged; // use merged element
} else {
let reportedIdentifier = localIdentifierIfImport || element.identifierNode;
if (isDeclaredElement(existing.kind)) {
this.program.errorRelated(
DiagnosticCode.Duplicate_identifier_0,
element.identifierNode.range,
(<DeclaredElement>existing).declaration.name.range,
element.identifierNode.text
reportedIdentifier.range,
(<DeclaredElement>existing).identifierNode.range,
reportedIdentifier.text
);
} else {
this.program.error(
DiagnosticCode.Duplicate_identifier_0,
element.identifierNode.range, element.identifierNode.text
reportedIdentifier.range, reportedIdentifier.text
);
}
return false;
Expand Down Expand Up @@ -2338,13 +2341,13 @@ export class File extends Element {
}

/* @override */
add(name: string, element: DeclaredElement, isImport: bool = false): bool {
add(name: string, element: DeclaredElement, localIdentifierIfImport: IdentifierExpression | null = null): bool {
if (element.hasDecorator(DecoratorFlags.GLOBAL)) {
element = this.program.ensureGlobal(name, element); // possibly merged globally
}
if (!super.add(name, element)) return false;
if (!super.add(name, element, localIdentifierIfImport)) return false;
element = assert(this.lookupInSelf(name)); // possibly merged locally
if (element.is(CommonFlags.EXPORT) && !isImport) {
if (element.is(CommonFlags.EXPORT) && !localIdentifierIfImport) {
this.ensureExport(
element.name,
element
Expand Down Expand Up @@ -2404,12 +2407,10 @@ export class File extends Element {
}

/** Creates an imported namespace from this file. */
asImportedNamespace(name: string, parent: Element): Namespace {
var ns = new Namespace(
name,
parent,
this.program.makeNativeNamespaceDeclaration(name)
);
asImportedNamespace(name: string, parent: Element, localIdentifier: IdentifierExpression): Namespace {
var declaration = this.program.makeNativeNamespaceDeclaration(name);
declaration.name = localIdentifier;
var ns = new Namespace(name, parent, declaration);
var exports = this.exports;
if (exports) {
for (let [memberName, member] of exports) {
Expand Down Expand Up @@ -3719,7 +3720,7 @@ function tryMerge(older: Element, newer: Element): DeclaredElement | null {
// NOTE: some of the following cases are not supported by TS, not sure why exactly.
// suggesting to just merge what seems to be possible for now and revisit later.
assert(older.program === newer.program);
assert(!newer.members);
if (newer.members) return null;
var merged: DeclaredElement | null = null;
switch (older.kind) {
case ElementKind.FUNCTION_PROTOTYPE: {
Expand Down