Make direct assignments to cjs exports considered literal contexts #39816

weswigham · 2020-07-29T19:52:16Z

sandersn

I agree with this if we already issue an error for

module.exports.a = 1
module.exports.a = 2

Although I didn't think we did. It's a little weird that I don't see any new error baselines, so maybe we just don't have coverage.

Besides that, I have a couple of questions and a few formatting suggestions.

sandersn · 2020-07-29T21:17:40Z

src/compiler/checker.ts

@@ -8069,7 +8069,8 @@ namespace ts {
            if (containsSameNamedThisProperty(expression.left, expression.right)) {
                return anyType;
            }
-            const type = resolvedSymbol ? getTypeOfSymbol(resolvedSymbol) : getWidenedLiteralType(checkExpressionCached(expression.right));
+            const isDirectExport = kind === AssignmentDeclarationKind.ExportsProperty && (isPropertyAccessExpression(expression.left) || isElementAccessExpression(expression.left)) && (isModuleExportsAccessExpression(expression.left.expression) || (isIdentifier(expression.left.expression) && isExportsIdentifier(expression.left.expression)));


Suggested change

const isDirectExport = kind === AssignmentDeclarationKind.ExportsProperty && (isPropertyAccessExpression(expression.left) || isElementAccessExpression(expression.left)) && (isModuleExportsAccessExpression(expression.left.expression) || (isIdentifier(expression.left.expression) && isExportsIdentifier(expression.left.expression)));

const isDirectExport = kind === AssignmentDeclarationKind.ExportsProperty

&& isAccessExpression(expression.left)

&& (isModuleExportsAccessExpression(expression.left.expression) || (isIdentifier(expression.left.expression) && isExportsIdentifier(expression.left.expression)));

I think there might be an existing utility that does what you want here, but it may include other cases. (Could it be isCommonJsExportPropertyAssignment?)

I couldn't find one. :S

It would still be nice to (1) switch to isAccessExpression (2) break up this super-long line

src/compiler/checker.ts

src/compiler/utilities.ts

src/compiler/checker.ts

src/compiler/utilities.ts

weswigham · 2020-07-29T21:55:26Z

I agree with this if we already issue an error for

We do! I was surprised we did as well! But given that, considering the exports as psuedo-readonly makes sense to me.

weswigham · 2020-07-31T18:11:51Z

@typescript-bot user test this

typescript-bot · 2020-07-31T18:11:55Z

Heya @weswigham, I've started to run the parallelized community code test suite on this PR at 380990f. You can monitor the build here.

typescript-bot · 2020-07-31T18:48:47Z

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

weswigham · 2020-07-31T18:58:18Z

Diff from master's user baselines is just a commit sha, so it looks fine~

weswigham · 2020-07-31T18:58:33Z

Lemme fix that lint error...

sandersn

As I said during the design meeting, I think if there aren't many breaking changes in the user tests, you should just merge after we've cut 4.0 RC.

sandersn · 2020-07-31T19:56:10Z

src/compiler/checker.ts

@@ -8069,7 +8069,8 @@ namespace ts {
            if (containsSameNamedThisProperty(expression.left, expression.right)) {
                return anyType;
            }
-            const type = resolvedSymbol ? getTypeOfSymbol(resolvedSymbol) : getWidenedLiteralType(checkExpressionCached(expression.right));
+            const isDirectExport = kind === AssignmentDeclarationKind.ExportsProperty && (isPropertyAccessExpression(expression.left) || isElementAccessExpression(expression.left)) && (isModuleExportsAccessExpression(expression.left.expression) || (isIdentifier(expression.left.expression) && isExportsIdentifier(expression.left.expression)));


It would still be nice to (1) switch to isAccessExpression (2) break up this super-long line

sheetalkamat · 2020-08-03T19:21:54Z

src/compiler/checker.ts

+            const isDirectExport = kind === AssignmentDeclarationKind.ExportsProperty && (isPropertyAccessExpression(expression.left) || isElementAccessExpression(expression.left)) && (isModuleExportsAccessExpression(expression.left.expression) || (isIdentifier(expression.left.expression) && isExportsIdentifier(expression.left.expression)));
+            const type = resolvedSymbol ? getTypeOfSymbol(resolvedSymbol)
+                : isDirectExport ? getRegularTypeOfLiteralType(checkExpressionCached(expression.right))
+                : getWidenedLiteralType(checkExpressionCached(expression.right));
            if (type.flags & TypeFlags.Object &&


If we change how js file works we probably want to change how json works too. As we say that json is nothing but module.exports = json but with json it is questionable if we really need to use literal type or not.

sandersn · 2020-09-03T22:22:32Z

@weswigham what do you think about @sheetalkamat's point about JSON? I think that's the only thing blocking this from being merged.

weswigham · 2020-09-03T23:11:27Z

@weswigham what do you think about @sheetalkamat's point about JSON? I think that's the only thing blocking this from being merged.

I'd rather not change the literaliness of json at only the top level (also it's an export assignment, not an assignment of an individual export, so it's a bit different in structure) since we have ongoing discussions about as const on a json import and the like (which would be much more complete).

sandersn · 2020-09-24T20:52:49Z

@sheetalkamat does that make sense to you? If so, can you sign off?

sheetalkamat · 2020-09-24T21:38:45Z

I think @DanielRosenwasser should comment on this or potentially discuss this in design meeting to ensure we are all on same page. I am not sure why export.j =1 in js file should be different from export = { j : 1} in the json file
On the other hand the repro from the issue this tries to fix atleast somewhat makes sense why the const x = 10; export.x = x should not be widened but i dont think scneario export.x = 1 makes it similar which is similar to json file in my opinion

sandersn · 2021-03-11T01:00:29Z

@DanielRosenwasser can you give an opinion on @sheetalkamat 's question?

DanielRosenwasser · 2021-03-11T01:03:20Z

It sounds like this might be fixed as per #37403 (comment)

ExE-Boss · 2021-03-11T08:45:16Z

Actually, #37404 is still broken, and #37403 was only partially fixed, so this is still needed.

weswigham · 2021-03-11T21:45:04Z

Yeah, I can't actually guide/update a thing right now, soo.... would need help from y'all 🤷

sandersn · 2021-05-28T21:53:46Z

Re-reading this, I vote that we take this change and then discuss a change to JSON literalness separately. I think this is a good change on its own and the mismatch with JSON won't be too noticeable.

@sheetalkamat is that OK with you?
If so, @weswigham, can you bring this back to date with master?

weswigham · 2022-03-09T20:42:02Z

@sandersn been awhile, but think we wanna merge this now?

Make direct assignments to cjs exports considered literal contexts

249f925

weswigham requested a review from sandersn July 29, 2020 19:52

typescript-bot assigned weswigham Jul 29, 2020

typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone For Backlog Bug PRs that fix a backlog bug labels Jul 29, 2020

weswigham added the Breaking Change Would introduce errors in existing code label Jul 29, 2020

sandersn requested changes Jul 29, 2020

View reviewed changes

Style feedback from PR

380990f

weswigham added this to the TypeScript 4.1.0 milestone Jul 31, 2020

typescript-bot mentioned this pull request Jul 31, 2020

🤖 User test baselines have changed for export-assignments-nonmutable typescript-bot/TypeScript#69

Closed

Trailing whitespaaaaace

90311dc

weswigham requested a review from sandersn July 31, 2020 19:03

sandersn approved these changes Jul 31, 2020

View reviewed changes

DanielRosenwasser mentioned this pull request Jul 31, 2020

Design Meeting Notes, 7/31/2020 #39852

Closed

sheetalkamat reviewed Aug 3, 2020

View reviewed changes

sandersn assigned sheetalkamat Sep 3, 2020

Merge branch 'master' into export-assignments-nonmutable

96fcae9

RyanCavanaugh modified the milestones: TypeScript 4.1.0, TypeScript 4.3.0 Mar 4, 2021

RyanCavanaugh removed this from the TypeScript 4.3.0 milestone Aug 5, 2021

Merge branch 'main' into export-assignments-nonmutable

06d309a

weswigham merged commit 3f63804 into microsoft:main Mar 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make direct assignments to cjs exports considered literal contexts #39816

Make direct assignments to cjs exports considered literal contexts #39816

weswigham commented Jul 29, 2020 •

edited

Loading

sandersn left a comment

sandersn Jul 29, 2020

weswigham Jul 29, 2020

sandersn Jul 31, 2020 •

edited

Loading

weswigham commented Jul 29, 2020

weswigham commented Jul 31, 2020

typescript-bot commented Jul 31, 2020 •

edited

Loading

typescript-bot commented Jul 31, 2020

weswigham commented Jul 31, 2020

weswigham commented Jul 31, 2020

sandersn left a comment

sandersn Jul 31, 2020 •

edited

Loading

sheetalkamat Aug 3, 2020

sandersn commented Sep 3, 2020

weswigham commented Sep 3, 2020

sandersn commented Sep 24, 2020

sheetalkamat commented Sep 24, 2020

sandersn commented Mar 11, 2021

DanielRosenwasser commented Mar 11, 2021

ExE-Boss commented Mar 11, 2021 •

edited

Loading

weswigham commented Mar 11, 2021

sandersn commented May 28, 2021

weswigham commented Mar 9, 2022

Make direct assignments to cjs exports considered literal contexts #39816

Make direct assignments to cjs exports considered literal contexts #39816

Conversation

weswigham commented Jul 29, 2020 • edited Loading

sandersn left a comment

Choose a reason for hiding this comment

sandersn Jul 29, 2020

Choose a reason for hiding this comment

weswigham Jul 29, 2020

Choose a reason for hiding this comment

sandersn Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

weswigham commented Jul 29, 2020

weswigham commented Jul 31, 2020

typescript-bot commented Jul 31, 2020 • edited Loading

typescript-bot commented Jul 31, 2020

weswigham commented Jul 31, 2020

weswigham commented Jul 31, 2020

sandersn left a comment

Choose a reason for hiding this comment

sandersn Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

sheetalkamat Aug 3, 2020

Choose a reason for hiding this comment

sandersn commented Sep 3, 2020

weswigham commented Sep 3, 2020

sandersn commented Sep 24, 2020

sheetalkamat commented Sep 24, 2020

sandersn commented Mar 11, 2021

DanielRosenwasser commented Mar 11, 2021

ExE-Boss commented Mar 11, 2021 • edited Loading

weswigham commented Mar 11, 2021

sandersn commented May 28, 2021

weswigham commented Mar 9, 2022

weswigham commented Jul 29, 2020 •

edited

Loading

sandersn Jul 31, 2020 •

edited

Loading

typescript-bot commented Jul 31, 2020 •

edited

Loading

sandersn Jul 31, 2020 •

edited

Loading

ExE-Boss commented Mar 11, 2021 •

edited

Loading