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.

base/dict.jl

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.

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 · 2024-06-08T14:50:14Z

@LilithHafner I rebased code. Can you check and validate whenever you are free ?

We will have to register handlers for tests separately again.

typo

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

minor tweaks from suggestions Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>

LilithHafner

LGTM!

Thank you for tacking this issue and following through when it turned out to be more complex than it initially seemed it would be.

test/errorshow.jl

KristofferC · 2024-06-10T10:02:44Z

base/errorshow.jl

+            Consider using indexing syntax.
+
+            # Example
+            julia> dict = Dict(:$(field)=>5)


Seems odd to use markdown syntax here since this will just get printed to stderr.

Also, the error hints tend to be quite short and to the point. Something like:

Dictionaries are indexed using the [key] syntax

or something along those lines should be enough.

For most markdown syntax (e.g. code formatting), I agree with you (see above conversation in this PR). However, the single # header is also reasonable plaintext "syntax".

This is a very niche error case (it only happens when you call my_dict.foo) for some my_dict::Dict, so it is unlikely that this will come across as spammy. The only case I can think of where this extended explanation would be unwelcome is if folks are intentionally attempting to access internal Dict fields and also making typos.

It's good to keep the styling consistent with the rest of the hints for things to be cohesive. Following the example of * on AbstractString here would be the simplest.

KristofferC · 2024-06-10T10:03:42Z

base/errorshow.jl

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


Could be AbstractDict perhaps?

I disagree. I could implement an AbstractDict that forwards getproperty to getindex.

Could be said "typically" then.

tecosaur · 2024-06-11T02:14:36Z

Just to come in here at the 11th hour with a bit of greediness, could

struct FieldError <: Exception
    type::DataType
    field::Symbol
end

be

struct FieldError <: Exception
    val::Any
    field::Symbol
end

I'm thinking this could be useful when the properties of an object depend on the value, not just the type (e.g. DataFrame).

That said, I'm not sure if it's possible to do this without overhead? If so it seems nice to have though.

arhik · 2024-06-11T14:32:23Z

Just to come in here at the 11th hour with a bit of greediness, could
struct FieldError <: Exception
    type::DataType
    field::Symbol
end
be
struct FieldError <: Exception
    val::Any
    field::Symbol
end
I'm thinking this could be useful when the properties of an object depend on the value, not just the type (e.g. DataFrame).

That said, I'm not sure if it's possible to do this without overhead? If so it seems nice to have though.

I imagined a separate PropertyError Exception for that use case. I will let others speak on it. In my opinion having a separate Exception like PropertyError similar to FieldError will avoid confusion and also complexity in internal code. This will also be in line with existing convention like fieldnames and propertynames. As I said I will let other speak since I dont have much exposure to design decisions and internals.

You must be familiar with it FYI an implementation similar to first commit in this PR could possibly do the job for the use case you are alluding to. Ex: getproperty line here can be overwritten with custom function and error exception continuation.

arhik · 2024-06-11T14:38:46Z

Quickly tried this and pasting this. Let me know if this is what you are implying. (overhead version obviously)

struct PropertyError <: Exception
	val::Any
	x::Symbol
end

using DataFrames

Base.getproperty(d::DataFrame, x::Symbol) = begin
	if x in propertynames(d)
		return d[!, x]
	else
		throw(PropertyError(d, x))
	end
end

function Base.showerror(io::IO, exc::PropertyError)
	@nospecialize
	println(io, "PropertyError: could not access property $(exc.x) on this $(typeof(exc.val)) type\n")
	Base.Experimental.show_error_hints(io, exc)
end

function propertyAccessHandler(io, exc)
	@nospecialize
	x = exc.x
	val = exc.val
	if val isa DataFrame
		println(io, "Looks like there is no column `$x` defined on this DataFrame obj.")
		print(io, "Only following columns are defined on this object: ")
		print(io, "$(propertynames(val))")
	else
		println(io, 
		"""
		A simple else message here
		"""
		)
	end
end


Base.Experimental.register_error_hint(propertyAccessHandler, PropertyError)

# Test

h = DataFrame(ENV)

h.c

An implementation without overhead like this PR should do the job.

oscardssmith · 2024-06-11T14:41:03Z

I don't think the overhead is a problem here since it's only overhead in throwing a user-facing error.

oscardssmith · 2024-06-11T14:45:06Z

@arhik I agree that this would probably be an improvement, but I am tempted to just merge this now and make the change to Any in a followup PR. (how can you not merge when CI is fully green? It's a sign...)

base/errorshow.jl

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

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

arhik commented May 17, 2024

View reviewed changes

base/dict.jl Outdated Show resolved Hide resolved

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 2 times, most recently from 586d3f0 to 25e9ccb Compare May 27, 2024 13:04

arhik force-pushed the dictErrorHint branch 2 times, most recently from ad114f1 to b2676de Compare June 8, 2024 14:48

arhik and others added 7 commits June 10, 2024 00:19

register hint handlers for test

cb1d5c8

We will have to register handlers for tests separately again.

codegen changes for fielderror

fb1e0f7

typo in comment

752ac7c

typo

symbol instead of value in field error hint

5dfe5f8

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

removing code block

9006635

Apply suggestions from code review

a8ca205

minor tweaks from suggestions Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>

suggestions fix

2063b9b

arhik force-pushed the dictErrorHint branch from 13bd084 to 2063b9b Compare June 9, 2024 18:58

LilithHafner approved these changes Jun 9, 2024

View reviewed changes

LilithHafner added the status:merge me PR is reviewed. Merge when all tests are passing label Jun 9, 2024

LilithHafner reviewed Jun 9, 2024

View reviewed changes

test/errorshow.jl Outdated Show resolved Hide resolved

32 bit systems update

7f09293

KristofferC reviewed Jun 10, 2024

View reviewed changes

KristofferC removed the status:merge me PR is reviewed. Merge when all tests are passing label Jun 10, 2024

KristofferC reviewed Jun 10, 2024

View reviewed changes

LilithHafner approved these changes Jun 10, 2024

View reviewed changes

shorter description for error hint

b1ef85f

LilithHafner reviewed Jun 11, 2024

View reviewed changes

base/errorshow.jl Outdated Show resolved Hide resolved

Update base/errorshow.jl

4f96718

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

LilithHafner added status:merge me PR is reviewed. Merge when all tests are passing domain:error messages Better, more actionable error messages labels Jun 11, 2024

IanButterworth merged commit 7e5d9a3 into JuliaLang:master Jun 12, 2024
9 checks passed

oscardssmith added domain:collections Data structures holding multiple items, e.g. sets and removed status:merge me PR is reviewed. Merge when all tests are passing labels Jun 12, 2024

arhik deleted the dictErrorHint branch June 12, 2024 03:31

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

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

arhik commented Jun 8, 2024

LilithHafner left a comment

KristofferC Jun 10, 2024

KristofferC Jun 10, 2024

LilithHafner Jun 10, 2024

KristofferC Jun 10, 2024

KristofferC Jun 10, 2024

LilithHafner Jun 10, 2024

KristofferC Jun 10, 2024

tecosaur commented Jun 11, 2024

arhik commented Jun 11, 2024 •

edited

arhik commented Jun 11, 2024

oscardssmith commented Jun 11, 2024

oscardssmith commented Jun 11, 2024

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

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

LilithHafner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented May 17, 2024

arhik commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LilithHafner commented May 22, 2024

Choose a reason for hiding this comment

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 Jun 8, 2024

LilithHafner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tecosaur commented Jun 11, 2024

arhik commented Jun 11, 2024 • edited

arhik commented Jun 11, 2024

oscardssmith commented Jun 11, 2024

oscardssmith commented Jun 11, 2024

arhik commented May 17, 2024 •

edited

arhik commented May 17, 2024 •

edited

arhik commented Jun 11, 2024 •

edited