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

Parse assert as a regular function #6180

Merged
merged 27 commits into from Apr 20, 2023

Conversation

aspeddro
Copy link
Contributor

@aspeddro aspeddro commented Apr 20, 2023

rescript-lang/syntax#168

TODO:

  • update CHANGELOG

@cristianoc
Copy link
Collaborator

Quick check before even reading the code: this still parses the old style and converts to the new style, correct?

@aspeddro
Copy link
Contributor Author

No, old style is a syntax error.

@cristianoc
Copy link
Collaborator

That's a breaking change.
Should accept it and convert it on format.

@aspeddro
Copy link
Contributor Author

We can convert to the new format, but it changes the precedence.

// Before
assert 1 == 2
// Parsed as
(assert 1) == 2

// Now
assert(1 == 2)

Currently assert is an unary expression.

@cristianoc
Copy link
Collaborator

I think that OK. It's going to matter in very few cases.

@cristianoc
Copy link
Collaborator

Still: to be mentioned in release notes.

@cristianoc
Copy link
Collaborator

Are there tests that show that old style assert is still parsed? E.g. assert false without parens.

| Nothing -> doc
let doc = printExpressionWithComments ~state expr cmtTbl in
let expr =
match expr.pexp_desc with
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I understand why this code is required.
Seems oddly specific for a generic assert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in ef6501e. It is in fact no longer necessary.

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Looking great!
Ready to merge?

@aspeddro
Copy link
Contributor Author

👍🏾

@cristianoc cristianoc merged commit ec6ae34 into rescript-lang:master Apr 20, 2023
7 checks passed
@DZakh
Copy link
Contributor

DZakh commented Apr 28, 2023

It's the first time a hear about assert. Also, I haven't found it in the docs (besides the reserved keywords section). I wonder whether it should be removed from the compiler entirely and come to the userland.

@cristianoc
Copy link
Collaborator

It's the first time a hear about assert. Also, I haven't found it in the docs (besides the reserved keywords section). I wonder whether it should be removed from the compiler entirely and come to the userland.

The thing is avoiding breaking changes.
This is a small thing but by experience a couple of small things are enough to delay people upgrading compiler version.

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.

None yet

3 participants