Update parser.rb for ruby 3.4 warning

[chaadow]

No description provided.

[boazsegev]

This change results in a copy of the underlying String data, whereas the original code merely changed the encoding (overwriting a few bytes in the String object data) without copying any of the bytes.

This degrades performance.

What was the warning that this commit is attempting to solve?

Thanks,
B.

[chaadow]

Any force_encoding in the code must be changed somehow because force_encoding mutates the string

here i the warning:

/app/vendor/bundle/ruby/3.4.0/gems/combine_pdf-1.0.29/lib/combine_pdf/parser.rb:727: warning: string returned by :__WKANCHOR_4.to_s will be frozen in the future 

@boazsegev

[boazsegev]

Thanks!

Seems related to this:

https://github.com/ruby/ruby/blob/c6dbb10b7408cab17f170f0b23d1bbf0db03ad55/doc/NEWS/NEWS-3.4.0.md?plain=1#L139-L144

...which indicates it might relate to a specific path (or the code Encoding::ASCII_8BIT), allowing us to minimize String copying to that specific instance.

Also, __WKANCHOR_4 is a funky symbol. I think this is worth further investigation before we abandon performance considerations.

What do you think?

B.

[chaadow]

Unfortunately I do not have the entire stack on sentry because i only send the first line in backtrace. If I reproduce this locally i can tell you which line led to this warning!

I've seen a lot of force_encoding inside the parser.rb file, so yeah I think a review of the entire file and ( the gem ) as well can be benefitial @boazsegev

[chaadow]

@boazsegev what do you think of this instead?

[chaadow]

@boazsegev Happy new year!

I've tested this (by using +) in production and it eliminated all ruby warnings. What are your thoughts?

[boazsegev]

I'm not a fan of the +, as it forces Ruby to duplicate the string and degrades performance.

Also, it seems a better approach might be to add a fix in unify_string rather than every time it's called.

I'll try to come up with a solution as well.

[chaadow]

I've seen the solution which is to duplicate if the string passed is frozen

but it won't work in a setting where all strings are considered frozen, ( i mean right now they are not, but they will be)

so your solution will probably work in the future, but right now it will still emit warnings :/

[chaadow]

@boazsegev i know you've cut a release recently, so I've decided to use on production to check if it fixes all issues, but i still got some ruby warnings like here:

/app/vendor/bundle/ruby/3.4.0/gems/combine_pdf-1.0.31/lib/combine_pdf/parser.rb:730: warning: string returned by :__WKANCHOR_6.to_s will be frozen in the future

or

/app/vendor/bundle/ruby/3.4.0/gems/combine_pdf-1.0.31/lib/combine_pdf/parser.rb:730: warning: string returned by :__WKANCHOR_4.to_s will be frozen in the future