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

Partial revert "Compiler: refactor and slightly optimize merging two types (#12436)" #12709

Merged
merged 3 commits into from
Nov 2, 2022

Conversation

caspiano
Copy link
Contributor

@caspiano caspiano commented Nov 1, 2022

This PR reverts commit bfc33db.

I haven't been able to extract a minimal reproduction of the error discussed on the forum, but I'll work on one.

I'll also review the individual commits made in #12436 with the failing code to see if we can keep any of the optimisations.

@caspiano caspiano added kind:bug kind:regression Something that used to correctly work but no longer works labels Nov 1, 2022
@caspiano
Copy link
Contributor Author

caspiano commented Nov 1, 2022

This commit in the original PR introduced the regression

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Looks good!

Ideally we'd have a small test that shows what breaks with this change, but I understand it can be hard to do that sometimes.

@caspiano
Copy link
Contributor Author

caspiano commented Nov 1, 2022

Honestly, struggling to come up with a minimal reproduction.
This is the line that causes the issue, crashing on request_value called on the in prepare_call_args_non_external.

This is the method called in the block, #debug(module_id : String, &callback : (String) -> Nil) : Nil. Which looks like it is casting the return type of Channel#send(value : T) : self to Nil

I'll take a look at the different return values of type_merge against the problematic code before and after this PR for further insight and hopefully a test case.

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

Thanks @caspiano ! That was fast!

@beta-ziliani beta-ziliani added this to the 1.6.2 milestone Nov 1, 2022
@straight-shoota straight-shoota changed the title Revert #12436 due to induced regression Partial revert " Compiler: refactor and slightly optimize merging two types (#12436)" Nov 2, 2022
@straight-shoota straight-shoota changed the title Partial revert " Compiler: refactor and slightly optimize merging two types (#12436)" Partial revert "Compiler: refactor and slightly optimize merging two types (#12436)" Nov 2, 2022
@straight-shoota straight-shoota merged commit 66d4244 into crystal-lang:master Nov 2, 2022
beta-ziliani pushed a commit that referenced this pull request Nov 3, 2022
…types (#12436)" (#12709)

This reverts part of commit bfc33db

The reverted part is 8d19346 (Generalize the case of a non-union type merged with a union type) from #12436
beta-ziliani pushed a commit that referenced this pull request Nov 3, 2022
…types (#12436)" (#12709)

This reverts part of commit bfc33db

The reverted part is 8d19346 (Generalize the case of a non-union type merged with a union type) from #12436
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug kind:regression Something that used to correctly work but no longer works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants