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.

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

Fix up code so we don't crash when TS itself is emitted with let/const #50151

Merged
merged 2 commits into from
Aug 10, 2022
Merged

Fix up code so we don't crash when TS itself is emitted with let/const #50151

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
2e4f129
Fix up code so we don't crash when TS itself is emitted with let/const
jakebailey Aug 2, 2022
e53273d
Make switch case code equivalent
jakebailey Aug 4, 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
6 changes: 4 additions & 2 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27022,9 +27022,11 @@ namespace ts {
case AssignmentDeclarationKind.ExportsProperty:
case AssignmentDeclarationKind.Prototype:
case AssignmentDeclarationKind.PrototypeProperty:
let valueDeclaration = binaryExpression.left.symbol?.valueDeclaration;
// falls through
case AssignmentDeclarationKind.ModuleExports:
let valueDeclaration: Declaration | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Was this intended to be equivalent? It doesn't look equivalent (and it isn't obviously related to let/const).

Copy link
Member

Choose a reason for hiding this comment

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

+1 : This is not equivalent but clearly tests are not failing..

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks equivalent to me... Am I missing something?

Copy link
Member Author

@jakebailey jakebailey Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If it's any of the original three, it'll not be equal to the 4th one, so it picks the first branch, otherwise it's ModuleExports, so it picks the second branch. I tried to keep them in order, but maybe it'd be clearer to just not use an if, and copy the two lines to both blocks.

Copy link
Member Author

@jakebailey jakebailey Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I see. If it was one of the first few cases but the expression turned out to be undefined, it'll pick the second expression even if it's not ModuleExports. That is different!

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked, and in all of our test cases, if binaryExpression.left.symbol?.valueDeclaration is undefined, binaryExpression.symbol?.valueDeclaration is also undefined, hence why my PR doesn't fail even though the logic is different.

Copy link
Member

Choose a reason for hiding this comment

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

I’m sure I wouldn’t have written this as a falls-through and ||= if I could have combined the cases like you have here, but it’s disappointing the tests don’t capture that. It’s possible I was being overly defensive.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm OOF but had some time in the airport, changed it so it's semantically equivalent to be safe.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a little worried about someone adding a case and missing the exception, but I agree that it looks equivalent now.

Copy link
Member Author

Choose a reason for hiding this comment

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

The alternative is to just use var, if you all find that preferable.

if (kind !== AssignmentDeclarationKind.ModuleExports) {
valueDeclaration = binaryExpression.left.symbol?.valueDeclaration;
}
valueDeclaration ||= binaryExpression.symbol?.valueDeclaration;
const annotated = valueDeclaration && getEffectiveTypeAnnotationNode(valueDeclaration);
return annotated ? getTypeFromTypeNode(annotated) : undefined;
Expand Down
6 changes: 5 additions & 1 deletion src/services/services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1363,6 +1363,11 @@ namespace ts {
onUnRecoverableConfigFileDiagnostic: noop,
};

// The call to isProgramUptoDate below may refer back to documentRegistryBucketKey;
Copy link
Member

Choose a reason for hiding this comment

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

That's scary, but I don't see a better way around it. @sheetalkamat might.

Either way, thanks for the comment!

Copy link
Member

Choose a reason for hiding this comment

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

Seems like its needed only for config file's json and we currently dont use that since we implement getParsedCommandLine on project.. But yeah this change looks good

// calculate this early so it's not undefined if downleveled to a var (or, if emitted
// as a const variable without downleveling, doesn't crash).
const documentRegistryBucketKey = documentRegistry.getKeyForCompilationSettings(newSettings);

// If the program is already up-to-date, we can reuse it
if (isProgramUptoDate(program, rootFileNames, newSettings, (_path, fileName) => host.getScriptVersion(fileName), fileName => compilerHost!.fileExists(fileName), hasInvalidatedResolution, hasChangedAutomaticTypeDirectiveNames, getParsedCommandLine, projectReferences)) {
return;
Expand All @@ -1374,7 +1379,6 @@ namespace ts {
// the program points to old source files that have been invalidated because of
// incremental parsing.

const documentRegistryBucketKey = documentRegistry.getKeyForCompilationSettings(newSettings);
const options: CreateProgramOptions = {
rootNames: rootFileNames,
options: newSettings,
Expand Down