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

errorshow: simplify printing of keyword argument types using a new macro format #49959

Merged
merged 10 commits into from
May 31, 2023

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented May 26, 2023

In Julia, keyword arguments are represented as Base.Pairs objects. However, the object type often appears unnecessarily complex, especially when printed in a stack trace.

This commit aims to simplify the printing of stack traces that involve keyword method calls, while still allowing us to reconstruct the actual method signature types from the printed signature types.

The approach is similar to #49117: this commit introduces a new macro called Base.@Kwargs. It follows the same syntax as @NamedTuple and returns a Base.Pairs type that is used for keyword method calls. We use this syntax when printing keyword argument types.

Here's an example of a stack trace for sum("julia"; init=1):

diff --git a/b.jl b/a.jl
index 91dd6f0464..b804ae4be5 100644
--- a/b.jl
+++ b/a.jl
@@ -22,12 +22,11 @@ Stacktrace:
     @ Base ./reduce.jl:44 [inlined]
   [6] mapfoldl(f::typeof(identity), op::typeof(Base.add_sum), itr::String; init::Int64)
     @ Base ./reduce.jl:175 [inlined]
-  [7] mapreduce(f::typeof(identity), op::typeof(Base.add_sum), itr::String; kw::Base.Pairs{…})
+  [7] mapreduce(f::typeof(identity), op::typeof(Base.add_sum), itr::String; kw::@Kwargs{init::Int64})
     @ Base ./reduce.jl:307 [inlined]
-  [8] sum(f::typeof(identity), a::String; kw::Base.Pairs{Symbol, Int64, Tuple{Symbol}, @NamedTuple{init::Int64}})
+  [8] sum(f::typeof(identity), a::String; kw::@Kwargs{init::Int64})
     @ Base ./reduce.jl:535 [inlined]
-  [9] sum(a::String; kw::Base.Pairs{Symbol, Int64, Tuple{Symbol}, @NamedTuple{init::Int64}})
+  [9] sum(a::String; kw::@Kwargs{init::Int64})
     @ Base ./reduce.jl:564 [inlined]
  [10] top-level scope

Feel free to share any comments or suggestions. If this idea seems acceptable, I will add test cases and also address any broken test cases.

@aviatesk aviatesk force-pushed the avi/show-pairs branch 2 times, most recently from 82937a6 to f244ad9 Compare May 29, 2023 09:21
@KristofferC
Copy link
Member

If it is exported then the Base. prefix in Base.@Kwargs{init::Int64} would disappear. That seems worth it to me.

@aviatesk
Copy link
Member Author

I didn't export it because I considered it as an internal implementation detail that keyword arguments are represented as Base.Pairs. But I'm fine with exporting it as I agree with the benefit of further simplifying the stacktrace.

@KristofferC
Copy link
Member

I didn't export it because I considered it as an internal implementation detail that keyword arguments are represented as Base.Pairs.

If that implementation changes then @Kwargs would also change. I don't think a user of @Kwargs can expect that the return type is fixed, only that it returns the same one as used for keyword arguments.

base/show.jl Outdated
print(io, "}")
return
end
elseif get(io, :simplify_kwstype, false) && kwsnt !== nothing
Copy link
Member Author

Choose a reason for hiding this comment

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

I enabled the printing with the @Kwargs-macro syntax only within stack trace view. Please let me know if you think it is more reasonable to enable the printing in the other situations.

The main reasoning behind the current choice is that Base.Pairs is a general data structure and its usage is not restricted to represent keyword arguments.

@aviatesk aviatesk added keyword arguments f(x; keyword=arguments) error messages Better, more actionable error messages labels May 29, 2023
test/errorshow.jl Outdated Show resolved Hide resolved
test/show.jl Outdated Show resolved Hide resolved
@aviatesk aviatesk force-pushed the avi/show-pairs branch 2 times, most recently from 8aa7e1e to 5e6baf9 Compare May 30, 2023 06:24
@aviatesk
Copy link
Member Author

aviatesk commented May 31, 2023

I'd argue that 6 👍 means this PR is preferable. I'm planning to merge this PR tomorrow if there are no objections.

aviatesk and others added 9 commits May 31, 2023 16:40
…ew macro format

In Julia, keyword arguments are represented as `Base.Pairs` objects.
However, the object type often appears unnecessarily complex,
especially when printed in a stack trace.

This commit aims to simplify the printing of stack traces that involve
keyword method calls, while still allowing us to reconstruct the actual
method signature types from the printed signature types.

The approach is similar to #49117: this commit introduces a new macro
called `Base.@Kwargs`. It follows the same syntax as `@NamedTuple` and
returns a `Base.Pairs` type that is used for keyword method calls.
We use this syntax when printing keyword argument types.

Here's an example of a stack trace:
```diff
diff --git a/b.jl b/a.jl
index 91dd6f0464..b804ae4be5 100644
--- a/b.jl
+++ b/a.jl
@@ -22,12 +22,11 @@ Stacktrace:
     @ Base ./reduce.jl:44 [inlined]
   [6] mapfoldl(f::typeof(identity), op::typeof(Base.add_sum), itr::String; init::Int64)
     @ Base ./reduce.jl:175 [inlined]
-  [7] mapreduce(f::typeof(identity), op::typeof(Base.add_sum), itr::String; kw::Base.Pairs{…})
+  [7] mapreduce(f::typeof(identity), op::typeof(Base.add_sum), itr::String; kw::Base.@kwargs{init::Int64})
     @ Base ./reduce.jl:307 [inlined]
-  [8] sum(f::typeof(identity), a::String; kw::Base.Pairs{Symbol, Int64, Tuple{Symbol}, @NamedTuple{init::Int64}})
+  [8] sum(f::typeof(identity), a::String; kw::Base.@kwargs{init::Int64})
     @ Base ./reduce.jl:535 [inlined]
-  [9] sum(a::String; kw::Base.Pairs{Symbol, Int64, Tuple{Symbol}, @NamedTuple{init::Int64}})
+  [9] sum(a::String; kw::Base.@kwargs{init::Int64})
     @ Base ./reduce.jl:564 [inlined]
  [10] top-level scope
```

Feel free to share any comments or suggestions. If this idea seems
acceptable, I will add test cases and also address any broken test cases.
Since keyword pairs can appear within positional arguments, it can be
confusing if we print the same type with different representations.
@aviatesk aviatesk changed the title RFC: errorshow: simplify printing of keyword argument types using a new macro format errorshow: simplify printing of keyword argument types using a new macro format May 31, 2023
@aviatesk aviatesk merged commit 9b27a8f into master May 31, 2023
@aviatesk aviatesk deleted the avi/show-pairs branch May 31, 2023 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error messages Better, more actionable error messages keyword arguments f(x; keyword=arguments)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants