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.

Sign up for GitHub

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

Jump to bottom

Inference: more thorough UnionAll renaming in apply_type_tfunc #48662

Merged
merged 1 commit into from
Feb 14, 2023
Merged

Inference: more thorough UnionAll renaming in apply_type_tfunc #48662

merged 1 commit into from
Feb 14, 2023

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented Feb 13, 2023

Should fix the "An unreachable instruction was executed" error in recent PkgEval.
MWE has been added to test.

@N5N3 N5N3 requested a review from aviatesk February 13, 2023 06:04
@aviatesk
Copy link
Sponsor Member

Should fix #48631?

aviatesk
aviatesk approved these changes Feb 14, 2023
View reviewed changes
Copy link
Sponsor Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

LGTM.

@N5N3
Copy link
Member Author

N5N3 commented Feb 14, 2023 •
edited
Loading

I didn't test the bound error locally. But I guess it won't be fixed by this PR as it happened in type_more_complex
Edit: Well, the MWE there works correctly. I think we can close #48631 if PkgEval prove that.

@N5N3 N5N3 merged commit cd955e1 into JuliaLang:master Feb 14, 2023
@N5N3 N5N3 deleted the apply_type_tfunc branch February 14, 2023 07:23
@gbaraldi
Copy link
Member

@maleadt s original MWE still fails locally for me. At least on macos on the latest master, so not sure if we can close it.

vtjnash
vtjnash reviewed Feb 14, 2023
View reviewed changes
base/compiler/tfuncs.jl
if contains_is(outervars, tai.var)
ai = rename_unionall(ai)
unw = unwrap_unionall(ai)
break
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Suggested change
break

?

Copy link
Member Author

@N5N3 N5N3 Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

IIUC, rename_unionall replaces all typevars in ai thus there's no need to re-rename?

Copy link
Sponsor Member

@vtjnash vtjnash Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, okay. That function is a bit awkward then, since it also exists in C without that behavior, and this is the only usage of the function anywhere (including not existing in any tests) from 93a725d

@N5N3
Copy link
Member Author

N5N3 commented Feb 15, 2023

At least on macos on the latest master, so not sure if we can close it.

My fault. Looks like one of my local subtype/intersect patch silented the error. Still need some effort to find the remaining issue.

@N5N3 N5N3 mentioned this pull request Feb 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

@aviatesk aviatesk aviatesk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@N5N3 @aviatesk @gbaraldi @vtjnash