Add &+ &- &* &** operators parsing #6329

bcardiff · 2018-07-04T11:07:29Z

Assignment operators are supported for &+= &-= &*=.

Since **= is not currently supported, neither is &**=.

Since &- collide with &->proc that expression needs to be written as &(->proc) after this PR. Unless the lexer is changed to add lookahead in that case there is a slightly breaking change here.
I'm fine with this breaking change.

Ref: this is part of the follow up of #6223 (comment)

Assignment operators are supported for &+= &-= &*=

jhass · 2018-07-04T11:13:26Z

spec/compiler/normalize/op_assign_spec.cr

-  it "normalizes var +=" do
-    assert_normalize "a = 1; a += 2", "a = 1\na = a + 2"
+  ["+", "-", "*", "&+", "&-", "&*"].each do |op|
+    it "normalizes var +=" do


Might be a good opportunity to update the description to "normalizes var #{op}="

RX14 · 2018-07-04T11:27:35Z

Can we get formatter specs on these?

bcardiff · 2018-07-04T11:44:14Z

Yes, I was up to do that in other PR, but can do it here...

straight-shoota · 2018-07-04T11:57:11Z

spec/compiler/codegen/proc_spec.cr

@@ -600,13 +600,13 @@ describe "Code gen: proc" do
      )).to_i.should eq(1)
  end

-  it "passes proc as &-> to method that yields" do
+  it "passes proc as &(->expr) to method that yields" do


Why not add a lookahead to avoid this breaking change?

Because the current lexer does not do lookahead AFAIK. So is changing a bit the nature of it for this case. And that syntax is not wide used.

peek_next_char?

You can use this diff:

diff --git a/spec/compiler/codegen/proc_spec.cr b/spec/compiler/codegen/proc_spec.cr index bee6aa1f0..4e08c075c 100644 --- a/spec/compiler/codegen/proc_spec.cr +++ b/spec/compiler/codegen/proc_spec.cr @@ -600,13 +600,13 @@ describe "Code gen: proc" do )).to_i.should eq(1) end - it "passes proc as &(->expr) to method that yields" do + it "passes proc as &->expr to method that yields" do run(%( def foo yield end - foo &(->{ 123 }) + foo &->{ 123 } )).to_i.should eq(123) end diff --git a/spec/compiler/parser/parser_spec.cr b/spec/compiler/parser/parser_spec.cr index 3493e4c83..168b18344 100644 --- a/spec/compiler/parser/parser_spec.cr +++ b/spec/compiler/parser/parser_spec.cr @@ -1085,6 +1085,8 @@ module Crystal it_parses "->foo=", ProcPointer.new(nil, "foo=") it_parses "foo = 1; ->foo.foo=", [Assign.new("foo".var, 1.int32), ProcPointer.new("foo".var, "foo=")] + it_parses "foo &->bar", Call.new(nil, "foo", block_arg: ProcPointer.new(nil, "bar")) + it_parses "foo.bar = {} of Int32 => Int32", Call.new("foo".call, "bar=", HashLiteral.new(of: HashLiteral::Entry.new("Int32".path, "Int32".path))) it_parses "alias Foo = Bar", Alias.new("Foo", "Bar".path) diff --git a/src/compiler/crystal/syntax/lexer.cr b/src/compiler/crystal/syntax/lexer.cr index c02bc8364..915ab8da7 100644 --- a/src/compiler/crystal/syntax/lexer.cr +++ b/src/compiler/crystal/syntax/lexer.cr @@ -596,11 +596,18 @@ module Crystal @token.type = :"&+" end when '-' - case next_char - when '=' - next_char :"&-=" + # Check if '>' comes after '&-', making it '&->'. + # We want to parse that like '&(->...)', + # so we only return '&' for now. + if peek_next_char == '>' + @token.type = :"&" else - @token.type = :"&-" + case next_char + when '=' + next_char :"&-=" + else + @token.type = :"&-" + end end when '*' case next_char

Update: I just edited the diff because you can call peek_next_char instead of reader.peek_next_char

Updated again to revert the change in the spec that uses &-> where parentheses were added.

Diff merged. Thanks @asterite

Sija · 2018-07-04T11:58:43Z

spec/compiler/parser/parser_spec.cr

@@ -132,6 +132,7 @@ module Crystal
    it_parses "~ 1", Call.new(1.int32, "~")
    it_parses "1 && 2", And.new(1.int32, 2.int32)
    it_parses "1 || 2", Or.new(1.int32, 2.int32)
+    it_parses "&- 1", Call.new(1.int32, "&-")


Is support for &+ 1 left out intentionally?

No, I just added &- because there is a prefix operation -. I will add the specs.

I asked since AFAIR there's also prefix + operator.

Can prefix - or + overflow? If not, maybe it's not worth adding these.

@asterite - can on 2 complement as 0 is taken from the positive side. Dunno about systems with other bases.

Note that I am not planning on implementing prefix &-, but since unitary - is allowed, &- should also work. It is only used in an stdlib string operation that can be replaced by 0 &- expr

bcardiff · 2018-07-04T19:08:12Z

All review comments addressed

bcardiff · 2018-07-05T10:41:52Z

Ready for another review round

yxhuvud · 2018-07-05T11:44:15Z

@bcardiff Isn't it still a breaking change as the old operators will raise if overflow happen when they didn't before?

Ah, or no. This is just the parsing part. Never mind.

straight-shoota · 2018-07-05T12:12:07Z

spec/compiler/semantic/block_spec.cr

@@ -914,7 +914,7 @@ describe "Block inference" do
      )) { types["Moo"].types["Bar"] }
  end

-  it "passes &->f" do
+  it "passes &(->f)" do


These changes should no longer be necessary.

jkthorne · 2018-07-05T22:22:50Z

spec/compiler/normalize/op_assign_spec.cr

@@ -1,8 +1,10 @@
 require "../../spec_helper"

 describe "Normalize: op assign" do
-  it "normalizes var +=" do
-    assert_normalize "a = 1; a += 2", "a = 1\na = a + 2"
+  ["+", "-", "*", "&+", "&-", "&*"].each do |op|


seem like in the rest of this file each test case is broken out. I think it might make it easier to understand if they are different test cases.

Add &+ &- &* &** operators parsing

71f72f4

Assignment operators are supported for &+= &-= &*=

bcardiff added kind:feature topic:compiler:parser breaking-change labels Jul 4, 2018

jhass reviewed Jul 4, 2018

View reviewed changes

jhass approved these changes Jul 4, 2018

View reviewed changes

Update test description

12e0e54

straight-shoota reviewed Jul 4, 2018

View reviewed changes

Sija reviewed Jul 4, 2018

View reviewed changes

bcardiff added 3 commits July 4, 2018 18:59

Add lookahead to parse &->expr as &(->expr)

25cab7c

Allow &+ as prefix operator

ad0a3c8

Add formatter specs and rules

96db8fd

bcardiff removed the breaking-change label Jul 4, 2018

straight-shoota reviewed Jul 5, 2018

View reviewed changes

Rollback changes in semantic/block_spec.cr

d3b1c0d

jkthorne reviewed Jul 5, 2018

View reviewed changes

RX14 approved these changes Jul 7, 2018

View reviewed changes

RX14 modified the milestone: Next Jul 7, 2018

bcardiff merged commit 8d28cbb into crystal-lang:master Jul 9, 2018

bcardiff added this to the Next milestone Jul 9, 2018

RX14 modified the milestones: Next, 0.26.0 Jul 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add &+ &- &* &** operators parsing #6329

Add &+ &- &* &** operators parsing #6329

bcardiff commented Jul 4, 2018 •

edited

jhass Jul 4, 2018

RX14 commented Jul 4, 2018

bcardiff commented Jul 4, 2018

straight-shoota Jul 4, 2018

bcardiff Jul 4, 2018

straight-shoota Jul 4, 2018

asterite Jul 4, 2018 •

edited

asterite Jul 4, 2018

asterite Jul 4, 2018

bcardiff Jul 4, 2018

Sija Jul 4, 2018

bcardiff Jul 4, 2018

Sija Jul 4, 2018

asterite Jul 4, 2018

yxhuvud Jul 4, 2018 •

edited

bcardiff Jul 4, 2018

bcardiff commented Jul 4, 2018

bcardiff commented Jul 5, 2018

yxhuvud commented Jul 5, 2018 •

edited

straight-shoota Jul 5, 2018

jkthorne Jul 5, 2018 •

edited

Add &+ &- &* &** operators parsing #6329

Add &+ &- &* &** operators parsing #6329

Conversation

bcardiff commented Jul 4, 2018 • edited

Choose a reason for hiding this comment

RX14 commented Jul 4, 2018

bcardiff commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite Jul 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yxhuvud Jul 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardiff commented Jul 4, 2018

bcardiff commented Jul 5, 2018

yxhuvud commented Jul 5, 2018 • edited

Choose a reason for hiding this comment

jkthorne Jul 5, 2018 • edited

Choose a reason for hiding this comment

bcardiff commented Jul 4, 2018 •

edited

asterite Jul 4, 2018 •

edited

yxhuvud Jul 4, 2018 •

edited

yxhuvud commented Jul 5, 2018 •

edited

jkthorne Jul 5, 2018 •

edited