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

PropertyError similar to FieldError #55298

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

arhik
Copy link
Contributor

@arhik arhik commented Jul 29, 2024

This PR drafts a naive implementation for PropertyError similar to FieldError addressing #55224.

This first implementation piggybacks on FieldError.

julia> struct D
               a::Float32
               b::Float64
       end

julia> d = D(1, 2)
D(1.0f0, 2.0)

julia> d.c
ERROR: PropertyError: instance of type `D` has no property `c`
D has following properties (:a, :b)

Stacktrace:
 [1] getproperty(x::D, f::Symbol)
   @ Base ./Base.jl:53
 [2] top-level scope
   @ REPL[3]:1

caused by: FieldError: type D has no field c
Stacktrace:
 [1] getproperty(x::D, f::Symbol)
   @ Base ./Base.jl:51
 [2] top-level scope
   @ REPL[3]:1

We just overload getproperties in this implementation.

https://github.com/JuliaLang/julia/pull/55298/files#diff-552535418914ef7bb1ebe851ebb5bdd5c1485570a1b4c24ffa42a8d90aa972deR49-R55

image

Since we are piggybacking on FieldError with try catch exceptions, this might add overhead. We can push this implementation into jl_f_getfield or jl_has_no_field_error or any other function to keep Base.jl code unchanged may be.


update

Base.jl is unchanged now.

And PropertyError is duplicate of FieldError,nothing special, but code emission is specific to objects instead.

base/Base.jl Outdated Show resolved Hide resolved
@KristofferC KristofferC added the needs nanosoldier run This PR should have benchmarks run on it label Jul 29, 2024
@arhik
Copy link
Contributor Author

arhik commented Jul 29, 2024

At the end of this completed draft we would like Base.jl to remain unchanged. And manage everything in Core or builtins.


update:
Done

@arhik
Copy link
Contributor Author

arhik commented Jul 29, 2024

Design Decisions:

  1. This PR (naive).
  2. Unlike this PR PropertyError can be thrown directly from getfields internal functions, (Something I have affinity with).
  3. go through propertynames like in the issue requirement for PropertyError similar to FieldError #55224 (comment) or check field with hasfield directly. This will mask root cause like FieldError. It might mask original cause like FieldError when custom propertynames overload is implemented; will have to reraise FieldError maybe.

1st Design.

julia> d.c
ERROR: PropertyError: instance of type `D` has no property `c`
D has following properties (:a, :b)

Stacktrace:
 [1] getproperty(x::D, f::Symbol)
   @ Base ./Base.jl:53
 [2] top-level scope
   @ REPL[3]:1

caused by: FieldError: type D has no field c
Stacktrace:
 [1] getproperty(x::D, f::Symbol)
   @ Base ./Base.jl:51
 [2] top-level scope
   @ REPL[3]:1

2nd Design output will same but with keeping Base.jl unchanged and Core implementation to raise PropertyError

julia> d.c
ERROR: PropertyError: instance of type `D` has no property `c`
D has following properties (:a, :b)

Stacktrace:
 [1] getproperty(x::D, f::Symbol)
   @ Base ./Base.jl:53
 [2] top-level scope
   @ REPL[3]:1

caused by: FieldError: type D has no field c
Stacktrace:
 [1] getproperty(x::D, f::Symbol)
   @ Base ./Base.jl:51
 [2] top-level scope
   @ REPL[3]:1

3rd Design output will mask root cause like this.

julia> d.c
ERROR: PropertyError: instance of type `D` has no property `c`
D has following properties (:a, :b)

Stacktrace:
 [1] getproperty(x::D, f::Symbol)
   @ Base ./Base.jl:53
 [2] top-level scope
   @ REPL[3]:1

Looks like they are mergable. Will have to tinker on this.

Update:
Not sure if we have to think along these lines anymore.

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.

With how heavily this function is used, I am very skeptical that we can add any code here without causing problems

@arhik
Copy link
Contributor Author

arhik commented Jul 29, 2024

@vtjnash True. I sat on it for a while questioning the same.

I will bail out as soon as possible and close this when I see concrete evidence that this is impractical. May be you guys see it already. Let me check for myself.

@arhik arhik marked this pull request as draft July 29, 2024 19:29
@@ -450,7 +450,7 @@ JL_DLLEXPORT jl_value_t *jl_get_field(jl_value_t *o, const char *fld)
jl_task_t *ct = jl_current_task;
JL_TRY {
jl_value_t *s = (jl_value_t*)jl_symbol(fld);
int i = jl_field_index((jl_datatype_t*)jl_typeof(o), (jl_sym_t*)s, 1);
int i = jl_field_index((jl_datatype_t*)jl_typeof(o), (jl_sym_t*)s, 2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

temporary change. Trying to use existing signature.

jl_has_no_field_error(t, fld);
if (err == 2)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

temporary change. Trying to use existing signature. Avoiding more untrackable changes.

@arhik
Copy link
Contributor Author

arhik commented Jul 30, 2024

update:

FieldError and PropertyError are seperated
still needs more tinkering


julia> struct D
               a::Float32
               b::Float64
       end

julia> d = D(1, 2)
D(1.0f0, 2.0)

julia> d.l
ERROR: PropertyError: instance of type `DataType` has no property `l`
DataType has following properties (:name, :super, :parameters, :types, :instance, :layout, :hash, :flags)

Stacktrace:
 [1] getproperty(x::D, f::Symbol)
   @ Base ./Base.jl:49
 [2] top-level scope
   @ REPL[3]:1

julia> d.a
1.0f0

julia> d.b
2.0

julia> D.l
ERROR: FieldError: type DataType has no field l
Stacktrace:
 [1] getproperty(x::Type, f::Symbol)
   @ Base ./Base.jl:43
 [2] top-level scope
   @ REPL[6]:1

@arhik
Copy link
Contributor Author

arhik commented Jul 30, 2024

getfield and getproperty raises seperate errors as they work on Type and object respectively

julia> getfield(d, :c)
ERROR: FieldError: type D has no field c
Stacktrace:
 [1] top-level scope
   @ REPL[7]:1

julia> getproperty(d, :c)
ERROR: PropertyError: instance of type `DataType` has no property `c`
DataType has following properties (:name, :super, :parameters, :types, :instance, :layout, :hash, :flags)

Stacktrace:
 [1] getproperty(x::D, f::Symbol)
   @ Base ./Base.jl:49
 [2] top-level scope
   @ REPL[8]:1

@LilithHafner Can I pull you into this ?

@LilithHafner
Copy link
Member

Is your proposed semantic for determining whether to throw a PropertyError or a FieldError equivalent to this:

function Base.getproperty(x, f::Symbol)
    @inline
    res = try
        Some(getfield(x, f))
    catch ex
        ex isa FieldError ? nothing : rethrow()
    end
    res === nothing && throw(PropertyError(x, f))
    getfield(res, :value)
end

i.e. throw a property error whenever someone calls getproperty and only throw a field error whenever someone calls getfield directly?

@arhik
Copy link
Contributor Author

arhik commented Jul 30, 2024

@LilithHafner Thank you for asking. Thats right. Atleast that was what I am aiming for.

I just made a parallel path for objects to raise PropertyError. Most of the code is just copy paste of FieldError related code. I don't understand some of the parts. Any guidance would help. So I pulled you in. Thanks for checking on this.

@arhik arhik force-pushed the PropertyError branch 5 times, most recently from ac4f496 to 1b78e25 Compare July 31, 2024 11:55
@arhik
Copy link
Contributor Author

arhik commented Jul 31, 2024

Build should pass locally for anyone who wants to check.

Quickly wanted to shared with friends and missed inline annotation.
just to satisfy build warning as errors. No good real reason to do so.
docs

Update base.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs nanosoldier run This PR should have benchmarks run on it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants