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

effects: improve effects analysis for Type-objects #46328

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aviatesk
Copy link
Member

This PR is composed of the following two changes:

@aviatesk aviatesk requested a review from Keno August 12, 2022 09:43
@aviatesk aviatesk added don't squash Don't squash merge compiler:effects effect analysis labels Aug 12, 2022
@martinholters
Copy link
Member

Type-object are immutable (essentially on Julia-level)

Almost, but not quite:

julia> struct T; x::Val{T.flags}; end

julia> T.flags
0x6a

julia> fieldtype(T, :x)
Val{0x62}

Don't know whether that is to worry about in practice, but it allows to produce the following inconsistency on this branch:

julia> f(::Type{T} where T) = Val{T.flags}
f (generic function with 1 method)

julia> struct T; x::f(T); end

julia> f(T)
Val{0x62}

julia> @code_typed f(T)
CodeInfo(
1return Val{0x6a}
) => Type{Val{0x6a}}

Can we restrict treating types as immutable to cases where a) we know not only it is a type, but which type and b) that type is fully constructed? Additionally, we'd want the result for a type that is still under construction not to be cached, as a future analysis might be less pessimistic. Can we do that?

Base automatically changed from avi/isType to master August 13, 2022 01:01
@martinholters
Copy link
Member

If we decide correctness is edge cases like those is sufficiently important to to be more conservative about treating types as immutable, we could use the opportunity to also fix this case which we already have on master:

julia> f(::Type{T}) where T = isdefined(T, :instance)
f (generic function with 1 method)

julia> struct T; x::Val{f(T)}; end

julia> f(T)
false

julia> @code_typed f(T)
CodeInfo(
1return true
) => Bool

@vtjnash
Copy link
Member

vtjnash commented Sep 2, 2022

It is UB right now to call a function (in this case f) on a type during its declaration. We allow it, because we need to permit apply_type expressions with {}, but otherwise it should perhaps be more carefully restricted for user-sanity.

1 similar comment
@vtjnash
Copy link
Member

vtjnash commented Sep 2, 2022

It is UB right now to call a function (in this case f) on a type during its declaration. We allow it, because we need to permit apply_type expressions with {}, but otherwise it should perhaps be more carefully restricted for user-sanity.

@vtjnash
Copy link
Member

vtjnash commented Sep 2, 2022

But as you note there, types have both #undef fields and non-constant fields (esp. layout and types)

@martinholters
Copy link
Member

There's also super:

julia> struct Foo <: (println(isdefined(Foo, :super)); Any) end
false

julia> isdefined(Foo, :super)
true

But AFAICT, all of these are set once during type construction and they don't change anymore.

It is UB right now to call a function (in this case f) on a type during its declaration.

But then this PR is valid as is? Or is a type actually changed somehow after its declaration is complete?

@vtjnash
Copy link
Member

vtjnash commented Sep 6, 2022

AFAIK currently, the PR assumption is not valid (#46328 (comment)), so currently we have specific lists of fields that are valid for these effects.

`Type`-object never has "undef" field and we should prove
`getfield` access to `Type`-object is `:consistent`.
@martinholters
Copy link
Member

Do you have an example where undef-ness or mutability is observable which is not already UB?

@aviatesk
Copy link
Member Author

Sorry I just mistakenly pushed unrelated changes into this branch and fixed it up. I will follow the discussion later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:effects effect analysis don't squash Don't squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants