Skip to content

Port return conditional expression checking improvements #1072

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.

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

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Jun 5, 2025

closes #984
ports part of microsoft/TypeScript#56941 that wasn't reverted by microsoft/TypeScript#61136

@Copilot Copilot AI review requested due to automatic review settings June 5, 2025 19:03
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors return-type checks by introducing a unified checkReturnExpression helper, updates the AST to recognize conditional expressions, and refreshes the compiler test baselines to reflect improved error reporting for conditional returns.

  • Consolidate duplicated return-type validation into a new checkReturnExpression method.
  • Add ast.IsConditionalExpression to detect ternary expressions during type checks.
  • Refresh test baselines under testdata/baselines/reference/submodule/compiler for conditional return errors.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
testdata/baselines/reference/submodule/compiler/conditionalReturnExpression.errors.txt.diff Removed obsolete .diff baseline now replaced by updated .txt.
testdata/baselines/reference/submodule/compiler/conditionalReturnExpression.errors.txt Updated error positions and messages for conditional return tests.
internal/checker/checker.go Added checkReturnExpression and replaced inline return checks.
internal/ast/ast.go Introduced IsConditionalExpression helper.
Comments suppressed due to low confidence (3)

internal/checker/checker.go:3896

  • [nitpick] The parameter named node is ambiguous when expr is also passed. Consider renaming node to something like errorContext or checkTarget to clarify its role as the context for diagnostics.
func (c *Checker) checkReturnExpression(container *ast.Node, unwrappedReturnType *Type, node *ast.Node, expr *ast.Node, exprType *Type, inConditionalExpression bool) {

testdata/baselines/reference/submodule/compiler/conditionalReturnExpression.errors.txt.diff:1

  • [nitpick] This .diff file has been emptied by the change. If it's no longer used in CI/test tooling, consider deleting it from the repo to avoid confusion.
diff --git a/testdata/baselines/reference/submodule/compiler/conditionalReturnExpression.errors.txt.diff b/testdata/baselines/reference/submodule/compiler/conditionalReturnExpression.errors.txt.diff

internal/checker/checker.go:3901

  • [nitpick] Consider adding a new unit test that covers nested conditional expressions in async functions to ensure checkReturnExpression handles combined async/ternary cases correctly.
if ast.IsConditionalExpression(unwrappedExpr) {

@Andarist Andarist force-pushed the port/per-branch-conditional-expression-checks branch from 8b7afeb to 0def524 Compare June 5, 2025 19:04
@jakebailey jakebailey closed this Jun 5, 2025
@jakebailey jakebailey reopened this Jun 5, 2025
@jakebailey
Copy link
Member

The baselines imply this is right, but is there any way you could link to the PRs you implemented here so I can mark them off as ported?

@Andarist
Copy link
Contributor Author

Andarist commented Jun 6, 2025

@jakebailey I added the links you asked for :)

@Andarist Andarist force-pushed the port/per-branch-conditional-expression-checks branch from 0def524 to 95c1e6b Compare June 6, 2025 08:53
@gabritto gabritto added this pull request to the merge queue Jun 6, 2025
Merged via the queue into microsoft:main with commit 4822b79 Jun 6, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TS Errors on different lines - return with ternary
3 participants