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

Unfreeze string literals for ParseNode#inspect #1016

Merged
merged 1 commit into from Aug 6, 2019

Conversation

@jalyna
Copy link

jalyna commented Aug 6, 2019

Since newrelic is using the #inspect method on ParseNode the ruby code crashes due to frozen strings in inspect, which are recursively called with gsub! causing this error:

Screen Shot 2019-08-06 at 10 54 08

I added a regression test that fails with the current code.

@jalyna jalyna force-pushed the jalyna:fix-frozen-string-literals-on-inspect branch from 1ea26c4 to bc88181 Aug 6, 2019
@amatsuda

This comment has been minimized.

Copy link
Member

amatsuda commented Aug 6, 2019

@jalyna Thank you for the patch!
I understand that we need the first children.each_with_object(''.dup) modification, but I don't see why we need the second .dup call at the end of that line.

@jalyna

This comment has been minimized.

Copy link
Author

jalyna commented Aug 6, 2019

@amatsuda this is a really good question! the second .dup is still needed because %Q[] creates a frozen string literal (still). The test will fail if it gets removed.

# frozen_string_literal: true

puts %Q[hello world].frozen?

prints true.

We could also use something else here 🤔

@amatsuda

This comment has been minimized.

Copy link
Member

amatsuda commented Aug 6, 2019

@jalyna Ah! I just got what you meant by "which are recursively called". Thank you for a clear explanation (and I'm sorry that I wasn't carefully reading it...)!

@amatsuda amatsuda merged commit 4dbb180 into haml:master Aug 6, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@amatsuda

This comment has been minimized.

Copy link
Member

amatsuda commented Aug 6, 2019

@jalyna Just published version 5.1.2 gem with this patch. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.