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(49702): LS crash in binder for module in a JS file #52537

Merged
merged 5 commits into from Mar 8, 2023
Merged

Conversation

navya9singh
Copy link
Member

Fixes #49702
Crash occurs due to unexpected module in a .js file. Adding a case in bindThisPropertyAssignment() to handle a ModuleDeclaration fixes the problem.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jan 31, 2023
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Couple of small requests:

  • add a comment
  • clean up test

src/compiler/binder.ts Show resolved Hide resolved
Comment on lines 10 to 14
//// [thisKeyword.js]
var foo;
(function (foo) {
this.bar = 4;
})(foo || (foo = {}));
Copy link
Member

Choose a reason for hiding this comment

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

This is part of the .js emit for the thisKeyword test and shouldn't be part of the test case.

// @checkJs: true
// @outDir: out/

//// [thisKeyword.ts]
Copy link
Member

Choose a reason for hiding this comment

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

remove this line, because it hides the fact that this test is in a .js file.

@@ -0,0 +1,14 @@
// @filename: a.js
Copy link
Member

Choose a reason for hiding this comment

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

Move the filename comment closer to the actual code. Then add another section with // @filename: b.js that has similar same code but uses a namespace declaration:

// @filename: b.js
namespace blah {
    this.prop = 42;
}

Also, rename the containing file to thisAssignmentInNamespaceDeclaration1.ts.

Naming the test after the original behavior ("crash") is not really helpful long-term. Just name them after what's happening in the test, make sure we don't crash, and the baseline will demonstrate the actual behavior.

@sandersn sandersn added this to Not started in PR Backlog Feb 14, 2023
@sandersn
Copy link
Member

sandersn commented Mar 7, 2023

@DanielRosenwasser do you want to take another look at this? Because I think it's ready to merge now.

@sandersn sandersn moved this from Not started to Needs merge in PR Backlog Mar 7, 2023
@navya9singh navya9singh merged commit 74a37f8 into main Mar 8, 2023
PR Backlog automation moved this from Needs merge to Done Mar 8, 2023
@navya9singh navya9singh deleted the fix(49702) branch March 8, 2023 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Fatal LS crash in binder for module in JS file
4 participants