error hint for getproperty(::Dict, ::Symbol) #54504

arhik · 2024-05-17T10:34:47Z

error hint for getproperty(::Dict, ::Symbol)

addresses #53618

arhik · 2024-05-17T11:05:44Z

@oscardssmith This is the least invasive commit with computational overhead on getproperty that I could come up with. I see a 10ns increase in getproperty calls. On mac M1 its jumping from 13ns to 24ns on average. Since Other exceptions are not carrying the information we need, I introduced an Exception type. Not sure if it's worth it. If it is a standard requirement we can make a separate commit on the MemberAccessError exception. (Should come up with better name may be). I am open to discussion on this PR.

KristofferC · 2024-05-17T11:10:27Z

If this is to be done it has to be done without any overhead at all for the working case. Isn't it possible to add an error hint for the already thrown exception or is it missing something?

arhik · 2024-05-17T11:14:30Z

@KristofferC I tried to fit other Exceptions available. I will take a look again. Even if there is an existing exception that fits the purpose, we will have to filter symbols that are actually Dict field members. So the problem should still exist. I am starting to think if this is necessary feature.

EDIT:
Sorry I think I misunderstood you. Already thrown exception should go through similar strategy I think.

Actually there is nuance to it. I will explain the overhead part clearly in the first message. Allow me some time to do that. Simply put user end code like keys(dict) won't see any overhead. but dict.jl using dict.keys anywhere will see an overhead.

arhik · 2024-05-17T11:17:05Z

base/dict.jl

@@ -116,6 +116,14 @@ Dict(ps::Pair...)                  = Dict(ps)

 Dict(kv) = dict_with_eltype((K, V) -> Dict{K, V}, kv, eltype(kv))

+function getproperty(d::Dict, x::Symbol)
+    if x in propertynames(d)


I will have to check if incoming symbol x is in the propertynames. This is where overhead will be. Other option is to change every occurance of getproperty with getfield in dict.jl. That is more involved and if other packages are trying to check fields of Dict, It will be a problem.

LilithHafner

Thanks for creating this hint! We should definitely add it to the language, though it will take a bit of work to make it fit in nicely with existing systems.

base/boot.jl

base/errorshow.jl

LilithHafner · 2024-05-17T11:47:04Z

base/dict.jl

+function getproperty(d::Dict, x::Symbol)
+    if x in propertynames(d)
+        return getfield(d, x)
+    else
+        throw(MemberAccessError(typeof(d), x))
+    end
+end


If we're going to add an UndefFieldError or MemberAccessError, instead of special casing Dict, it makes more sense to update the error thrown by getfield itself. This would require digging into the c code:

diff --git a/base/boot.jl b/base/boot.jl index 4a59bf6279..1569f6277e 100644 --- a/base/boot.jl +++ b/base/boot.jl @@ -205,8 +205,8 @@ export ErrorException, BoundsError, DivideError, DomainError, Exception, InterruptException, InexactError, OutOfMemoryError, ReadOnlyMemoryError, OverflowError, StackOverflowError, SegmentationFault, UndefRefError, UndefVarError, - TypeError, ArgumentError, MethodError, AssertionError, LoadError, InitError, - UndefKeywordError, ConcurrencyViolationError, + UndefFieldError, TypeError, ArgumentError, MethodError, AssertionError, LoadError, + InitError, UndefKeywordError, ConcurrencyViolationError, # AST representation Expr, QuoteNode, LineNumberNode, GlobalRef, # object model functions @@ -388,6 +388,11 @@ struct UndefKeywordError <: Exception var::Symbol end +struct UndefFieldError <: Exception + type::DataType + field::Symbol +end + const typemax_UInt = Intrinsics.sext_int(UInt, 0xFF) const typemax_Int = Core.Intrinsics.udiv_int(Core.Intrinsics.sext_int(Int, 0xFF), 2) diff --git a/src/datatype.c b/src/datatype.c index abbec420bb..1a4694a594 100644 --- a/src/datatype.c +++ b/src/datatype.c @@ -1736,7 +1736,7 @@ JL_DLLEXPORT int jl_field_index(jl_datatype_t *t, jl_sym_t *fld, int err) } } if (err) - jl_has_no_field_error(t->name->name, fld); + jl_has_no_field_error(t, fld); return -1; } diff --git a/src/jl_exported_data.inc b/src/jl_exported_data.inc index 79ff437841..90920df78a 100644 --- a/src/jl_exported_data.inc +++ b/src/jl_exported_data.inc @@ -138,6 +138,7 @@ XX(jl_uint8_type) \ XX(jl_undefref_exception) \ XX(jl_undefvarerror_type) \ + XX(jl_undeffielderror_type) \ XX(jl_unionall_type) \ XX(jl_uniontype_type) \ XX(jl_upsilonnode_type) \ diff --git a/src/jltypes.c b/src/jltypes.c index 420b88aeb7..89607a97de 100644 --- a/src/jltypes.c +++ b/src/jltypes.c @@ -3700,6 +3700,7 @@ void post_boot_hooks(void) jl_diverror_exception = jl_new_struct_uninit((jl_datatype_t*)core("DivideError")); jl_undefref_exception = jl_new_struct_uninit((jl_datatype_t*)core("UndefRefError")); jl_undefvarerror_type = (jl_datatype_t*)core("UndefVarError"); + jl_undeffielderror_type = (jl_datatype_t*)core("UndefFieldError"); jl_atomicerror_type = (jl_datatype_t*)core("ConcurrencyViolationError"); jl_interrupt_exception = jl_new_struct_uninit((jl_datatype_t*)core("InterruptException")); jl_boundserror_type = (jl_datatype_t*)core("BoundsError"); diff --git a/src/julia.h b/src/julia.h index 0d46f15776..2501040797 100644 --- a/src/julia.h +++ b/src/julia.h @@ -868,6 +868,7 @@ extern JL_DLLIMPORT jl_datatype_t *jl_initerror_type JL_GLOBALLY_ROOTED; extern JL_DLLIMPORT jl_datatype_t *jl_typeerror_type JL_GLOBALLY_ROOTED; extern JL_DLLIMPORT jl_datatype_t *jl_methoderror_type JL_GLOBALLY_ROOTED; extern JL_DLLIMPORT jl_datatype_t *jl_undefvarerror_type JL_GLOBALLY_ROOTED; +extern JL_DLLIMPORT jl_datatype_t *jl_undeffielderror_type JL_GLOBALLY_ROOTED; extern JL_DLLIMPORT jl_datatype_t *jl_atomicerror_type JL_GLOBALLY_ROOTED; extern JL_DLLIMPORT jl_datatype_t *jl_missingcodeerror_type JL_GLOBALLY_ROOTED; extern JL_DLLIMPORT jl_datatype_t *jl_lineinfonode_type JL_GLOBALLY_ROOTED; @@ -2023,7 +2024,7 @@ JL_DLLEXPORT void JL_NORETURN jl_type_error_rt(const char *fname, jl_value_t *ty JL_MAYBE_UNROOTED, jl_value_t *got JL_MAYBE_UNROOTED); JL_DLLEXPORT void JL_NORETURN jl_undefined_var_error(jl_sym_t *var, jl_value_t *scope JL_MAYBE_UNROOTED); -JL_DLLEXPORT void JL_NORETURN jl_has_no_field_error(jl_sym_t *type_name, jl_sym_t *var); +JL_DLLEXPORT void JL_NORETURN jl_has_no_field_error(jl_datatype_t *t, jl_sym_t *var); JL_DLLEXPORT void JL_NORETURN jl_atomic_error(char *str); JL_DLLEXPORT void JL_NORETURN jl_bounds_error(jl_value_t *v JL_MAYBE_UNROOTED, jl_value_t *t JL_MAYBE_UNROOTED); diff --git a/src/rtutils.c b/src/rtutils.c index f18a1ac112..29fa145880 100644 --- a/src/rtutils.c +++ b/src/rtutils.c @@ -152,9 +152,10 @@ JL_DLLEXPORT void JL_NORETURN jl_undefined_var_error(jl_sym_t *var, jl_value_t * jl_throw(jl_new_struct(jl_undefvarerror_type, var, scope)); } -JL_DLLEXPORT void JL_NORETURN jl_has_no_field_error(jl_sym_t *type_name, jl_sym_t *var) +JL_DLLEXPORT void JL_NORETURN jl_has_no_field_error(jl_datatype_t *t, jl_sym_t *var) { - jl_errorf("type %s has no field %s", jl_symbol_name(type_name), jl_symbol_name(var)); + // jl_errorf("type %s has no field %s", jl_symbol_name(t->name->name), jl_symbol_name(var)); + jl_throw(jl_new_struct(jl_undeffielderror_type, t, var)); } JL_DLLEXPORT void JL_NORETURN jl_atomic_error(char *str) // == jl_exceptionf(jl_atomicerror_type, "%s", str) diff --git a/src/staticdata.c b/src/staticdata.c index f6461e0b42..eaa5d66754 100644 --- a/src/staticdata.c +++ b/src/staticdata.c @@ -235,6 +235,7 @@ jl_value_t **const*const get_tags(void) { INSERT_TAG(jl_loaderror_type); INSERT_TAG(jl_initerror_type); INSERT_TAG(jl_undefvarerror_type); + INSERT_TAG(jl_undeffielderror_type); INSERT_TAG(jl_stackovf_exception); INSERT_TAG(jl_diverror_exception); INSERT_TAG(jl_interrupt_exception);

(This approach also has no overhead)

Wow. Can I borrow this and check ?

JeffBezanson · 2024-05-17T18:06:00Z

This needs a different name. We use "undef" to mean a null reference. Maybe just FieldError or PropertyError?

arhik · 2024-05-22T09:46:23Z

I like FieldError. Going with it.

LilithHafner · 2024-05-22T11:33:23Z

src/jltypes.c

    jl_undefvarerror_type  = (jl_datatype_t*)core("UndefVarError");
+    jl_fielderror_type = (jl_datatype_t*)core("FieldError");
    jl_atomicerror_type    = (jl_datatype_t*)core("ConcurrencyViolationError");


Suggested change

jl_undefvarerror_type = (jl_datatype_t*)core("UndefVarError");

jl_fielderror_type = (jl_datatype_t*)core("FieldError");

jl_atomicerror_type = (jl_datatype_t*)core("ConcurrencyViolationError");

jl_undefvarerror_type = (jl_datatype_t*)core("UndefVarError");

jl_fielderror_type = (jl_datatype_t*)core("FieldError");

jl_atomicerror_type = (jl_datatype_t*)core("ConcurrencyViolationError");

Consistent style

Thanks. Will batch it.

LilithHafner · 2024-05-22T11:34:18Z

This looks pretty good! It should have tests to make sure the new error is being thrown and the hint looks right.

arhik · 2024-05-22T11:36:45Z

base/errorshow.jl

+    @nospecialize
+    field = exc.field
+    type = exc.type
+    if type == :Dict


I was expecting a type Dict but I was a bit surprised to see I actually get Symbol. I should familiarize with internals more. But if anyone sees this as anomaly let me know.

Yes, that's seriously broken. I think the struct construction on the C side is buggy.

arhik · 2024-05-24T00:28:16Z

This looks pretty good! It should have tests to make sure the new error is being thrown and the hint looks right.

Added tests. Let me know if they are reasonable.

arhik · 2024-05-24T00:31:24Z

@LilithHafner I raised another PR JuliaSparse/SparseArrays.jl#539 related to this. How do we resolve this dependency. We need SparseArrays PR to be accepted for julia tests to pass. ref (https://buildkite.com/julialang/julia-master/builds/36697#018fa506-1c87-42bf-b902-09ef3cb581ec/826-1173)

…le in Base See also: JuliaLang/julia#54504

LilithHafner · 2024-05-25T19:06:02Z

This PR now includes the changes you made to SparseArrays tests

LilithHafner

For the most part, this LGTM, but the fact that the exception's type field is a Symbol when it is type checked to be a Type is very broken.

It would be nice to add a failing test for that, and we definitely can't merge before fixing it.

arhik · 2024-05-26T18:38:02Z

Understood the concern. I didn't get chance to look into it. I will try make time soon.

error hint for getproperty(::Dict, ::Symbol) addresses JuliaLang#53618

Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>

We will have to register handlers for tests separately again.

arhik · 2024-05-27T13:20:48Z

@LilithHafner The type information is lost even before it comes to jl_new_struct. On inspection found issue in codegen. We didn't make corresponding changes in llvm related emit function signatures.

@gbaraldi I made some codegen changes for FieldError. Its trivial but could have unintended consequences. Your view would help.

https://github.com/JuliaLang/julia/pull/54504/files#diff-62cfb2606c6a323a7f26a3eddfa0bf2b819fa33e094561fee09daeb328e3a1e7R4082

LilithHafner · 2024-05-27T14:42:52Z

I'm a little out of my depth here, I would appreciate @gbaraldi's feedback or for @gbaraldi to reccomend an alternative reviewer.

out of date

arhik changed the title ~~Update errorshow.jl~~ error hint for getproperty(::Dict, ::Symbol) May 17, 2024

arhik commented May 17, 2024

View reviewed changes

LilithHafner reviewed May 17, 2024

View reviewed changes

arhik force-pushed the dictErrorHint branch from 951ae71 to 3d35c75 Compare May 17, 2024 17:40

arhik force-pushed the dictErrorHint branch from 3d35c75 to 73026c5 Compare May 22, 2024 10:53

LilithHafner reviewed May 22, 2024

View reviewed changes

LilithHafner added the needs tests Unit tests are required for this change label May 22, 2024

arhik commented May 22, 2024

View reviewed changes

arhik mentioned this pull request May 23, 2024

tests to use fallback exception to handle FieldError exception JuliaSparse/SparseArrays.jl#539

Merged

LilithHafner pushed a commit to JuliaSparse/SparseArrays.jl that referenced this pull request May 25, 2024

Make tests expect FieldError exception once that exception is availab…

82b385f

…le in Base See also: JuliaLang/julia#54504

LilithHafner removed the needs tests Unit tests are required for this change label May 26, 2024

LilithHafner previously requested changes May 26, 2024

View reviewed changes

arhik force-pushed the dictErrorHint branch from 18bdc67 to 586d3f0 Compare May 27, 2024 13:01

arhik and others added 7 commits May 27, 2024 18:33

Update errorshow.jl

a5af1f0

error hint for getproperty(::Dict, ::Symbol) addresses JuliaLang#53618

formatting

12dda4c

Update base/boot.jl: julian naming

d877eb7

Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>

removing else case in Dict Hint

bf1d9af

Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>

Update base/errorshow.jl for better julian naming convention

a78491f

Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>

increment tag count

633112d

fixing existing tests

af68992

arhik added 4 commits May 27, 2024 18:33

missed few test fixes and formatting

b13443d

Test for Dict error hint and FieldError in general

be322c8

register hint handlers for test

5733411

We will have to register handlers for tests separately again.

codegen changes for fielderror

25e9ccb

arhik force-pushed the dictErrorHint branch from 586d3f0 to 25e9ccb Compare May 27, 2024 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

error hint for getproperty(::Dict, ::Symbol) #54504

error hint for getproperty(::Dict, ::Symbol) #54504

arhik commented May 17, 2024

arhik commented May 17, 2024 •

edited

KristofferC commented May 17, 2024

arhik commented May 17, 2024 •

edited

arhik May 17, 2024

LilithHafner left a comment

LilithHafner May 17, 2024

arhik May 17, 2024

JeffBezanson commented May 17, 2024

arhik commented May 22, 2024

LilithHafner May 22, 2024

arhik May 22, 2024

LilithHafner commented May 22, 2024

arhik May 22, 2024

LilithHafner May 22, 2024

arhik commented May 24, 2024

arhik commented May 24, 2024

LilithHafner commented May 25, 2024

LilithHafner left a comment

arhik commented May 26, 2024

arhik commented May 27, 2024

LilithHafner commented May 27, 2024

error hint for getproperty(::Dict, ::Symbol) #54504

Are you sure you want to change the base?

error hint for getproperty(::Dict, ::Symbol) #54504

Conversation

arhik commented May 17, 2024

arhik commented May 17, 2024 • edited

KristofferC commented May 17, 2024

arhik commented May 17, 2024 • edited

arhik May 17, 2024

Choose a reason for hiding this comment

LilithHafner left a comment

Choose a reason for hiding this comment

LilithHafner May 17, 2024

Choose a reason for hiding this comment

arhik May 17, 2024

Choose a reason for hiding this comment

JeffBezanson commented May 17, 2024

arhik commented May 22, 2024

LilithHafner May 22, 2024

Choose a reason for hiding this comment

arhik May 22, 2024

Choose a reason for hiding this comment

LilithHafner commented May 22, 2024

arhik May 22, 2024

Choose a reason for hiding this comment

LilithHafner May 22, 2024

Choose a reason for hiding this comment

arhik commented May 24, 2024

arhik commented May 24, 2024

LilithHafner commented May 25, 2024

LilithHafner left a comment

Choose a reason for hiding this comment

arhik commented May 26, 2024

arhik commented May 27, 2024

LilithHafner commented May 27, 2024

arhik commented May 17, 2024 •

edited

arhik commented May 17, 2024 •

edited