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

Macro: Parsing string fail when #{{...}} is inside the string #5287

Open
anykeyh opened this issue Nov 13, 2017 · 7 comments · May be fixed by #5288
Open

Macro: Parsing string fail when #{{...}} is inside the string #5287

anykeyh opened this issue Nov 13, 2017 · 7 comments · May be fixed by #5288

Comments

@anykeyh
Copy link

anykeyh commented Nov 13, 2017

Parser got wrong while parsing macro with string

macro do_something(a, b)
  "{{a.id}}#{{b.id}}"
end

puts do_something("1", "2") #Expected: 1#2

This will fail to compile, as macro will ignore #{{b.id}}

However I'm not sure I'm doing it right. I've tried with different position of backslashes \ to escaping it, but nothing is working.

@anykeyh anykeyh changed the title Parsing string with #{} in macro Macro: Parsing string fail when #{{...}} is inside the string Nov 13, 2017
@maiha
Copy link
Contributor

maiha commented Nov 13, 2017

@anykeyh
It would be fixed in next release. As a workaround you can use this.

  "%s#%s" % [{{a.id}}, {{b.id}}]

@makenowjust
Copy link
Contributor

Better workaround for performance:

macro do_something(a, b)
  "#{{{a.id}}}##{{{b.id}}}"
end

puts do_something("1", "2") # puts "1#2"

@bew
Copy link
Contributor

bew commented Nov 13, 2017

Better workaround for performance again! (nothing at runtime):

macro do_something(a, b)
  "{{a.id}}{{'#'.id}}{{b.id}}"
end

puts do_something "1", "2" # => "1#2"

makenowjust added a commit to makenowjust/crystal that referenced this issue Nov 13, 2017
Fixed crystal-lang#5287

Now `"\#{{ ... }}"` is parsed as macro expreession `{{ ... }}` wrapped with
macro literal `"\#` and `"`.
And this commit also fixed `"\\#{{ ... }}"` to parse it as only macro
literal `"\\#{{ ... }}"`.
@makenowjust makenowjust linked a pull request Nov 13, 2017 that will close this issue
@straight-shoota
Copy link
Member

Having stumbled across this issue, I find it only intuitive that "#{{foo}}" is interpreted as a macro expansion prefixed by # and not string interpolation of a tuple. Macro expressions are on a higher level than other code so they should have precedence.

So #5288 is IMO wrong as it provides a workaround for what should be valid by default.

@anykeyh anykeyh closed this as completed Jul 6, 2018
@straight-shoota
Copy link
Member

Why is this closed? It's still a valid issue.

@anykeyh anykeyh reopened this Jul 8, 2018
@anykeyh
Copy link
Author

anykeyh commented Jul 8, 2018

Sorry, mistake of mine, I thought it has been fixed long time ago 👎

@straight-shoota
Copy link
Member

There is another solution: We could completely disable macro expansion inside literals. I don't think that is a necessary feature. There doesn't even seem to be a spec for that, so maybe it's something that just happens to be but nobody really thought about it?

The thing is: There are currently two types of expansions in string literals: string interpolation (#{}) and macro expansion ({{}}). The latter is rarely talked about and probably many developers are not aware of it. It is still applied in some occasions in stdlib and probably also in shards.

If macro expansions were removed from literals, it would make the language a bit simpler. They can easily be replaced by interpolation, either as a macro or at runtime.

# macro expansion
"foo {{ bar.id }}"
# macro string interpolation
{{ "foo #{bar.id}" }}
# runtime interpolation
"foo #{ {{ bar }} }"

The latter has a small performance impact because the string is composed at runtime. But that can be mitigated by using a macro string interpolation. Apart from that, all three variants should be equivalent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants