Project

General

Profile

Feature #15751

Add FrozenError#receiver

Added by jeremyevans0 (Jeremy Evans) 3 months ago. Updated about 1 month ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:92165]

Description

Similar to NameError#receiver, this returns the object on which
the modification was attempted. This is useful as it can pinpoint
exactly what is frozen, as in many cases when a FrozenError is
raised, you cannot determine from the context which object is
frozen that you expect not to be.

I'm not sure that using name_err_receiver as the C function for
FrozenError#receiver is acceptable, but it doesn't appear to cause
problems.

Users of the current rb_error_frozen C function will have to switch
to using rb_error_frozen_object or the new rb_raise_frozen_error
in order to set the receiver of the FrozenError.

Patch to implement this feature is attached.


Files

Add-FrozenError-receiver.patch (6.48 KB) Add-FrozenError-receiver.patch jeremyevans0 (Jeremy Evans), 04/06/2019 07:05 AM
Add-FrozenError-receiver-v2.patch (6.66 KB) Add-FrozenError-receiver-v2.patch jeremyevans0 (Jeremy Evans), 04/06/2019 06:00 PM
Add-FrozenError-receiver-v3.patch (6.49 KB) Add-FrozenError-receiver-v3.patch jeremyevans0 (Jeremy Evans), 04/07/2019 03:36 AM

Associated revisions

Revision 39eadca7
Added by jeremyevans (Jeremy Evans) about 1 month ago

Add FrozenError#receiver

Similar to NameError#receiver, this returns the object on which
the modification was attempted. This is useful as it can pinpoint
exactly what is frozen. In many cases when a FrozenError is
raised, you cannot determine from the context which object is
frozen that you attempted to modify.

Users of the current rb_error_frozen C function will have to switch
to using rb_error_frozen_object or the new rb_frozen_error_raise
in order to set the receiver of the FrozenError.

To allow the receiver to be set from Ruby, support an optional
second argument to FrozenError#initialize.

Implements [Feature #15751]

History

Updated by nobu (Nobuyoshi Nakada) 3 months ago

  • Description updated (diff)

Seems reasonable, but shouldn't FrozenError#initialize use a keyword argument as well as NameError#initialize?

Updated by jeremyevans0 (Jeremy Evans) 3 months ago

nobu (Nobuyoshi Nakada) wrote:

Seems reasonable, but shouldn't FrozenError#initialize use a keyword argument as well as NameError#initialize?

NameError#initialize uses an additional positional argument for the name argument, and I figured that was easier as it is unlikely FrozenError#initialize would need to support additional arguments. However, I don't have strong feelings about it, so attached is a patch that switches to using a keyword argument.

Updated by Eregon (Benoit Daloze) 3 months ago

NameError#initialize uses a keyword argument for receiver, so I it makes some sense to be consistent.

OTOH, keyword arguments often imply extra processing, especially if the caller is not Ruby code, so I actually find the positional argument version simpler.
And indeed, I don't think extra arguments will be needed for FrozenError#initialize.

In the patch in rb_raise_frozen_error, exc is used for both the message and the exception object, could you use different variables?
I find it confusing to use the same and re-assign it when there is no need.

Updated by jeremyevans0 (Jeremy Evans) 3 months ago

Eregon (Benoit Daloze) wrote:

In the patch in rb_raise_frozen_error, exc is used for both the message and the exception object, could you use different variables?
I find it confusing to use the same and re-assign it when there is no need.

Sure, I can make that change. Can you and nobu come to an agreement on whether to use the positional argument or the keyword argument so I know which patch to update?

Updated by nobu (Nobuyoshi Nakada) 3 months ago

I haven't look C-API well, the additional argument would be ok.
rb_raise_frozen_error should be rb_frozen_error_raise like rb_name_err_raise?

As for the test, assert_same would be better for the equality of the receiver.

Updated by jeremyevans0 (Jeremy Evans) 3 months ago

nobu (Nobuyoshi Nakada) wrote:

I haven't look C-API well, the additional argument would be ok.
rb_raise_frozen_error should be rb_frozen_error_raise like rb_name_err_raise?

As for the test, assert_same would be better for the equality of the receiver.

OK, v3 of the patch is attached, based off the initial patch with the positional argument, with the function renamed to rb_frozen_error_raise and using assert_same in the tests.

Eregon (Benoit Daloze) wrote:

In the patch in rb_raise_frozen_error, exc is used for both the message and the exception object, could you use different variables?
I find it confusing to use the same and re-assign it when there is no need.

v3 patch includes this change as well.

Updated by Eregon (Benoit Daloze) 3 months ago

The patch looks good to me.
I will let nobu (Nobuyoshi Nakada) commit it.

A nice extra would be specs in spec/ruby/core/exception/frozen_error_spec.rb (similar to spec/ruby/core/exception/name_error_spec.rb) but this is not a requirement.

#8

Updated by jeremyevans (Jeremy Evans) about 1 month ago

  • Status changed from Open to Closed

Applied in changeset git|39eadca76b48fc7841da688f6745e40897ec37ff.


Add FrozenError#receiver

Similar to NameError#receiver, this returns the object on which
the modification was attempted. This is useful as it can pinpoint
exactly what is frozen. In many cases when a FrozenError is
raised, you cannot determine from the context which object is
frozen that you attempted to modify.

Users of the current rb_error_frozen C function will have to switch
to using rb_error_frozen_object or the new rb_frozen_error_raise
in order to set the receiver of the FrozenError.

To allow the receiver to be set from Ruby, support an optional
second argument to FrozenError#initialize.

Implements [Feature #15751]

Also available in: Atom PDF