Skip to content

fix(39373): const {} = require import causes Debug Failure #39567

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

Merged
merged 1 commit into from
Jul 17, 2020

Conversation

a-tarasyuk
Copy link
Contributor

@a-tarasyuk a-tarasyuk commented Jul 11, 2020

Fixes #39373

@a-tarasyuk a-tarasyuk force-pushed the bug/39373 branch 4 times, most recently from 0ea45f3 to 4eeb747 Compare July 12, 2020 04:00
@DanielRosenwasser
Copy link
Member

Generally we use getNameOfDeclaration or something like that to avoid duplicating the diagnostic. It'll fill it in with something like (anonymous) or (default).

@a-tarasyuk
Copy link
Contributor Author

a-tarasyuk commented Jul 14, 2020

@DanielRosenwasser Thanks for the feedback 👍. Actually, getHeritageClauseVisibilityError uses it

typeName: getNameOfDeclaration(node.parent.parent as Declaration)

and it returns undefined for the case

export default class extends Foo {}

Based on the type definition, a name can be undefined, however, that case was not handled in getHeritageClauseVisibilityError.

/** May be undefined in `export default class { ... }`. */
readonly name?: Identifier;

For that reason, TS crashes because it uses diagnostic messages with two arguments, however, receive only one.

context.addDiagnostic(createDiagnosticForNode(symbolAccessibilityResult.errorNode || errorInfo.errorNode,
errorInfo.diagnosticMessage,
symbolAccessibilityResult.errorSymbolName,
symbolAccessibilityResult.errorModuleName));

Do you propose to replace the missed name by creating Identifier with the(anonymous) name instead of using a new diagnostic message? I think we can do it, just need to create a new Identifier and change the logic to getting text because a new Identifier will not be present in the source file.

context.addDiagnostic(createDiagnosticForNode(symbolAccessibilityResult.errorNode || errorInfo.errorNode,
errorInfo.diagnosticMessage,
getTextOfNode(errorInfo.typeName),

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Jul 16, 2020
@DanielRosenwasser
Copy link
Member

Actually I think it's fine, especially since functions have the same diagnostic.

@DanielRosenwasser DanielRosenwasser merged commit 4e24b1b into microsoft:master Jul 17, 2020
@DanielRosenwasser
Copy link
Member

I actually realized that the test might need to be changed to use at least one .js file because of #39617...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

const {} = require import causes Debug Failure
4 participants