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

Fix Reference#dup of file private class #3858

Conversation

makenowjust
Copy link
Contributor

For instance:

private class Foo
end

Foo.new.dup

this code causes an error:

Error in foo.cr:4: instantiating 'Foo#dup()'

Foo.new.dup
        ^~~

in /.../src/reference.cr:40: expanding macro

    {% if @type.abstract? %}
    ^

in macro 'macro_XXX' /.../src/reference.cr:40, line 2:

   1.
>  2.       dup = Foo.allocate
   3.       dup.as(Void*).copy_from(self.as(Void*), instance_sizeof(Foo))
   4.       dup
   5.

This commit fixes it by wrapping {{@type}} expansion with #<loc:> pragma
of class defined location.

For instance:

    private class Foo
    end

    Foo.new.dup

this code causes an error:

    Error in foo.cr:4: instantiating 'Foo#dup()'

    Foo.new.dup
            ^~~

    in /.../src/reference.cr:40: expanding macro

        {% if @type.abstract? %}
        ^

    in macro 'macro_XXX' /.../src/reference.cr:40, line 2:

       1.
    >  2.       dup = Foo.allocate
       3.       dup.as(Void*).copy_from(self.as(Void*), instance_sizeof(Foo))
       4.       dup
       5.

This commit fixes it by wrapping {{@type}} expansion with #<loc:> pragma
of class defined location.
@@ -140,7 +140,7 @@ struct Int

{% begin %}
if self < 0 && self == {{@type}}::MIN && other == -1
raise ArgumentError.new "overflow: {{@type}}::MIN / -1"
raise ArgumentError.new "overflow: {{@type.id}}::MIN / -1"
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change

@asterite asterite closed this Jan 11, 2017
@asterite
Copy link
Member

I closed this because this introduces breaking changes. I believe a better solution is to improve macros. Maybe macro interpolation {{...}} shouldn't work inside string literals, and then we could always emit loc pragmas.

A specific solution for dup might be to mention {%@type%} (just so that the compiler knows it's a macro def) and then do self.class.allocate

@makenowjust
Copy link
Contributor Author

@asterite Macros in comment is disallowed, isn't it? If we can disallow macro in string literal, I think we can decide whether to output loc pragama on context, it is better.

@ysbaddaden
Copy link
Contributor

Macros are allowed everywhere, including within comments.

@makenowjust
Copy link
Contributor Author

But if loc pragma is emitted always, your comment will be so dirty.

@ysbaddaden
Copy link
Contributor

Well, it renders properly in Crystal docs.

@asterite
Copy link
Member

That's why I think the macros system should be improved, because when you pass nodes to them there's no easy way to preserve their location. And blindly outputting loc pragmas isn't the correct solution (for the yield inside macros this can be safely done because you usually yield a block of code and you wouldn't yield inside a string interpolation or other strange places)

@makenowjust makenowjust deleted the fix/crystal/private-type-lookup branch January 12, 2017 01:04
asterite pushed a commit that referenced this pull request Jan 13, 2017
@makenowjust makenowjust restored the fix/crystal/private-type-lookup branch January 31, 2017 06:31
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