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

internals: add _defaultctor function for defining ctors #57317

Merged
merged 2 commits into from
Feb 14, 2025
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Feb 9, 2025

Deferring this part of defining the ctor code allows it to do some value-based optimizations, without the awkwardness previously of needing to do conditional lowering of all of the possible versions that might be useful just from syntax transforms. This feels very like a generated function: how it expands just the function body, except it is flipped so that then we wrap the result in a single Method instead of being created from a Method!

Avoids lowering inaccuracies where a variable appears but is not used. These warnings are now prevented:

julia> struct Foo{T}
           x::(T; Any)
       end
WARNING: method definition for Foo at REPL[1]:2 declares type variable T but does not use it.

julia> struct Foo{T}
   x::Union{Any, T}
end
WARNING: method definition for Foo at REPL[1]:2 declares type variable T but does not use it.

Avoids hitting #31542. This lowering mistake is now avoided:

julia> struct Foo{T}
           x::(Val{T} where T)
       end
ERROR: syntax: invalid variable expression in "where" around REPL[1]:2

As a minor optimization, this avoids generating a convert call when the user declares explicit ::Any declarations, the same as if the user didn't annotate the field type:

julia> struct Foo
           x::Any
           y::Int
           z
       end

julia> code_lowered(Foo)[2]
CodeInfo(
    @ REPL[1]:2 within `unknown scope`
1%1 =   builtin Core.fieldtype(#ctor-self#, 2)%2 = y
│        @_4 = %2%4 =   builtin @_4 isa %1
└──      goto #3 if not %4
2 ─      goto #4
3@_4 = Base.convert(%1, @_4)
4%8 = @_4%9 = %new(#ctor-self#, x, %8, z)
└──      return %9
)

The outer/inner names might be a bit historical at this point (predating where clauses allowing specifying them flexibly inside or outside of the struct def): they are really exact-type-type&convert-args-from-any / exact-arg-types&apply-type-from-args if named for precisely what they do.

@vtjnash vtjnash added compiler:lowering Syntax lowering (compiler front end, 2nd stage) backport 1.12 Change should be backported to release-1.12 labels Feb 9, 2025
@oscardssmith
Copy link
Member

Does JuliaLowering need to copy this?

@Keno
Copy link
Member

Keno commented Feb 10, 2025

Is there any strong reason to write this code in C? I have been generally hoping to move more of this kind of code to Julia.

@vtjnash
Copy link
Member Author

vtjnash commented Feb 11, 2025

Bootstrapping mainly--ctors are pretty low level and important. In theory this should be replaceable by any arbitrary call later, although Revise may want to model it

@vtjnash
Copy link
Member Author

vtjnash commented Feb 11, 2025

It may be an interesting use case for binding replacement too, as we could turn this into a julia function after bootstrapping gets far enough

@Keno
Copy link
Member

Keno commented Feb 11, 2025

I guess my question was mainly, which default ctors do we define between the point where bootstrapping is far enough that struct definition works at all and the point where we have enough infrastructure to write this function. My intuition would be that that delta isn't actually that large, but I don't know.

@KristofferC KristofferC mentioned this pull request Feb 11, 2025
32 tasks
vtjnash added a commit to timholy/Revise.jl that referenced this pull request Feb 13, 2025
This is the Revise.jl dual of JuliaLang/julia#57317, needed so that PR can be merged
Deferring this part of defining the ctor code allows it to do some
value-based optimizations, without the awkwardness previously of needing
to do conditional lowering of all of the possible versions that might be
useful just from syntax transforms. This feels very like a generated
function: how it expands just the function body, except it is flipped so
that then we wrap the result in a single Method instead of being created
from a Method!

Avoids lowering inaccuracies where a variable appears but is not used.
These warnings are now prevented:
```julia
julia> struct Foo{T}
           x::(T; Any)
       end
WARNING: method definition for Foo at REPL[1]:2 declares type variable T but does not use it.

julia> struct Foo{T}
   x::Union{Any, T}
end
WARNING: method definition for Foo at REPL[1]:2 declares type variable T but does not use it.
```

Avoids hitting #31542.
This lowering mistake is now avoided:
```julia
julia> struct Foo{T}
           x::(Val{T} where T)
       end
ERROR: syntax: invalid variable expression in "where" around REPL[1]:2
```

As a minor optimization, this avoids generating a `convert` call when
the user declares explicit `::Any` declarations, the same as if the user
didn't annotate the field type:
```julia
julia> struct Foo
           x::Any
           y::Int
           z
       end

julia> code_lowered(Foo)[2]
CodeInfo(
    @ REPL[1]:2 within `unknown scope`
1 ─ %1 =   builtin Core.fieldtype(#ctor-self#, 2)
│   %2 = y
│        @_4 = %2
│   %4 =   builtin @_4 isa %1
└──      goto #3 if not %4
2 ─      goto #4
3 ─      @_4 = Base.convert(%1, @_4)
4 ┄ %8 = @_4
│   %9 = %new(#ctor-self#, x, %8, z)
└──      return %9
)
```
This was breaking effects tests.
@KristofferC KristofferC mentioned this pull request Feb 14, 2025
31 tasks
@vtjnash vtjnash merged commit f4a9d25 into master Feb 14, 2025
4 of 7 checks passed
@vtjnash vtjnash deleted the jn/defaultctors branch February 14, 2025 15:39
@vtjnash
Copy link
Member Author

vtjnash commented Feb 14, 2025

Most of this infrastructure is still in julia-syntax.scm, since synthesizing a correct CodeInfo (even a simple one like this) is non-trivial to do correctly, and so benefits from using the standard code path for that. This might be an interesting case for binding replacement though, if we do ever want to give users the ability to write their own default constructor generator for their module, as there is potentially a lot that they could do with that (though equally they could just put a macro in there to do the same).

KristofferC pushed a commit that referenced this pull request Feb 14, 2025
Deferring this part of defining the ctor code allows it to do some
value-based optimizations, without the awkwardness previously of needing
to do conditional lowering of all of the possible versions that might be
useful just from syntax transforms. This feels very like a generated
function: how it expands just the function body, except it is flipped so
that then we wrap the result in a single Method instead of being created
from a Method!

Avoids lowering inaccuracies where a variable appears but is not used.
These warnings are now prevented:
```julia
julia> struct Foo{T}
           x::(T; Any)
       end
WARNING: method definition for Foo at REPL[1]:2 declares type variable T but does not use it.

julia> struct Foo{T}
   x::Union{Any, T}
end
WARNING: method definition for Foo at REPL[1]:2 declares type variable T but does not use it.
```

Avoids hitting #31542. This
lowering mistake is now avoided:
```julia
julia> struct Foo{T}
           x::(Val{T} where T)
       end
ERROR: syntax: invalid variable expression in "where" around REPL[1]:2
```

As a minor optimization, this avoids generating a `convert` call when
the user declares explicit `::Any` declarations, the same as if the user
didn't annotate the field type:
```julia
julia> struct Foo
           x::Any
           y::Int
           z
       end

julia> code_lowered(Foo)[2]
CodeInfo(
    @ REPL[1]:2 within `unknown scope`
1 ─ %1 =   builtin Core.fieldtype(#ctor-self#, 2)
│   %2 = y
│        @_4 = %2
│   %4 =   builtin @_4 isa %1
└──      goto #3 if not %4
2 ─      goto #4
3 ─      @_4 = Base.convert(%1, @_4)
4 ┄ %8 = @_4
│   %9 = %new(#ctor-self#, x, %8, z)
└──      return %9
)
```

The outer/inner names might be a bit historical at this point (predating
where clauses allowing specifying them flexibly inside or outside of the
struct def): they are really exact-type-type&convert-args-from-any /
exact-arg-types&apply-type-from-args if named for precisely what they
do.

(cherry picked from commit f4a9d25)
KristofferC added a commit that referenced this pull request Feb 17, 2025
Backported PRs:
- [x] #57346 <!-- lowering: Only try to define the method once -->
- [x] #57341 <!-- bpart: When backdating replace the entire bpart chain
-->
- [x] #57381 <!-- staticdata: Set min validation world to require world
-->
- [x] #57357 <!-- Only implicitly `using` Base, not Core -->
- [x] #57383 <!-- staticdata: Fix typo in recursive edge revalidation
-->
- [x] #57385 <!-- bpart: Move kind enum into its intended place -->
- [x] #57275 <!-- Compiler: fix unsoundness of getfield_tfunc on Tuple
Types -->
- [x] #57378 <!-- print admonition for auto-import only once per module
-->
- [x] #57392 <!-- [LateLowerGCFrame] fix PlaceGCFrameReset for
returns_twice -->
- [x] #57388 <!-- Bump JuliaSyntax to v1.0.2 -->
- [x] #57266 <!-- 🤖 [master] Bump the Statistics stdlib from d49c2bf to
77bd570 -->
- [x] #57395 <!-- lowering: fix has_fcall computation -->
- [x] #57204 <!-- Clarify mathematical definition of `gcd` -->
- [x] #56794 <!-- Make `Pairs` public -->
- [x] #57407 <!-- staticdata: corrected implementation of
jl_collect_new_roots -->
- [x] #57405 <!-- bpart: Also partition the export flag -->
- [x] #57420 <!-- Compiler: Fix check for IRShow definedness -->
- [x] #55875 <!-- fix `(-Inf)^-1` inconsistency -->
- [x] #57317 <!-- internals: add _defaultctor function for defining
ctors -->
- [x] #57406 <!-- bpart: Ignore guard bindings for ambiguity purposes
-->
- [x] #49933 <!-- Allow for :foreigncall to transition to GC safe
automatically -->
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:lowering Syntax lowering (compiler front end, 2nd stage)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants