GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — No error on toplevel return in JS by sandersn · Pull Request #48874 · microsoft/TypeScript

Conversation

@sandersn

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

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sandersn
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 (typescript-bot) assigned Nathan Shively-Sanders (sandersn) Apr 28, 2022
@typescript-bot TypeScript Bot (typescript-bot) added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Apr 28, 2022
@sandersn
Nathan Shively-Sanders (sandersn) requested review from Daniel Rosenwasser (DanielRosenwasser), Gabriela Araujo Britto (gabritto) and Jake Bailey (jakebailey) April 28, 2022 15:58
jakebailey
Jake Bailey (jakebailey) reviewed Apr 28, 2022
View reviewed changes
Comment thread tests/cases/conformance/salsa/plainJSGrammarErrors.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
Daniel Rosenwasser (DanielRosenwasser) and others added 4 commits April 29, 2022 16:28
@DanielRosenwasser
Restore 'return' statement.
ecd14c3
@typescript-bot
Update Baselines and/or Applied Lint Fixes
55444ac
@sandersn
Merge branch 'main' into no-error-on-toplevel-return-in-js
6340b62
@sandersn
Re-add missing baselines
0aab0d1
@gabritto

Gabriela Araujo Britto (gabritto) commented May 2, 2022

Copy link
Copy Markdown
Member

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sandersn

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

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sandersn

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

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gabritto

Gabriela Araujo Britto (gabritto) commented May 2, 2022

Copy link
Copy Markdown
Member

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Nathan Shively-Sanders (sandersn) added 2 commits May 2, 2022 11:11
@sandersn
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.
@sandersn
Keep Disallowing return in ambient locations
714ea8e
jakebailey
Jake Bailey (jakebailey) reviewed May 2, 2022
View reviewed changes
Comment thread tests/baselines/reference/multiLinePropertyAccessAndArrowFunctionIndent1.types Outdated
>this : typeof globalThis
>edit : any
>role : any
>role : error

@jakebailey Jake Bailey (jakebailey) May 2, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) reviewed May 2, 2022
View reviewed changes
Comment thread ...aselines/reference/showConfig/Shows tsconfig for single option/noDtsResolution/tsconfig.json
@@ -1,5 +0,0 @@
{
"compilerOptions": {

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) May 2, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why did this get deleted?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sandersn Nathan Shively-Sanders (sandersn) May 4, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sandersn Nathan Shively-Sanders (sandersn) May 4, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I undid the delete but all tests pass either way.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) requested changes May 2, 2022
View reviewed changes
Comment thread tests/baselines/reference/parserStatementIsNotAMemberVariableDeclaration1.errors.txt
Comment on lines -5 to -16
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;

}

}; No newline at end of file

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) May 2, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented May 2, 2022

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sandersn

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

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Nathan Shively-Sanders (sandersn) added 2 commits May 2, 2022 14:45
@sandersn
Allow toplevel return only in non-ESM JS files
6291ae3
@sandersn
Add test of toplevel return in JS script
2a6dec4
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) requested changes May 3, 2022
View reviewed changes

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/checker.ts Outdated

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

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) May 3, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) May 3, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sandersn Nathan Shively-Sanders (sandersn) May 4, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Nathan Shively-Sanders (sandersn) added 4 commits May 4, 2022 07:43
@sandersn
Revert "Add test of toplevel return in JS script"
cd780fb 
This reverts commit 2a6dec4.
@sandersn
Revert "Allow toplevel return only in non-ESM JS files"
4a39e95 
This reverts commit 6291ae3.
@sandersn
Revert "Keep Disallowing return in ambient locations"
f25876b 
This reverts commit 714ea8e.
@sandersn
Revert "No error in toplevel script files"
0473df6 
This reverts commit 2056e13.
Nathan Shively-Sanders (sandersn) added 2 commits May 4, 2022 08:30
@sandersn
Merge branch 'main' into no-error-on-toplevel-return-in-js
ea8aa63
@sandersn
restore orphaned baseline
a58d92e
jakebailey
Jake Bailey (jakebailey) approved these changes May 4, 2022
View reviewed changes

@jakebailey Jake Bailey (jakebailey) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sandersn

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

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sandersn
Nathan Shively-Sanders (sandersn) merged commit 650c056 into main May 5, 2022
@sandersn
Nathan Shively-Sanders (sandersn) deleted the no-error-on-toplevel-return-in-js branch May 5, 2022 16:35
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) DanielRosenwasser requested changes
@jakebailey Jake Bailey (jakebailey) jakebailey approved these changes
@gabritto Gabriela Araujo Britto (gabritto) Awaiting requested review from gabritto

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@sandersn Nathan Shively-Sanders (sandersn)

Labels

Author: Team For Milestone Bug PRs that fix a bug with a specific milestone

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TS1108 "problem" showing on pure Javascript (node) file - Erroneous Error Flag

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@sandersn @gabritto @DanielRosenwasser @jakebailey @typescript-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.