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: parse var as Call evan if var is declared in call args #4114

Merged

Conversation

makenowjust
Copy link
Contributor

Fixed #4106

@makenowjust makenowjust force-pushed the fix/crystal/class-var-scope branch from 682dbed to bcabb8f Compare March 4, 2017 13:35
@makenowjust
Copy link
Contributor Author

This change is useful in following case for example:

class Foo
  class_property foo : String = "hello"

  p foo
end

Currently this file causes compile error: read before assignment to local variable 'foo'. But, this behavior looks like a bug.

@@ -3687,10 +3688,12 @@ module Crystal
node
end

def preserve_stop_on_do(new_value = false)
def preserve_stop_on_do(new_value = false, call_args? = true)
Copy link
Member

Choose a reason for hiding this comment

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

Could you use is_call_args instead? call_args? as a variable name should be a parse error (at least it's not allowed in Ruby, and in Crystal it looks weird). If we don't change it now we'll have to change it later when we fix the parser for this, but I don't want to encourage this names.

old_stop_on_do = @stop_on_do
@stop_on_do = new_value
@call_args_nest += 1 if call_args?
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call_args_nest should just be incremented and decremented in the methods parse_call_args and parse_call_args_space_consumed? I don't think this is the right place to do it.

@asterite
Copy link
Member

asterite commented Mar 4, 2017

Thank you for this!

I think this change is OK. It's a breaking change, because if you have code similar to the spec you added:

macro debug_init(x)
  puts "initializing {{x.var}} to {{x.value}}"
  {{x}}
end

debug_init a : Int32 = 0
a

it will stop working (now it works). But I don't think that's a good usage for a macro, and in general we use var : Type to eventually declare the type of instance/class vars, never local vars.

I made a few comments, after that I'll review it again and probably merge this.

@makenowjust
Copy link
Contributor Author

@asterite Thanks for reviewing. I fixed to incr/decr @call_args_nest inside of parse_call_args and parse_call_args_space_consumed.

@asterite asterite merged commit f218af6 into crystal-lang:master Mar 4, 2017
@asterite
Copy link
Member

asterite commented Mar 4, 2017

@makenowjust Thank you! 💚

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.

3 participants