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

Interpreter: fix 12351 #12374

Merged
merged 3 commits into from
Aug 11, 2022
Merged

Interpreter: fix 12351 #12374

merged 3 commits into from
Aug 11, 2022

Conversation

asterite
Copy link
Member

Fixes #12351

@asterite asterite added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:interpreter labels Aug 11, 2022
@asterite
Copy link
Member Author

It would be great to merge #12355 so that we can easily see if this caused any regressions :-)

Comment on lines 182 to 183
# Make sure to always cast the argument to the target def arg's type
cast = Cast.new(var, TypeNode.new(target_def_arg.type))
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering, could we skip this cast if arg type and target type are identical?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true!

I'm now thinking that we only need the cast in the last if, where there's no restriction on the argument. Because otherwise the arg's type is always checked. Let me try it...

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

Copy link
Member Author

Choose a reason for hiding this comment

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

To expand a bit more on this: a multidispatch will generate code like this:

def multidispatch(arg)
  if arg.is_a?(String)
    foo(arg)
  elsif arg.is_a?(Int32)
    foo(arg)
  else
    foo(arg)
  end
end

In this case if arg's type is String | Int32 | Nil the last branch will have arg's type to be Nil. But, the method corresponding to foo might either take Nil or maybe String | Nil, or even String | Int32 | Nil depending on how the overloads are defined.

To make the cast conditionally on the last branch we'd have to know what the type of arg is there: removing any types that were already checked in .is_a?. Given that this is an optimization, I'd like to leave it for later or maybe not even implement it later (at most we just have one Cast node that does nothing)

@straight-shoota straight-shoota added this to the 1.6.0 milestone Aug 11, 2022
@asterite
Copy link
Member Author

Is it okay to merge this even if the Windows build is failing?

@straight-shoota
Copy link
Member

Yeah, that's probably just a hiccup.

@straight-shoota straight-shoota merged commit 273e82d into master Aug 11, 2022
@straight-shoota straight-shoota deleted the interpreter/fix-12351 branch August 11, 2022 18:26
@cyangle
Copy link
Contributor

cyangle commented Aug 11, 2022

It seems like this PR introduced a bug. The interpreter failed to run the std specs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:interpreter
Projects
None yet
3 participants