No error on toplevel return in JS by sandersn · Pull Request #48874 · microsoft/TypeScript

Nathan Shively-Sanders (sandersn) · 2022-04-28T15:56:43Z

Turns out it's only an error in modules. It's possible to keep this error on the list of "OK for JS" errors and make the checker code stop issuing it for JS scripts only. However, I don't think the error is valuable enough to do that.

Fixes #48224

Turns out it's only an error in modules. It's possible to keep this error on the list of "OK for JS" errors and make the checker code stop issuing it for JS scripts only. However, I don't think the error is valuable enough to do that. Fixes #48224

Gabriela Araujo Britto (gabritto) · 2022-05-02T15:49:09Z

Is detecting if a js file is a script or a module not reliable? I'm not sure I understand why you feel it is not valuable enough to do this

Nathan Shively-Sanders (sandersn) · 2022-05-02T17:29:02Z

Making decisions based on JS-vs-TS is not a common thing to do in the middle of the checker. Of course, there are some constructs like commonjs imports that should only show up in JS, but the checker treats them the same regardless of location. In this case, it would have to skip the error only in the case that top-level return is in a JS file.

Nathan Shively-Sanders (sandersn) · 2022-05-02T17:33:29Z

Alternatively, we could instead stop issuing the error for any script. It's really good default assumption that a .ts script will emit to .js script that will run in node or the browser, where top-level return is legal.

Actually, I kind of like that idea. Let me try it.

Gabriela Araujo Britto (gabritto) · 2022-05-02T17:40:50Z

Alternatively, we could instead stop issuing the error for any script. It's really good default assumption that a .ts script will emit to .js script that will run in node or the browser, where top-level return is legal.

Actually, I kind of like that idea. Let me try it.

yeah, that's what I was thinking and it seems reasonable to me

Only issue "no top-level return" error for modules, not scripts, regardless of whether it's TS or JS.

Jake Bailey (jakebailey) · 2022-05-02T20:30:15Z

 >this : typeof globalThis
 >edit : any
->role : any
+>role : error


This is a weird change... this is the role parameter in this test's confusing return statement. Not sure why it went from an implicit any to an error.

Daniel Rosenwasser (DanielRosenwasser) · 2022-05-02T20:47:58Z

@@ -1,5 +0,0 @@
-{
-    "compilerOptions": {


Why did this get deleted?

I'm pretty sure this is an orphaned baseline. I have no idea how I decided to delete it though.

I undid the delete but all tests pass either way.

Daniel Rosenwasser (DanielRosenwasser) · 2022-05-02T20:49:08Z

-    return {
-    ~~~~~~
-!!! error TS1108: A 'return' statement can only be used within a function body.
-
-      "set": function (key, value) {
-
-        // 'private' should not be considered a member variable here.
-        private[key] = value;
-
-      }
-
-    };


We're still supposed to disallow this in TS files, right?

Daniel Rosenwasser (DanielRosenwasser) · 2022-05-02T20:50:35Z

Allowing return at the top-level of a .ts file was never allowed, and probably shouldn't be allowed now. Node.js is one of the few environments that actually allows this, and new code should use process.exit(0) to work across environments.

Nathan Shively-Sanders (sandersn) · 2022-05-02T21:44:40Z

Hmm, OK. I looked again and there are piles of isInJSFile calls throughout the checker, so I guess I made up the "don't check for js" rule there.

I'll push a commit that makes the checker issue the error for all non-js files and for JS files that use ES modules.

Daniel Rosenwasser (DanielRosenwasser)

Just undo the introduction of this check. It used to only show up in TS files and in JS files when checkJS was on. Reinstate that behavior.

Daniel Rosenwasser (DanielRosenwasser) · 2022-05-03T22:34:48Z


            if (!container) {
-                grammarErrorOnFirstToken(node, Diagnostics.A_return_statement_can_only_be_used_within_a_function_body);
+                if (!!getSourceFileOfNode(node).externalModuleIndicator || !isInJSFile(node)) {


Suggested change

if (!!getSourceFileOfNode(node).externalModuleIndicator || !isInJSFile(node)) {

if (!isInJSFile(node)) {

In fact, I would say that the right fix is still to issue the error in checkJs, but not when providing the default grammar errors for JS files.

We should maybe revisit this after the beta since Gabriela's semantics are more accurate, but for the purposes of shipping a fix, it's definitely simpler to not show the error in any plain JS file.

This reverts commit 2a6dec4.

This reverts commit 6291ae3.

This reverts commit 714ea8e.

This reverts commit 2056e13.

Jake Bailey (jakebailey)

This LGTM now, a conservative choice, but I don't know if Daniel Rosenwasser (@DanielRosenwasser) or Gabriela Araujo Britto (@gabritto) have any other last comments. I know Danial's PR status is still requesting changes.

Nathan Shively-Sanders (sandersn) · 2022-05-05T16:34:34Z

I'm pretty sure I addressed Daniel's comments; Daniel, Gabriela and I will need to decide what future semantics should be separately, so I'm going to merge this PR so that it's not hanging around until the last minute of the beta.

No error on toplevel return in JS

f6a55eb

Turns out it's only an error in modules. It's possible to keep this error on the list of "OK for JS" errors and make the checker code stop issuing it for JS scripts only. However, I don't think the error is valuable enough to do that. Fixes #48224

TypeScript Bot (typescript-bot) assigned Nathan Shively-Sanders (sandersn) Apr 28, 2022

TypeScript Bot (typescript-bot) added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Apr 28, 2022

Nathan Shively-Sanders (sandersn) requested review from Daniel Rosenwasser (DanielRosenwasser), Gabriela Araujo Britto (gabritto) and Jake Bailey (jakebailey) April 28, 2022 15:58

Jake Bailey (jakebailey) reviewed Apr 28, 2022

View reviewed changes

Comment thread tests/cases/conformance/salsa/plainJSGrammarErrors.ts

Daniel Rosenwasser (DanielRosenwasser) and others added 4 commits April 29, 2022 16:28

Restore 'return' statement.

ecd14c3

Update Baselines and/or Applied Lint Fixes

55444ac

Merge branch 'main' into no-error-on-toplevel-return-in-js

6340b62

Re-add missing baselines

0aab0d1

Nathan Shively-Sanders (sandersn) added 2 commits May 2, 2022 11:11

No error in toplevel script files

2056e13

Only issue "no top-level return" error for modules, not scripts, regardless of whether it's TS or JS.

Keep Disallowing return in ambient locations

714ea8e

Jake Bailey (jakebailey) reviewed May 2, 2022

View reviewed changes

Daniel Rosenwasser (DanielRosenwasser) reviewed May 2, 2022

View reviewed changes

Daniel Rosenwasser (DanielRosenwasser) requested changes May 2, 2022

View reviewed changes

Nathan Shively-Sanders (sandersn) added 2 commits May 2, 2022 14:45

Allow toplevel return only in non-ESM JS files

6291ae3

Add test of toplevel return in JS script

2a6dec4

Daniel Rosenwasser (DanielRosenwasser) requested changes May 3, 2022

View reviewed changes

Nathan Shively-Sanders (sandersn) added 4 commits May 4, 2022 07:43

Revert "Add test of toplevel return in JS script"

cd780fb

This reverts commit 2a6dec4.

Revert "Allow toplevel return only in non-ESM JS files"

4a39e95

This reverts commit 6291ae3.

Revert "Keep Disallowing return in ambient locations"

f25876b

This reverts commit 714ea8e.

Revert "No error in toplevel script files"

0473df6

This reverts commit 2056e13.

Nathan Shively-Sanders (sandersn) added 2 commits May 4, 2022 08:30

Merge branch 'main' into no-error-on-toplevel-return-in-js

ea8aa63

restore orphaned baseline

a58d92e

Jake Bailey (jakebailey) approved these changes May 4, 2022

View reviewed changes

Nathan Shively-Sanders (sandersn) merged commit 650c056 into main May 5, 2022

Nathan Shively-Sanders (sandersn) deleted the no-error-on-toplevel-return-in-js branch May 5, 2022 16:35

Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025

	if (!!getSourceFileOfNode(node).externalModuleIndicator \|\| !isInJSFile(node)) {
	if (!isInJSFile(node)) {

Conversation

Nathan Shively-Sanders (sandersn) commented Apr 28, 2022

Uh oh!

Uh oh!

Gabriela Araujo Britto (gabritto) commented May 2, 2022

Uh oh!

Nathan Shively-Sanders (sandersn) commented May 2, 2022

Uh oh!

Nathan Shively-Sanders (sandersn) commented May 2, 2022

Uh oh!

Gabriela Araujo Britto (gabritto) commented May 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Daniel Rosenwasser (DanielRosenwasser) commented May 2, 2022

Uh oh!

Nathan Shively-Sanders (sandersn) commented May 2, 2022

Uh oh!

Daniel Rosenwasser (DanielRosenwasser) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jake Bailey (jakebailey) left a comment

Choose a reason for hiding this comment

Uh oh!

Nathan Shively-Sanders (sandersn) commented May 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants