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

Indirect checking for return_type through an extra function #26836

Merged
merged 1 commit into from
Sep 1, 2018

Conversation

Keno
Copy link
Member

@Keno Keno commented Apr 17, 2018

Compiler.return_type is a bit of an odd beast because we basically treat
it like a built-in, but it's defined in the compiler. When working on
inference, it can be useful to load a second copy of inference with
the new changes (e.g. to test changes before the changes are capable
of running correctly on all input). This works quite well, but was
causing problems with return type, because Base's notion of
return_type and the second inference copy's notion of return_type
were different. This patch adds a simple is_return_type(f) predicate
that we can overload in our second inference copy in order to make it
recognize both Core.Compiler.return_type and return_type (from
the second copy's perspective as the return_type builtin).

@jrevels this fixes that weird non-inference of matvec we saw yesterday

@vtjnash
Copy link
Member

vtjnash commented Apr 17, 2018

it can be useful to load a second copy of inference with
the new changes

It's definitely wrong to treat a second copy of Core.Compiler.return_type as being related in any way to the primary copy. I suppose that's just a question of whether we care that this is UB though.

But, also, this won't (shouldn't?) do anything, since you can't alter the type-infer method table at runtime.

@Keno
Copy link
Member Author

Keno commented Apr 17, 2018

It's definitely wrong to treat a second copy of Core.Compiler.return_type as being related in any way to the primary copy. I suppose that's just a question of whether we care that this is UB though.

I don't care if it's wrong as long as it works and is useful ;). Also, I would question your "related in any way" assertion. After all, they implement the same semantics.

But, also, this won't (shouldn't?) do anything, since you can't alter the type-infer method table at runtime.

I'm not altering the type-infer method table, I'm loading a second copy of inference where is_return_type is defined differently.

Compiler.return_type is a bit of an odd beast because we basically treat
it like a built-in, but it's defined in the compiler. When working on
inference, it can be useful to load a second copy of inference with
the new changes (e.g. to test changes before the changes are capable
of running correctly on all input). This works quite well, but was
causing problems with return type, because Base's notion of
return_type and the second inference copy's notion of return_type
were different. This patch adds a simple is_return_type(f) predicate
that we can overload in our second inference copy in order to make it
recognize both Core.Compiler.return_type and return_type (from
the second copy's perspective as the return_type builtin).
@Keno Keno force-pushed the kf/isreturntype branch from 746d5df to d650a3a Compare August 30, 2018 21:21
@Keno
Copy link
Member Author

Keno commented Aug 30, 2018

Needed this again. I rebased this. Since it doesn't affect the regular use case, but is very useful for my development purposes, I plan to merge this once CI goes through.

@Keno Keno merged commit 226a4ec into master Sep 1, 2018
@martinholters martinholters deleted the kf/isreturntype branch September 2, 2018 09:32
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.

2 participants