Skip to content
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

fix(ts): parenthesized assert and assign #12933

Merged
merged 3 commits into from Mar 22, 2021

Conversation

fedeci
Copy link
Member

@fedeci fedeci commented Feb 28, 2021

Q                       A
Fixed Issues? Fixes #12932
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

We treat ParenthesizedExpression node as a TSAsExpression in #toAssignable() so that the function recurses one more time.

@babel-bot
Copy link
Collaborator

babel-bot commented Feb 28, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/44539/

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 28, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 62dc9f1:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Can you add a test that with the TS plugin and createParenthesizedExpression ({}) = x still throws?

@nicolo-ribaudo nicolo-ribaudo added pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Feb 28, 2021
@fedeci
Copy link
Member Author

fedeci commented Feb 28, 2021

It broke that, what about adding a check like this to the superclass' toAssignable

isParenthesizedAssignable(node) {
  const type = node.type
  return (
    type === "TSParameterProperty" || 
    type === "TSAsExpression" || 
    ... || 
    super.isParenthesizedAssignable(node)
  )
}

Co-Authored-By: Nicolò Ribaudo <7000710+nicolo-ribaudo@users.noreply.github.com>
case "ParenthesizedExpression":
if (
node.expression.type === "TSAsExpression" ||
node.expression.type === "ParenthesizedExpression"
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess TSNonNullExpression and TSTypeAssertion should be here too

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I tried running some tests on those, but they work like magic😁

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Like @thorn0 said, we still need to check for TSNonNullExpression and TSTypeAssertion. It the test is passing maybe there isn't a test which enables createParenthesizedExpressions. Clearly Babel parser 7.12 fails: https://astexplorer.net/#/gist/910487b331a19aac83c5489fa4203e66/4180181cff60d53a164ab16afb82a8e6e7d5698c

The error is thrown from

this.raise(node.start, Errors.InvalidParenthesizedAssignment);
, which assumes that parenthesized assignment can be either identifier / member expression. This is not the cases for TypeScript, where TSNonNullExpression, TSAsExpression and TSTypeAssertion plays a role of transparent expression wrapper, which should have been unwrapped before check. Consider handle this as we did in

export function isTransparentExprWrapper(node: Node) {
return (
t.isTSAsExpression(node) ||
t.isTSTypeAssertion(node) ||
t.isTSNonNullExpression(node) ||
t.isTypeCastExpression(node) ||
t.isParenthesizedExpression(node)
);

I think we can also simplifies how we handle toAssignable in the typescript plugin using this helper.

@fedeci
Copy link
Member Author

fedeci commented Mar 1, 2021

isParenthesizedAssignable(node) {

  const type = node.type

  return (

    type === "TSParameterProperty" || 

    type === "TSAsExpression" || 

    ... || 

    super.isParenthesizedAssignable(node)

  )

}

Maybe I got confused and I tested it with this code which does almost the same of that helper.

@JLHwung
Copy link
Contributor

JLHwung commented Mar 18, 2021

Can you add following test cases with { "createParenthesizedExpressions": true }?

(a!) = null;
(<string>a) = null;

Like Georgii mentioned before, they should not throw.

@nicolo-ribaudo
Copy link
Member

Thanks!

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

I'll merge this in 15 mins, as soon as the release CI job on main finishes

@nicolo-ribaudo nicolo-ribaudo merged commit 4f72713 into babel:main Mar 22, 2021
@fedeci fedeci deleted the fix-parenthesized-assert-and-assign branch March 22, 2021 16:43
fisker added a commit to fisker/prettier that referenced this pull request Mar 29, 2021
thorn0 pushed a commit to prettier/prettier that referenced this pull request Mar 29, 2021
* Throw errors for invalid left-hand side assignment

* Fix tests

* FIx tests

* Add tests from babel/babel#12933
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 22, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript: 'assert and assign' vs createParenthesizedExpressions
5 participants