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

Improve variable type name request #14

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

adisonlampert
Copy link

@adisonlampert adisonlampert commented Mar 29, 2023

Description

ruby#790 fixes this issue. In this issue, the debugger raises the error:

#<ArgumentError: wrong number of arguments (given 0, expected 1)>

I took a look at the repository this error was occurring in to figure out what was causing it. Essentially, this ArgumentError is coming from the self.name function having the parameter value which is not provided when klass.name is called here, causing an exception.

In ruby#790, if there is an exception when trying to get the class name, the variable's type is set to "<Error: #{e.message} (#{e.backtrace.first}>".

My first commit adds three tests that test this functionality:

  1. A test overriding the name method. The type should be an error message that begins with <Error: wrong number of arguments (given 0, expected 1) .
  2. A test overriding the to_s method. The type should be the same error as 1.
    • In order to test this, I had to override the name method and return nil because the type_name method checks klass.name first and without overriding the method to return nil, the test does not trigger klass.to_s
  3. A test overriding the class method. This was added in because M_CLASS.bind_call(obj) is used rather than obj.class so adding a test to ensure the class name is being set appropriately and not raising an error is useful.

I noticed that rather than catching an exception and setting the type to an error message, it would be better to use M_NAME.bind_call(klass) to get the type name as this does not call klass.name so there would be no error. This is an approach that is used by Tapioca here.

In my second commit, I implemented this approach. I updated the tests from my first commit to reflect the new changes including removing the to_s test as this method is no longer being used. You can see the type is now Foo instead of an error message. This is more correct.

@adisonlampert adisonlampert changed the title Add test for #790 Improved variable type name request Mar 29, 2023
@adisonlampert adisonlampert requested a review from a team March 29, 2023 17:26
@adisonlampert adisonlampert changed the title Improved variable type name request Improve variable type name request Mar 29, 2023
@@ -14,6 +14,7 @@ module DEBUGGER__
M_RESPOND_TO_P = method(:respond_to?).unbind
M_METHOD = method(:method).unbind
M_OBJECT_ID = method(:object_id).unbind
M_NAME = Module.instance_method(:name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use the same pattern as above.

if name
{ name: name,
value: str,
type: type_name(obj),
type: type_name || type_name.to_s,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using this or, can't we always invoke to_s?

if name
{ name: name,
value: str,
type: type_name(obj),
type: type_name.to_s,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type_name should already return a String so to_s may not be necessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, everything appears to be working fine when I use just type_name. Does that make the to_s test unnecessary?

end
end

class DAPOverwrittenToSMethod < ProtocolTestCase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we don't call to_s anymore, do we still need this test?

Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it's good to open a PR upstream now. Just a reminder that the PR description should be concise but mention:

  • What's the behavioural difference with examples (e.g. parts of the response content)
  • Why it's better

@adisonlampert adisonlampert force-pushed the al/add-test-for-790 branch 2 times, most recently from d59e3ca to a136d09 Compare April 5, 2023 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack Trace When Stepping Through Functions
3 participants