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

Allow Module as type parameters #47749

Merged
merged 1 commit into from
Dec 2, 2022
Merged

Allow Module as type parameters #47749

merged 1 commit into from
Dec 2, 2022

Conversation

Keno
Copy link
Member

@Keno Keno commented Nov 30, 2022

The intended use case for this is generated functions that want to generate some reference to a module-specific generic function. The current solution is to duplicate the generated function into every module (probably using a package-provided macro) or to have some sort of registry system in the package providing the generated function. Both of these seem a bit ugly and I don't think there's any particularly good reason not to allow Modules to be type parameters. Admittedly, modules are not part of the scope contemplated by #33387 as they are mutable, but I think the mental model of modules is that they're immutable references to a namespace and what's actually mutable is the namespace itself (i.e. people wouldn't expect two modules that happen to have the same content be ==). This makes me think it still fits the mental model.

@Keno Keno requested a review from JeffBezanson November 30, 2022 09:47
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

SGTM. Can you add a hash field (or otherwise make the hash something stable like hash(m->parent, hash(m->name, hash)) so that the type caches don't get corrupted when we load incremental files.

If I am reading the code correctly, there is a bit of discrepancy about where you added it in tfuncs (equivalent to is_nestable_type_param) vs builtins.c (approximately valid_tparam)

@oscardssmith oscardssmith self-assigned this Nov 30, 2022
base/hashing.jl Outdated
@@ -114,3 +114,9 @@ function hash(s::String, h::UInt)
h += memhash_seed
ccall(memhash, UInt, (Ptr{UInt8}, Csize_t, UInt32), s, sizeof(s), h % UInt32) + h
end

function hash(m::Module, h::UInt64)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding this, we should make objectid use the module's UUID.

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 don't think we can do that currently. The UUID is mutable.

Copy link
Member

Choose a reason for hiding this comment

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

We will have to solve that --- adding a julia hash method doesn't help because the type system and cache won't call it. I suppose this algorithm could be put into objectid for now though.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see you did that already. So this method can just be removed.

@Keno Keno force-pushed the kf/allowmoduletparam branch from 1870342 to 161acbb Compare December 2, 2022 00:40
@Keno Keno changed the title RFC: Allow Module as type parameters Allow Module as type parameters Dec 2, 2022
@Keno Keno force-pushed the kf/allowmoduletparam branch from 161acbb to a254dcc Compare December 2, 2022 03:25
Keno added a commit to JuliaDebug/CassetteOverlay.jl that referenced this pull request Dec 2, 2022
Can be used in place of the macro to give a pass CassetteoOverlay
behavior. The abstract type works by reading the appropriate binding
from the type parameter.

Requires JuliaLang/julia#47749
Keno added a commit to JuliaDebug/CassetteOverlay.jl that referenced this pull request Dec 2, 2022
Can be used in place of the macro to give a pass CassetteoOverlay
behavior. The abstract type works by reading the appropriate binding
from the type parameter.

Requires JuliaLang/julia#47749
@DilumAluthge
Copy link
Member

Does this let you dispatch on modules? E.g. can I write:

struct Foo{M}
end

f(x::Foo{<:CSV}) = ...
f(x::Foo{<:DataFrames}) = ...

@oscardssmith
Copy link
Member

yes

The intended use case for this is generated functions that want to
generate some reference to a module-specific generic function. The
current solution is to duplicate the generated function into every
module (probably using a package-provided macro) or to have some
sort of registry system in the package providing the generated
function. Both of these seem a bit ugly and I don't think there's
any particularly good reason not to allow Modules to be type
parameters. Admittedly, modules are not part of the scope
contemplated by #33387 as they are mutable, but I think the mental
model of modules is that they're immutable references to a namespace
and what's actually mutable is the namespace itself (i.e. people
wouldn't expect two modules that happen to have the same content
be `==`). This makes me think it still fits the mental model.
@Keno Keno force-pushed the kf/allowmoduletparam branch from a254dcc to 93b9aa8 Compare December 2, 2022 20:30
@JeffBezanson
Copy link
Member

Without the <: of course.

@Keno Keno merged commit 2a0eb70 into master Dec 2, 2022
@Keno Keno deleted the kf/allowmoduletparam branch December 2, 2022 23:03
@vchuravy
Copy link
Member

vchuravy commented Dec 4, 2022

Broke the gcext test

@vtjnash
Copy link
Member

vtjnash commented Dec 4, 2022

why is that allow-fail if you expect it to be noticed?

/cache/build/default-amdci5-0/julialang/julia-master/test/gcext/gcext.c: In function ‘main’:
/cache/build/default-amdci5-0/julialang/julia-master/test/gcext/gcext.c:615:14: error: too few arguments to function ‘jl_new_module’
  615 |     module = jl_new_module(jl_symbol("TestGCExt"));

@vchuravy
Copy link
Member

vchuravy commented Dec 5, 2022

Because we only recently wired it up and we have to backport some things to 1.9 before we can make it mandatory...

@vtjnash
Copy link
Member

vtjnash commented Dec 5, 2022

Can we make it conditional on branch name?

@DilumAluthge
Copy link
Member

Yeah, it's very easy to have different Buildkite configs for different branches.

Currently, 1.8 has its own config.

Currently, 1.9 and master have the same config (just for convenience), but we can split them whenever we want. cc: @staticfloat

@LilithHafner
Copy link
Member

#47383 may have broken @nanosoldier runbenchmarks("sort"). This is the previous commit, so testing here.

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - successfully executed benchmarks. A full report can be found here.

Keno added a commit that referenced this pull request Apr 8, 2023
This was an oversight in the implementation of #47749 and caused
spurious asserts in debug mode:

```
julia> struct Foo{A, B}; end

julia> Foo{Base}
julia-debug: /home/keno/julia/src/builtins.c:350: type_object_id_: Assertion `!tv->name->mutabl' failed.
```
Keno added a commit that referenced this pull request Apr 8, 2023
This was an oversight in the implementation of #47749 and caused
spurious asserts in debug mode:

```
julia> struct Foo{A, B}; end

julia> Foo{Base}
julia-debug: /home/keno/julia/src/builtins.c:350: type_object_id_: Assertion `!tv->name->mutabl' failed.
```
Xnartharax pushed a commit to Xnartharax/julia that referenced this pull request Apr 19, 2023
This was an oversight in the implementation of JuliaLang#47749 and caused
spurious asserts in debug mode:

```
julia> struct Foo{A, B}; end

julia> Foo{Base}
julia-debug: /home/keno/julia/src/builtins.c:350: type_object_id_: Assertion `!tv->name->mutabl' failed.
```
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.

8 participants