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

Reset location after block yielding in macro expansion #3751

Merged
merged 3 commits into from Dec 23, 2016

Conversation

makenowjust
Copy link
Contributor

@makenowjust makenowjust commented Dec 22, 2016

This pull request introduces #<loc:save> and #<loc:reset> new pragma comment, #<loc:save> saves current location and #<loc:reset> resets it. They are useful to keep original location when block yielding in macro expansion.

For example, this code makes broken error message:

macro foo
  a = {{ yield }}
  a.ok
end

foo { 1 }

It cannot be compiled and error message is:

in foo.cr:7: undefined method 'ok' for Int32

Error message points line 7 of foo.cr, but this code consists only 6 lines.

This bug comes from a location is not reset after block yielding in macro expansion. In fact, foo { 1 } expands to:

a =  begin #<loc:"foo.cr",6,7>1 end
a.ok

So, the parser reports a.ok is on line 7 of foo.cr.

This pull request also fixes it by wrapping #<loc:save> and #<loc:reeset> to yielding expanded code, such a like:

a =  #<loc:save>begin #<loc:"foo.cr",6,7>1 end#<loc:reset>
a.ok

It works well.

Introduce #<loc:save> and #<loc:reset> new pragma comment, #<loc:save>
saves current location and #<loc:reset> resets it. They are useful to
keep original location when block yielding in macro expansion.

For example, this code makes broken error message:

    macro foo
      a = {{ yield }}
      a.ok
    end

    foo { 1 }

It cannot be compiled and error message is:

    in foo.cr:7: undefined method 'ok' for Int32

Error message points line 7 of foo.cr, but this code consists only 6 lines.

This bug comes from a location is not reset after block yielding in
macro expansion. For instance, `foo { 1 }` expands to:

    a =  begin #<loc:"foo.cr",6,7>1 end
    a.ok

So, the parser reports `a.ok` is on line 7 of foo.cr.

This commit fixes it by wrapping #<loc:save> and #<loc:reeset> to
yielding expanded code, such a like:

    a =  #<loc:save>begin #<loc:"foo.cr",6,7>1 end#<loc:reset>
    a.ok
@asterite
Copy link
Member

Super interesting! I actually had this idea too and eventually was going to implement it, though I imagined something more like a stack (push/pop). What do you think if we change it to #<loc:push> and #<loc:pop> and use a stack in the lexer? (I'd use a record to store all the info you are remembering now)

@makenowjust
Copy link
Contributor Author

@asterite I'll try to implement #<loc:push> and #<loc:pop>. But, we cannot use record to remember old location. Because it is not immutable, it should be changed by lexer to report correct location.

@asterite
Copy link
Member

Hmm... then I misunderstood something. I thought loc:save just saved the current location, then loc:reset reseted it. But it seems when you do loc:save then some tracking is done in many places... why is this needed?

@makenowjust
Copy link
Contributor Author

For example, when given block is multiple lines:

macro foo
  a = {{ yield }}
  a.ok
end

foo do
  :foo
  :bar
end

It is expands to:

  a =  #<loc:save>begin #<loc:"foo.cr",7,3>:foo
#<loc:"foo.cr",8,3>:bar
end#<loc:reset>
a.ok

If #<loc:save> makes just copied and copied location's line number is not incremented, its location is wrong on #<loc:reset>.

@asterite
Copy link
Member

Oooh... I understand now. Yes, the lexer needs to continue incrementing lines for the old line number. I think like you implemented it it's fine :-)

@makenowjust
Copy link
Contributor Author

Thanks. I already implemented #<loc:push> and #<loc:pop> version, and now I run make spec on my environment. I'll write specs for #<loc:push> and #<loc:pop> (it is missing..), then I'll push it.

@asterite
Copy link
Member

@makenowjust Great! Though now I'm not sure there's a use case for push/pop. I thought it was going to be useful to implement/fix this, but I'm not sure anymore. Can you think of a use case?

@makenowjust
Copy link
Contributor Author

makenowjust commented Dec 22, 2016

@asterite No, so I didn't implement location stack at first. However, I think #<loc:save> and #<loc:reset> is not good naming. Perhaps #<loc:push> and #<loc:pop> is better even if stack size is 1 only.

@asterite asterite added this to the 0.20.2 milestone Dec 23, 2016
@asterite
Copy link
Member

@makenowjust Thank you! And thank you for extracting a method for incrementing line numbers. I was actually going to ask if you wanted to do that, so it seems you somehow read my mind... :-P

@asterite asterite merged commit 39ffa70 into crystal-lang:master Dec 23, 2016
@makenowjust makenowjust deleted the fix/macro-yield-location branch December 23, 2016 12:57
makenowjust added a commit to makenowjust/crystal that referenced this pull request Dec 23, 2016
It is fixed in crystal-lang#3751. Yeah, we need no hack for it!

This reverts commit 1d26d45.
makenowjust added a commit to makenowjust/crystal that referenced this pull request Dec 27, 2016
It is fixed in crystal-lang#3751. Yeah, we need no hack for it!

This reverts commit 1d26d45.
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

2 participants