Add optional error message to `only` to simplify usage #43729

tdunning · 2022-01-09T23:07:16Z

Uses of only are often replaced (or just not done in the first place) with explicit logic to get better error messages. Allowing the error message to be passed in would prevent this and make only more useful.

tdunning · 2022-01-09T23:09:34Z

This just came up on slack where Mosè Giordano pointed out this line should use only. Unfortunately, that loses the nice error message

https://github.com/JuliaPackaging/BinaryBuilder.jl/blob/4d263be3295dad597cc8e16d0b57374ccb06ace0/src/AutoBuild.jl#L1443

This tiny patch would allow the error message to be included in the call to only thus getting the concision of only with the kind error message.

DilumAluthge · 2022-01-10T02:03:15Z

Are we planning on dispatching on the type of message? If not, it seems like this might be better suited for a keyword argument?

DilumAluthge · 2022-01-10T02:04:31Z

Also, it seems to me that the must contain exactly 1 element part should always be kept in, and we just append the user's custom message if provided.

bramtayl · 2022-01-10T02:10:10Z

Maybe a macro would be more generalizable? Then you could throw a warning, or print a message, too?

only_one = @only things otherwise

mcabbott · 2022-01-10T02:10:41Z

The downside of this is that any message will be created unconditionally, e.g. this string interpolation will happen even when there is no error:

only(uuids, """
    Multiple UUIDs found for package `$(depname)`.
    Use `PackageSpec(name = \"$(depname)\", uuid = ..." to specify the UUID explicitly.
    """)

I'm not sure that matters there, but in tighter loops 100ns or whatever will hurt. It could solve this by being @only I guess.

DilumAluthge · 2022-01-10T02:05:14Z

base/iterators.jl

 Stacktrace:
 [...]
 ```
 """
-@propagate_inbounds function only(x)
+@propagate_inbounds function only(x, message="must contain exactly 1 element")


Suggested change

@propagate_inbounds function only(x, message="must contain exactly 1 element")

@propagate_inbounds function only(x; hint::String="")

DilumAluthge · 2022-01-10T02:06:32Z

base/iterators.jl

    i = iterate(x)
    @boundscheck if i === nothing
-        throw(ArgumentError("Collection is empty, must contain exactly 1 element"))
+        throw(ArgumentError("Collection is empty, $message"))


Suggested change

throw(ArgumentError("Collection is empty, $message"))

str = "Collection is empty, must contain exactly 1 element"

if hint !== ""

str = "$(str); $(hint)"

end

throw(ArgumentError(str))

DilumAluthge · 2022-01-10T02:07:15Z

base/iterators.jl

    end
    (ret, state) = i::NTuple{2,Any}
    @boundscheck if iterate(x, state) !== nothing
-        throw(ArgumentError("Collection has multiple elements, must contain exactly 1 element"))
+        throw(ArgumentError("Collection has multiple elements, $message"))


Suggested change

throw(ArgumentError("Collection has multiple elements, $message"))

str = "Collection has multiple elements, must contain exactly 1 element"

if hint !== ""

str = str * "; " * hint

end

throw(ArgumentError(str))

DilumAluthge · 2022-01-10T02:08:19Z

base/iterators.jl

@@ -1372,10 +1372,11 @@ IteratorEltype(::Type{Stateful{T,VS}}) where {T,VS} = IteratorEltype(T)
 length(s::Stateful) = length(s.itr) - s.taken

 """
-    only(x)
+    only(x, [message])


Suggested change

only(x, [message])

only(x; hint::String = "")

DilumAluthge · 2022-01-10T02:09:27Z

base/iterators.jl


 Return the one and only element of collection `x`, or throw an [`ArgumentError`](@ref) if the
-collection has zero or multiple elements.
+collection has zero or multiple elements. An optional error message can be provided to customize


Suggested change

collection has zero or multiple elements. An optional error message can be provided to customize

collection has zero or multiple elements. An optional error hint can be provided to customize

DilumAluthge · 2022-01-10T02:09:43Z

base/iterators.jl

+Stacktrace:
+[...]
+
+julia> only(('a', 'b'), "my error message")


Suggested change

julia> only(('a', 'b'), "my error message")

only(('a', 'b'); hint = "my error hint")

@bramtayl I think that macros are generally more flexible, but I think that would be an addition outside the scope here.

@mcabbott Good point about performance. I think that @DilumAluthge 's changes are constructive in the spirit of avoiding unnecessary interpolation.

@DilumAluthge Regarding your changes to move to keyword arguments, is there actually a significant win over a default value approach? I can't see that this is a hotbed of multiple dispatch, although one could imagine that it might become such (for instance, passing in a fully constructed exception to raise instead of the normal ArgumentException) but I can't see a big difference either way. If you feel there even a modest wind blowing toward the keyword approach, I am fine with it. I would tune the example slightly. I might also go further by adding message and hint.

I don't think that making it a keyword will change the interpolation cost. The user's string is still made before the function is called.

What could perhaps avoid that without a macro is to accept e.g. only(xs, "expected ", typeof(x), " to have one element "), crudely:

only(things, message...) = length(things)==1 ? things[1] : error(string(message...))

But that also seems a bit weird, and you have to remember to write it that way.

DilumAluthge · 2022-01-10T06:40:21Z

I'm not sure if this is the right approach. Would it be better to instead use the Base.Experimental.register_error_hint interface for this?

cc: @timholy

DilumAluthge · 2022-01-10T06:46:03Z

It would be good for triage to take a look at this.

Here are some specific questions for triage to consider:

Is it appropriate for this functionality to be added to Base, or would it be better suited for a package?
Should this functionality be added to the existing only function, or should a new @only macro be created?
Should the message/hint be passed as a positional argument or a keyword argument?
Should the message/hint be restricted to String, or should any type be allowed?
Would this be a good use case for the experimental Base.Experimental.register_error_hint interface?

tdunning · 2022-01-10T07:04:05Z

I noticed just now that there are more forms that need handling.

The question about whether we dispatch on the presence of the error message is now more interesting. Is it better to dispatch to a different entry point? Or will the extra method that Julia inserts without the default all just melt away under inlining (even down to the conditional on the presence of the message).

tdunning · 2022-01-10T07:11:34Z

Question number 5 in particular ran me off the cliff.

My general feeling is that overworking this is probably not worth it. Y'all are being amazingly responsive, but I am sure there are bigger things pending that I wouldn't want held up. If you feel this is worth it, cool. If not, also cool.

DilumAluthge · 2022-01-10T07:50:08Z

An alternate approach might be something like x = only(collection, MyCustomExceptionType::Callable).

Then, you can define Base.showerror for your MyCustomExceptionType type, and have complete control over the error message that is printed.

To address the issue of paying the interpolation cost even when the error isn't thrown, you could do something like this:

depname = "DataFrames"
x = only(collection, () -> MyCustomExceptionType(depname))

And then you don't actually perform the string interpolation until the Base.showerror(io:IO, exc::MyCustomExceptionType) function is called, which will only happen if the error is thrown.

If we switch the order of the arguments to be x = only(MyCustomExceptionType::Callable, collection), that would let people use do blocks as such:

depname = "DataFrames"
x = only(collection) do
    MyCustomExceptionType(depname)
end

DilumAluthge · 2022-01-10T07:55:15Z

Question number 5 in particular ran me off the cliff.

Don't worry too much about question 5. I'm not very familiar with the register_error_hint functionality (which is why I want someone else to answer that question), but my guess is that register_error_hint is probably not going to be relevant for this use case.

But that's why I want someone that's familiar with register_error_hint and its intended use cases to weigh in.

fredrikekre · 2022-01-10T13:23:35Z

This is quite strange API, it doesn't exist anywhere else I think? Something like tryonly(x) that return nothing instead of erroring would be more "base-api-like", and then you can handle the nothing case however you see fit (e.g. custom error message).

DilumAluthge · 2022-01-10T21:36:58Z

Something like tryonly(x) that return nothing instead of erroring would be more "base-api-like", and then you can handle the nothing case however you see fit (e.g. custom error message).

That sounds like a good approach!

tdunning · 2022-01-11T06:44:32Z

Allowing a Callable exception argument is quite reasonable although it forces a slightly more verbose idiom.

We don't have to use every cleverness in julia for this simple extension. Folks lived without any way to customize the error message for quite some time.

tdunning · 2022-01-11T17:29:45Z

I just reviewed the test results. The tester_linux64 failure that I see is a bit of a mystery. Parallel builds of the same sort succeeded and the test that failed was a process issue.

It seems that the test process just fell over due to external issues.

c42f · 2022-01-14T08:08:31Z

This is quite strange API, it doesn't exist anywhere else I think?

I agree, I don't think we should add arguments to functions just to customize their internal error handling, especially if there's a performance cost. Base doesn't have a lot of APIs like this and the ones which do exist are slightly weird anomalies (Eg, Meta.parse(str, raise=false))

However Dilum's do block idea does have precedent in the widely used get() function, and makes this whole idea much more useful: it's not just about error handling, but about giving a nice way to return a default if the collection is not of length one, but without computing the default value up front:

# A way to provide defaults
x = only(collection) do
    101
end

# Works well when the default is a costly computation
x = only(collection) do
    expensive_computation()
end

# Also works for custom error messages
x = only(collection) do
    error("Oops your collection was empty")
end

All the expressions above have equivalents with get(collection, key) do ... end

JeffBezanson · 2022-01-14T19:05:12Z

A long time ago we used to have a function version of assert that took a string message argument, and I saw some applications that spent all their time interpolating those error strings. That was not good 😄 I like the do block idea.

tdunning · 2022-01-14T20:24:45Z

The do block approach is very nice they way you can either provide a default or throw an error. Very visible.

JeffBezanson · 2022-01-20T19:38:56Z

From triage: please go ahead with the function argument (do block) implementation.

tdunning · 2022-01-20T22:59:05Z

Will do. I will experiment to see how hard it would be to do the same with similar functions like first or last.

…

On Thu, Jan 20, 2022 at 11:39 AM Jeff Bezanson ***@***.***> wrote: From triage: please go ahead with the function argument (do block) implementation. — Reply to this email directly, view it on GitHub <#43729 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5E6WKFB4QVHORD4GHD3TUXBQFZANCNFSM5LSMP4GQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

Add optional error message to only

a6786cf

DilumAluthge reviewed Jan 10, 2022

View reviewed changes

DilumAluthge added the triage This should be discussed on a triage call label Jan 10, 2022

Added message to all forms of only.

344baf0

Eliminated trailing whitespace

0beae0a

Adjusting tests to match error message

726df15

DilumAluthge added collections Data structures holding multiple items, e.g. sets iteration Involves iteration or the iteration protocol labels Jan 10, 2022

Fixed silly typo

42a0368

JeffBezanson removed the triage This should be discussed on a triage call label Jan 20, 2022

DilumAluthge mentioned this pull request Mar 1, 2022

Add the only(f, x) method, which allows users to provide a default value or customize the error message #44397

Closed

DilumAluthge marked this pull request as draft March 1, 2022 18:20

tdunning closed this by deleting the head repository Jun 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional error message to `only` to simplify usage #43729

Add optional error message to `only` to simplify usage #43729

tdunning commented Jan 9, 2022 •

edited

Loading

tdunning commented Jan 9, 2022

DilumAluthge commented Jan 10, 2022

DilumAluthge commented Jan 10, 2022

bramtayl commented Jan 10, 2022

mcabbott commented Jan 10, 2022

DilumAluthge Jan 10, 2022

DilumAluthge Jan 10, 2022

DilumAluthge Jan 10, 2022 •

edited

Loading

DilumAluthge Jan 10, 2022

DilumAluthge Jan 10, 2022

DilumAluthge Jan 10, 2022

tdunning Jan 10, 2022

mcabbott Jan 10, 2022 •

edited

Loading

DilumAluthge commented Jan 10, 2022

DilumAluthge commented Jan 10, 2022 •

edited

Loading

tdunning commented Jan 10, 2022

tdunning commented Jan 10, 2022

DilumAluthge commented Jan 10, 2022 •

edited

Loading

DilumAluthge commented Jan 10, 2022 •

edited

Loading

fredrikekre commented Jan 10, 2022

DilumAluthge commented Jan 10, 2022 •

edited

Loading

tdunning commented Jan 11, 2022

tdunning commented Jan 11, 2022

c42f commented Jan 14, 2022

JeffBezanson commented Jan 14, 2022

tdunning commented Jan 14, 2022

JeffBezanson commented Jan 20, 2022

tdunning commented Jan 20, 2022 via email

	@propagate_inbounds function only(x, message="must contain exactly 1 element")
	@propagate_inbounds function only(x; hint::String="")

-        throw(ArgumentError("Collection is empty, $message"))
+        str = "Collection is empty, must contain exactly 1 element"
+        if hint !== ""
+            str = "$(str); $(hint)"
+        end
+        throw(ArgumentError(str))

	collection has zero or multiple elements. An optional error message can be provided to customize
	collection has zero or multiple elements. An optional error hint can be provided to customize

	julia> only(('a', 'b'), "my error message")
	only(('a', 'b'); hint = "my error hint")

Add optional error message to only to simplify usage #43729

Add optional error message to only to simplify usage #43729

Conversation

tdunning commented Jan 9, 2022 • edited Loading

tdunning commented Jan 9, 2022

DilumAluthge commented Jan 10, 2022

DilumAluthge commented Jan 10, 2022

bramtayl commented Jan 10, 2022

mcabbott commented Jan 10, 2022

DilumAluthge Jan 10, 2022

Choose a reason for hiding this comment

DilumAluthge Jan 10, 2022

Choose a reason for hiding this comment

DilumAluthge Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

DilumAluthge Jan 10, 2022

Choose a reason for hiding this comment

DilumAluthge Jan 10, 2022

Choose a reason for hiding this comment

DilumAluthge Jan 10, 2022

Choose a reason for hiding this comment

tdunning Jan 10, 2022

Choose a reason for hiding this comment

mcabbott Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

DilumAluthge commented Jan 10, 2022

DilumAluthge commented Jan 10, 2022 • edited Loading

tdunning commented Jan 10, 2022

tdunning commented Jan 10, 2022

DilumAluthge commented Jan 10, 2022 • edited Loading

DilumAluthge commented Jan 10, 2022 • edited Loading

fredrikekre commented Jan 10, 2022

DilumAluthge commented Jan 10, 2022 • edited Loading

tdunning commented Jan 11, 2022

tdunning commented Jan 11, 2022

c42f commented Jan 14, 2022

JeffBezanson commented Jan 14, 2022

tdunning commented Jan 14, 2022

JeffBezanson commented Jan 20, 2022

tdunning commented Jan 20, 2022 via email

Add optional error message to `only` to simplify usage #43729

Add optional error message to `only` to simplify usage #43729

tdunning commented Jan 9, 2022 •

edited

Loading

DilumAluthge Jan 10, 2022 •

edited

Loading

mcabbott Jan 10, 2022 •

edited

Loading

DilumAluthge commented Jan 10, 2022 •

edited

Loading

DilumAluthge commented Jan 10, 2022 •

edited

Loading

DilumAluthge commented Jan 10, 2022 •

edited

Loading

DilumAluthge commented Jan 10, 2022 •

edited

Loading