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

Remove 1 item Expressions normalization #4861

Conversation

makenowjust
Copy link
Contributor

Fix #4849
See #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.

@asterite

end

it "normalizes an expression" do
assert_normalize "(1 < 2).as(Bool)", "((1 < 2)).as(Bool)"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@makenowjust 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
@asterite
Copy link
Member

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
Copy link
Contributor Author

makenowjust commented Aug 20, 2017

@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.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

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

@asterite ping. What do you think now?

@asterite
Copy link
Member

@makenowjust I won't have time for this

@makenowjust
Copy link
Contributor Author

@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
Copy link
Member

That would be great!

@Sija
Copy link
Contributor

Sija commented Sep 10, 2017

@RX14 @asterite @bcardiff 🏓

@makenowjust
Copy link
Contributor Author

@RX14 @asterite @bcardiff 🏓 again!

@asterite
Copy link
Member

@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
Copy link
Contributor Author

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

@asterite
Copy link
Member

@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)

@makenowjust makenowjust deleted the fix/crystal/remove-1-expression-normalize branch September 20, 2017 04:55
@makenowjust makenowjust restored the fix/crystal/remove-1-expression-normalize branch September 20, 2017 04:56
@makenowjust
Copy link
Contributor Author

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

@mverzilli
Copy link

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

@Sija
Copy link
Contributor

Sija commented Sep 20, 2017

@mverzilli why not just reopen this PR?

@mverzilli
Copy link

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

@mverzilli mverzilli reopened this Sep 20, 2017
@Sija
Copy link
Contributor

Sija commented Sep 20, 2017

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

@mverzilli
Copy link

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

@makenowjust
Copy link
Contributor Author

makenowjust commented Sep 20, 2017 via email

@Sija
Copy link
Contributor

Sija commented Sep 20, 2017

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

@asterite
Copy link
Member

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

@mverzilli
Copy link

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

@asterite
Copy link
Member

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

@bew
Copy link
Contributor

bew commented Sep 26, 2017

So is it ready for merging?

@bcardiff
Copy link
Member

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
Copy link
Member

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

@asterite asterite mentioned this pull request Sep 26, 2017
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
@makenowjust makenowjust force-pushed the fix/crystal/remove-1-expression-normalize branch from d70d28b to a3d7e58 Compare September 27, 2017 00:50
@makenowjust makenowjust force-pushed the fix/crystal/remove-1-expression-normalize branch from a3d7e58 to 679b6d7 Compare September 27, 2017 00:51
@makenowjust
Copy link
Contributor Author

@asterite @bcardiff Updated. What do you think?

@mverzilli mverzilli added this to the Next milestone Sep 27, 2017
@mverzilli mverzilli merged commit 8c5fb21 into crystal-lang:master Sep 29, 2017
@makenowjust makenowjust deleted the fix/crystal/remove-1-expression-normalize branch September 29, 2017 14:48
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.

BUG: Significant paranthesis removed in macro interpolation (introduced by #4708)
7 participants