Skip to content

inference: properly compare vararg-tuple PartialStructs #44966

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

Merged
merged 2 commits into from
Apr 14, 2022
Merged

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Apr 13, 2022

Within the issimplertype query.
Fix #44965.


Unrelated but another commit also adds
one more missing Vararg handling
found from a code review.

Just found this case from code review, but this `Vararg` should be widened.
@aviatesk aviatesk requested a review from vtjnash April 13, 2022 07:25
@aviatesk aviatesk added the backport 1.8 Change should be backported to release-1.8 label Apr 13, 2022
@aviatesk aviatesk changed the title inference: properly compare vararg-tuple PartialStructs inference: properly compare vararg-tuple PartialStructs Apr 13, 2022
@@ -313,7 +313,7 @@ function issimplertype(@nospecialize(typea), @nospecialize(typeb))
if typea isa PartialStruct
aty = widenconst(typea)
for i = 1:length(typea.fields)
ai = typea.fields[i]
ai = unwrapva(typea.fields[i])
Copy link
Member

Choose a reason for hiding this comment

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

is Vararg{T,N} simpler-than T? I think so, per the comment in __limit_type_size, but just wanted to confirm

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the main reason that justifies this change is that ai is finally compared with fieldtype(aty, i)/getfield_tfunc(typeb, Const(i)), which essentially unwraps Vararg{T} to T.

Copy link
Member

Choose a reason for hiding this comment

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

This function is not symmetric in the arguments, so that reasoning does not justify it

@vtjnash
Copy link
Member

vtjnash commented Apr 14, 2022

LibGit2 seems to have run into some internal memory corruption problems as commonly happens, but doesn't seem like anything is related to this PR

@vtjnash vtjnash merged commit 6319b3a into master Apr 14, 2022
@vtjnash vtjnash deleted the avi/44965 branch April 14, 2022 20:07
@aviatesk aviatesk mentioned this pull request Apr 15, 2022
67 tasks
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label May 26, 2022
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.

Julia (1.8.0-beta3) compiler errors when I evaluate a model using MLJ package.
3 participants