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

Parser: fix #4590, correct to parse empty parenthesis "()" #4592

Merged

Conversation

makenowjust
Copy link
Contributor

Currently "()" is parsed as NilLiteral, but it loses parenthesis information and it produces a formatter bug (#4590).

Correct to parse "()" as Expressions whose keyword property is :"(" and which contains Nop only. This looks like empty begin block. Also formatter works fine.

@RX14
Copy link
Contributor

RX14 commented Jun 19, 2017

Should it not just remove the empty brackets? Why would you use empty brackets anyway?

@makenowjust
Copy link
Contributor Author

@RX14 I believe I and we don't write empty parenthesis by hand, but it is useful for code generated by program.

@RX14
Copy link
Contributor

RX14 commented Jun 19, 2017

@makenowjust but do empty brackets change the meaning of the program? I can't see a way they do so shouldn't they just be removed?

@makenowjust
Copy link
Contributor Author

@RX14 It has the result value nil. For example foo = () is valid program, but when () is removed, of course foo = is invalid.

@RX14
Copy link
Contributor

RX14 commented Jun 19, 2017

@makenowjust Ah I see, yes this makes sense now.

@jhass
Copy link
Member

jhass commented Jun 19, 2017

Another point is that an editor integration that continously formats shouldn't remove them.

@asterite
Copy link
Member

I think I copied () from Ruby, but I don't see the point about it. foo = () shouldn't be a valid program either. I'd say we remove empty parentheses from the language, they have no use.

@makenowjust
Copy link
Contributor Author

@asterite How do you think about expressions list inside parenthesis like (foo; bar)? Is it needed? I think it is needless if () is needless.

@asterite
Copy link
Member

For multi expression one needs multiple expressions :-)

So (1; 2) is allowed, (1) is allowed, but () shouldn't parse. I added that at one point because Ruby does that, but I don't think it makes much sense. Not even for code generation.

@makenowjust
Copy link
Contributor Author

Multiple expressions is needed, really? I think all multiple expressions can be converted to begin ... end block. Mostly multiple expressions makes code hard to read. Instead of using multiple expressions, I prefer clearer way (i.e. using temporally variable).

@straight-shoota
Copy link
Member

A somewhat usefully application would be joining some expressions in a macro, like:

macro exprs(*exprs)
  ({{ exprs.join(";").id }}).to_s
end

exprs(1, 2 + 3)
exprs

If an empty () is valid and evaluates as nil there is no need to handle the case exprs.size == 0. Tough I don't have a real world use case for this.

@makenowjust begin and end adds much clutter, just having parenthesis could be better readable for short expressions

@makenowjust
Copy link
Contributor Author

@straight-shoota

begin and end adds much clutter, just having parenthesis could be better readable for short expressions

Please give me a real example. I thought it a day, but I couldn't find it.
For example foo + (bar; baz) looks good parenthesis multiple expressions use case. But you should consider more deeply. When we need to write this code, foo, bar and baz have side effects surely (because we expect foo, bar and baz are evaluated in a sequential order, and if not we can rewrite it more clearly like bar; foo + baz). In this case such a tricky code isn't preferred. We should write a clearer code like foo = foo(); bar; baz = baz(); foo + baz.

Important points are two:

  1. All multiple expressions are used with some side effects (internal state modification, network I/O and etc).
  2. We should treat side effects carefully. They may make many bugs.

Thank you.

@makenowjust
Copy link
Contributor Author

Every expressions have no side effect, this discussion can be thrown out, though we can't see Hello, World! on our console.

@faustinoaq
Copy link
Contributor

faustinoaq commented Jun 21, 2017

Hi, could be this a final solution to #4590?

@straight-shoota
Copy link
Member

It is intended as such see fix #4590 in the title.

@makenowjust
Copy link
Contributor Author

In fact, no multiple expressions with parenthesis is used in compiler source codes, standard library and all samples in this repository. Where is real world use case?

@mverzilli
Copy link

Let's remove () from the language as @asterite suggested. Closing this. PRs to remove () are welcome :).

@oprypin
Copy link
Member

oprypin commented Jun 22, 2017

Why not merge this clearly useful fix and then decide the fate of the syntax

Currently "()" is parsed as NilLiteral, but it loses parenthesis
information and it produces a formatter bug (crystal-lang#4590).

Correct to parse "()" as Expressions whose keyword property is :"(" and
which contains `Nop` only. This looks like empty begin block.
Also formatter works fine.
@makenowjust makenowjust force-pushed the fix/crystal/4590-wrap-paren-nop branch from 0dead34 to a7f574d Compare July 19, 2017 12:32
@makenowjust
Copy link
Contributor Author

@mverzilli ping

@mverzilli
Copy link

@RX14 @ysbaddaden @bcardiff @matiasgarciaisaia this is ready to merge IMO.

@makenowjust
Copy link
Contributor Author

@mverzilli Thank you ❤️

@RX14 RX14 merged commit eb52d56 into crystal-lang:master Jul 19, 2017
@makenowjust makenowjust deleted the fix/crystal/4590-wrap-paren-nop branch July 19, 2017 14:13
@mverzilli mverzilli added this to the Next milestone Jul 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants