No error on toplevel return in JS #48874

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

tests/cases/conformance/salsa/plainJSGrammarErrors.ts

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

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.

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.

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.

jakebailey · 2022-05-02T20:30:15Z

tests/baselines/reference/multiLinePropertyAccessAndArrowFunctionIndent1.types

@@ -6,12 +6,12 @@ return this.edit(role)
 >this.edit : any
 >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.

DanielRosenwasser · 2022-05-02T20:47:58Z

...aselines/reference/showConfig/Shows tsconfig for single option/noDtsResolution/tsconfig.json

@@ -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.

DanielRosenwasser · 2022-05-02T20:49:08Z

tests/baselines/reference/parserStatementIsNotAMemberVariableDeclaration1.errors.txt

-    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?

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.

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.

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.

DanielRosenwasser · 2022-05-03T22:34:48Z

src/compiler/checker.ts

@@ -38913,7 +38913,9 @@ namespace ts {
            }

            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.

jakebailey

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

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 assigned sandersn Apr 28, 2022

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

sandersn requested review from DanielRosenwasser, gabritto and jakebailey April 28, 2022 15:58

jakebailey reviewed Apr 28, 2022

View reviewed changes

tests/cases/conformance/salsa/plainJSGrammarErrors.ts Outdated Show resolved Hide resolved

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

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

jakebailey reviewed May 2, 2022

View reviewed changes

DanielRosenwasser reviewed May 2, 2022

View reviewed changes

DanielRosenwasser requested changes May 2, 2022

View reviewed changes

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

DanielRosenwasser requested changes May 3, 2022

View reviewed changes

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.

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

jakebailey approved these changes May 4, 2022

View reviewed changes

sandersn merged commit 650c056 into main May 5, 2022

sandersn deleted the no-error-on-toplevel-return-in-js branch May 5, 2022 16:35

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

No error on toplevel return in JS #48874

No error on toplevel return in JS #48874

Uh oh!

Conversation

sandersn commented Apr 28, 2022

Uh oh!

Uh oh!

gabritto commented May 2, 2022

Uh oh!

sandersn commented May 2, 2022

Uh oh!

sandersn commented May 2, 2022

Uh oh!

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!

DanielRosenwasser commented May 2, 2022

Uh oh!

sandersn commented May 2, 2022

Uh oh!

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!

jakebailey left a comment

Choose a reason for hiding this comment

Uh oh!

sandersn commented May 5, 2022

Uh oh!

Uh oh!