Remove 1 item Expressions normalization #4861

makenowjust · 2017-08-20T13:31:07Z

This contains some MainVisitor fix to follow this change, and adds specs to spec/compiler/normalize/expressions_spec.cr, it's a new file.

@asterite

bew · 2017-08-20T13:50:20Z

spec/compiler/normalize/expressions_spec.cr

+  end
+
+  it "normalizes an expression" do
+    assert_normalize "(1 < 2).as(Bool)", "((1 < 2)).as(Bool)"


Why are there 2 pairs of parenthesis in the normalized version?

It is ASTNode#to_s bug, but it is not related to this PR, and I've fixed it in my local repository already. I'll open a new PR.

Fix crystal-lang#4861 (comment)

asterite · 2017-08-20T14:27:26Z

Hm, now I'm not convinced this change is good. true_literal? should only return true for a BoolLiteral that's true, not for an Expressions that just has a true BoolLiteral in it.

This needs a bit more thought. Maybe your first way of fixing this was less obtrusive.

makenowjust · 2017-08-20T14:40:39Z

@asterite I added it to support very rare case (and there is this case in samples/fannkuch-redux.cr:

while (true)
  # ...
end

But I think this case should not be supported now and while (true) should be replaced to whie true.

Sija · 2017-08-22T00:39:39Z

src/compiler/crystal/semantic/main_visitor.cr

@@ -1811,10 +1814,14 @@ module Crystal
        filter_vars cond_type_filters, &.not
      when Or
        # Try to apply boolean logic: `!(a || b)` is `!a && !b`
+        cond_left = cond.left


Could it be a good case for refactoring? Making a helper method along the lines of:

def single_expression(cond) if cond.is_a?(Expressions) && cond.expressions.size == 1 return cond[0] end end # then you could re-use it like: cond_left = single_expression(cond) || cond.left # and l8r on, around line 2116 when Expressions return unless cond = single_expression(node) return get_while_cond_assign_target(cond)

Good catch!

makenowjust · 2017-08-22T13:03:36Z

@asterite ping. What do you think now?

asterite · 2017-08-26T03:23:17Z

@makenowjust I won't have time for this

makenowjust · 2017-08-26T13:49:43Z

@asterite Sorry.

But I think #4849 should be fixed before next version at least, so I want review by other members. Anyone can review this?

straight-shoota · 2017-09-05T17:47:00Z

That would be great!

Sija · 2017-09-10T22:41:08Z

@RX14 @asterite @bcardiff 🏓

makenowjust · 2017-09-15T11:55:14Z

@RX14 @asterite @bcardiff 🏓 again!

asterite · 2017-09-15T12:45:46Z

@makenowjust Sorry, but right now I'm very confused, but I think the changes are good.

I think we can still let while (true) work. Search for:

endless_while = node.cond.true_literal?

We can probably change it to something like:

endless_while = node.cond.true_literal? || single_expression(node.cond).try &.true_literal?

(not tested)

makenowjust · 2017-09-16T04:05:44Z

@asterite I can't understand why while (true) should work. Do you have any reason?

asterite · 2017-09-16T12:01:23Z

@makenowjust Because it works now and it's better not to introduce a breaking change if the fix is very simple :-)

(and because in your code you are considering an Expressions with a single expression the same as that single expression, so we should do it in this case too)

Fix #4861 (comment)

makenowjust · 2017-09-20T04:56:57Z

@mverzilli Sorry. Please re-open this! This pull request should not be closed.

mverzilli · 2017-09-20T12:16:42Z

Ooops, my bad. I'll send a PR to revert, sorry for this.

Sija · 2017-09-20T12:26:11Z

@mverzilli why not just reopen this PR?

mverzilli · 2017-09-20T12:27:10Z

Yes, I can reopen but I must also revert this change because now it's merged to master

Sija · 2017-09-20T12:29:42Z

@mverzilli "it's merged to master", where?

mverzilli · 2017-09-20T12:31:08Z

? https://github.com/crystal-lang/crystal/commits/master

makenowjust · 2017-09-20T12:55:40Z

@Sija I don't know, however I can't reopen (perhaps GitHub cannot allow normal contributor to reopen the issue closed by project author.) 水曜日, 20 9月 2017, 09:26午後 +09:00 from Sijawusz Pur Rahnama notifications@github.com :

…

@mverzilli why not just reopen this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub , or mute the thread .

Sija · 2017-09-20T13:38:18Z

@mverzilli It would help if you could provide SHA of a commit which you believe is merged into the master.

asterite · 2017-09-20T13:44:28Z

This was never merged. It was incorrectly automatically closed because of #4862

mverzilli · 2017-09-20T13:46:56Z

Oh, that's weird! Now I understand why @Sija was asking that, sorry! Anyway, it's already reopened, sorry for the noise.

asterite · 2017-09-20T13:53:29Z

@mverzilli To be honest, I also thought this was merged when I got the "closed" mail 😅

bew · 2017-09-26T14:30:07Z

So is it ready for merging?

bcardiff · 2017-09-26T15:30:02Z

I think this need to be rebased first. I found a bit overcomplicated to follow with all the references across the issues 😅

Since the normalization for single expression is removed and the single_expression function extract the inner sole expression. Aren't we losing cases like ((true))? Knowing which spec break if the single_expression is not introduced would probably help here.

asterite · 2017-09-26T15:32:56Z

@bcardiff Correct! I think #single_expression should check if the inner expression is also a single expression, recursively.

Fix crystal-lang#4849 See crystal-lang#4852 (comment) This contains some MainVisitor fix to follow this change, and adds specs to `spec/compiler/normalize/expressions_spec.cr`, it's a new file.

And revert src/compiler/crystal/syntax/ast.cr changes

Follow crystal-lang#4862 fix

@asterite

And now `while (true)` works. Thank you @asterite.

makenowjust · 2017-09-27T13:01:38Z

@asterite @bcardiff Updated. What do you think?

bew reviewed Aug 20, 2017

View reviewed changes

makenowjust force-pushed the fix/crystal/remove-1-expression-normalize branch from 49e82e0 to 72d785a Compare August 20, 2017 14:09

makenowjust added a commit to makenowjust/crystal that referenced this pull request Aug 20, 2017

Remove unnecessary parenthesis from Cast#to_s result

b782966

Fix crystal-lang#4861 (comment)

makenowjust mentioned this pull request Aug 20, 2017

Remove unnecessary parenthesis from Cast#to_s result #4862

Merged

Sija reviewed Aug 22, 2017

View reviewed changes

makenowjust force-pushed the fix/crystal/remove-1-expression-normalize branch from 94886bb to d70d28b Compare August 22, 2017 06:38

mverzilli closed this in #4862 Sep 20, 2017

mverzilli pushed a commit that referenced this pull request Sep 20, 2017

Remove unnecessary parenthesis from Cast#to_s result

f5abb5b

Fix #4861 (comment)

makenowjust deleted the fix/crystal/remove-1-expression-normalize branch September 20, 2017 04:55

makenowjust restored the fix/crystal/remove-1-expression-normalize branch September 20, 2017 04:56

mverzilli reopened this Sep 20, 2017

asterite mentioned this pull request Sep 26, 2017

Improve #4708 #4852

Closed

makenowjust added 4 commits September 27, 2017 08:50

Remove 1 item Expressions normalization

7b1fd73

Fix crystal-lang#4849 See crystal-lang#4852 (comment) This contains some MainVisitor fix to follow this change, and adds specs to `spec/compiler/normalize/expressions_spec.cr`, it's a new file.

Replace 'while (true)' to 'while true'

4daee4d

And revert src/compiler/crystal/syntax/ast.cr changes

Refactor single_expression

c964a19

Remove unnecessary parenthesis

d27122e

Follow crystal-lang#4862 fix

makenowjust force-pushed the fix/crystal/remove-1-expression-normalize branch from d70d28b to a3d7e58 Compare September 27, 2017 00:50

Let 'single_expression' check single expression recursively

679b6d7

And now `while (true)` works. Thank you @asterite.

makenowjust force-pushed the fix/crystal/remove-1-expression-normalize branch from a3d7e58 to 679b6d7 Compare September 27, 2017 00:51

asterite approved these changes Sep 27, 2017

View reviewed changes

mverzilli added this to the Next milestone Sep 27, 2017

mverzilli approved these changes Sep 29, 2017

View reviewed changes

mverzilli merged commit 8c5fb21 into crystal-lang:master Sep 29, 2017

mverzilli added kind:feature topic:compiler labels Sep 29, 2017

makenowjust deleted the fix/crystal/remove-1-expression-normalize branch September 29, 2017 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove 1 item Expressions normalization #4861

Remove 1 item Expressions normalization #4861

makenowjust commented Aug 20, 2017

bew Aug 20, 2017

makenowjust Aug 20, 2017

asterite commented Aug 20, 2017

makenowjust commented Aug 20, 2017 •

edited

Loading

Sija Aug 22, 2017

makenowjust Aug 22, 2017

makenowjust commented Aug 22, 2017

asterite commented Aug 26, 2017

makenowjust commented Aug 26, 2017

straight-shoota commented Sep 5, 2017

Sija commented Sep 10, 2017

makenowjust commented Sep 15, 2017

asterite commented Sep 15, 2017

makenowjust commented Sep 16, 2017

asterite commented Sep 16, 2017

makenowjust commented Sep 20, 2017

mverzilli commented Sep 20, 2017

Sija commented Sep 20, 2017

mverzilli commented Sep 20, 2017

Sija commented Sep 20, 2017

mverzilli commented Sep 20, 2017

makenowjust commented Sep 20, 2017 via email

Sija commented Sep 20, 2017

asterite commented Sep 20, 2017

mverzilli commented Sep 20, 2017

asterite commented Sep 20, 2017

bew commented Sep 26, 2017

bcardiff commented Sep 26, 2017

asterite commented Sep 26, 2017

makenowjust commented Sep 27, 2017

Remove 1 item Expressions normalization #4861

Remove 1 item Expressions normalization #4861

Conversation

makenowjust commented Aug 20, 2017

bew Aug 20, 2017

Choose a reason for hiding this comment

makenowjust Aug 20, 2017

Choose a reason for hiding this comment

asterite commented Aug 20, 2017

makenowjust commented Aug 20, 2017 • edited Loading

Sija Aug 22, 2017

Choose a reason for hiding this comment

makenowjust Aug 22, 2017

Choose a reason for hiding this comment

makenowjust commented Aug 22, 2017

asterite commented Aug 26, 2017

makenowjust commented Aug 26, 2017

straight-shoota commented Sep 5, 2017

Sija commented Sep 10, 2017

makenowjust commented Sep 15, 2017

asterite commented Sep 15, 2017

makenowjust commented Sep 16, 2017

asterite commented Sep 16, 2017

makenowjust commented Sep 20, 2017

mverzilli commented Sep 20, 2017

Sija commented Sep 20, 2017

mverzilli commented Sep 20, 2017

Sija commented Sep 20, 2017

mverzilli commented Sep 20, 2017

makenowjust commented Sep 20, 2017 via email

Sija commented Sep 20, 2017

asterite commented Sep 20, 2017

mverzilli commented Sep 20, 2017

asterite commented Sep 20, 2017

bew commented Sep 26, 2017

bcardiff commented Sep 26, 2017

asterite commented Sep 26, 2017

makenowjust commented Sep 27, 2017

makenowjust commented Aug 20, 2017 •

edited

Loading