Project

General

Profile

Actions

Feature #16602

closed

Add support for `frozen_string_literals` to eval.

Added by ioquatix (Samuel Williams) about 4 years ago. Updated about 4 years ago.

Status:
Rejected
Assignee:
-
Target version:
-
[ruby-core:97034]

Description

Would it make sense for eval(..., frozen_string_literal: true) to exist?

Updated by ioquatix (Samuel Williams) about 4 years ago

Yes, it does, but it:

  • Requires modification of the string which is sometimes messy.
  • Requires modification of line numbers if you want to do it in a way which is not visible to the user.

I think any option that can be provided on the comment line at the start should also be an option to eval, and *_eval. e.g. coding too.

Updated by sawa (Tsuyoshi Sawada) about 4 years ago

ioquatix (Samuel Williams) wrote in #note-2:

Yes, it does, but it:

  • Requires modification of the string which is sometimes messy.

It requires modification because it is indeed modification: You are adding specification to a piece of code that did not have such specification.

I think any option that can be provided on the comment line at the start should also be an option to eval, and *_eval. e.g. coding too.

Once you start asking for such feature, it would be a matter of time that someone would ask for features like:

require "foo", frozen_string_literal: true
load "bar.rb", frozen_string_literal: true
$ ruby --frozen-string-literal=true ./baz.rb

Updated by Eregon (Benoit Daloze) about 4 years ago

ioquatix (Samuel Williams) wrote in #note-2:

Requires modification of the string which is sometimes messy.

How is it messy? Isn't it as simple as @byroot (Jean Boussier) showed?

coding too.

Just using the encoding of the String seems much better for eval.

Requires modification of line numbers if you want to do it in a way which is not visible to the user.

You can pass a line number to eval to make it look like there was no extra line.

I think what @byroot (Jean Boussier) says is sufficient.
The global command-line flag is a very different thing that probably doesn't work for many gems.

The way it's implemented currently is entirely consistent, the parser reads magic comments like # frozen_string_literal: true no matter where it's parsed from.

Also, I think adding # frozen_string_literal to code not adding it itself is a potentially breaking change, so I think it doesn't need to be super convenient.
Basically, it's back to the discussion of enabling frozen_string_literal by default.

Updated by shevegen (Robert A. Heiler) about 4 years ago

Basically, it's back to the discussion of enabling frozen_string_literal by default.

As far as I know it is not so much a discussion (matz decided to enable it after
all) - the primary question was more when it would become the default. I do not know
when it will be the case, but matz said that ruby 3.0 will not have it by default,
primarily due to backwards compatibility (which is simply a decision to be made
either way).

I have no real opinion on this feature suggested by ioquatix (personally I do not
need it; all my .rb files that I use on a daily basis have frozen strings set to
true anyway) but I think the discussion is a bit coming or going from different
point of views. One point may originate from convenience (I would think so
ioquatix); the other about that it is already possible as-is, via the example
shown by byroot, but also "downward" request that could follow, as shown by
saba - e. g. to add the same to more methods, even if the suggestion here did
not include it.

Again, I have no real preference either way, but the convenience aspect should
possibly be considered, whether there may be a net benefit or a net drawback
of the feature at hand. This may also require perhaps other folks to comment
if they were to need the suggested functionality; I can't speak for them as
I would not need the functionality (I almost never use eval() anymore; in
general I try to avoid plain eval(); only using instance_eval and
class_eval if any eval at all these days).

Updated by ioquatix (Samuel Williams) about 4 years ago

@sawa (Tsuyoshi Sawada) your argument convince me that we don't need this feature then. But it's not always the case that "You are adding specification to a piece of code that did not have such specification."

Following that we reject this feature, I'm at a loss how to fix https://github.com/rack/rack/pull/1544 elegantly.

User code may contain:

# frozen_string_literal: true

But we end up calling eval like this:

eval "Rack::Builder.new {\n" + builder_script + "\n}.to_app",
        TOPLEVEL_BINDING, file, 0

So we end up with a kludge of:

Rack::Builder.new {
# frozen_string_literal: true
...
}

Which obviously isn't going to work correctly. The only other solution I can think of is to try and match comments in the user code, extract them outside the block, etc. Messy.

Actions #7

Updated by ioquatix (Samuel Williams) about 4 years ago

  • Status changed from Open to Rejected

Updated by Eregon (Benoit Daloze) about 4 years ago

I think I found a good solution for that case in rack:
https://github.com/rack/rack/pull/1544#issuecomment-581123830

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0