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

Improving Printf error messages #45366

Merged
merged 10 commits into from
Jul 8, 2022

Conversation

Seelengrab
Copy link
Contributor

@Seelengrab Seelengrab commented May 19, 2022

This replaces all "invalid format string: $f style messages with more informative error messages about what exactly is wrong and where it goes wrong:

screenshot_2022-05-20_16-45-42_212485069

There are also some parts that I couldn't yet figure out - I'll comment these specifics down below for a conversation.

@Seelengrab
Copy link
Contributor Author

Seelengrab commented May 19, 2022

Oh boy, looks like adding that additional error message that should cover what the docstring of @printf specifies broke something. Something about sysimg.jl and LibGit2?

LoadError("sysimg.jl", 19, 
    LoadError("/buildworker/worker/doctest_linux64/build/usr/share/julia/stdlib/v1.9/LibGit2/src/LibGit2.jl", 3,
    LoadError("/buildworker/worker/doctest_linux64/build/usr/share/julia/stdlib/v1.9/LibGit2/src/signature.jl", 58,
    LoadError("/buildworker/worker/doctest_linux64/build/usr/share/julia/stdlib/v1.9/LibGit2/src/signature.jl", 62,
        ArgumentError("First argument to `@printf` has to be either an `<: IO` to print to or a format `String`.")))))

@johnnychen94 johnnychen94 added domain:display and printing Aesthetics and correctness of printed representations of objects. domain:error messages Better, more actionable error messages labels May 19, 2022
@Seelengrab
Copy link
Contributor Author

Seelengrab commented May 19, 2022

Ok, I forgot that this is a macro and of course the first argument is either a literal String or a Symbol. I've removed the check, so it should work now. We'll get a MethodError later on with Printf.format, which is also not a good error, so I may insert a $io isa IO || throw(ArgumentError(...)) in the returned expression later.

@Seelengrab
Copy link
Contributor Author

Ok, no idea why the buildkite/julia-master job is still running (maybe macOS stuff?), but it seems like the change in error messages didn't break anything, which is good.

Looking through the existing tests, I'm not sure all of these are actually covered. Should I write some more to make sure they're explicitly covered?

stdlib/Printf/src/Printf.jl Outdated Show resolved Hide resolved
stdlib/Printf/src/Printf.jl Outdated Show resolved Hide resolved
stdlib/Printf/src/Printf.jl Outdated Show resolved Hide resolved
stdlib/Printf/src/Printf.jl Outdated Show resolved Hide resolved
@Seelengrab
Copy link
Contributor Author

Seelengrab commented May 20, 2022

Alright, the error message is much nicer now:

screenshot_2022-05-20_16-39-09_118051498

and it's also smart about not going too far back:

screenshot_2022-05-20_16-45-42_212485069

The only thing left now are the BigFloat things, which I'm still not too sure about how to best communicate.

@halleysfifthinc
Copy link
Contributor

Where is the LoadError part of the message coming from? InvalidFormatStringError is a new/independent <:Exception type with no relation to LoadError that I can see.

The extra new line between the error message and stacktrace might not be needed to sufficiently visually separate the text. With the indenting of the string and location arrow, I think there the error message will still be visually distinct from the stacktrace.

@Seelengrab
Copy link
Contributor Author

LoadError comes from it happening during loading of the code, specifically during parsing (as that's when the code is run due to it happening in the macro).

Yeah, I thought I'd try it first to see if I like it. I'm not sold either way 🤷

@Seelengrab Seelengrab marked this pull request as ready for review May 20, 2022 20:55
@Seelengrab
Copy link
Contributor Author

Alright, I've changed the \t to 4 spaces and clarified the error messages for the libmpfr case, as well as documented what that __BIG_FLOAT_MAX__ constant is for. Should be good to merge now.

@Seelengrab
Copy link
Contributor Author

Error in testset Profile:
Error During Test at C:\buildbot\worker-tabularasa\tester_win32\build\share\julia\stdlib\v1.9\Profile\test\allocs.jl:109
Test threw exception
Expression: length((Allocs.fetch()).allocs) > 10
OutOfMemoryError()

Huh - unrelated!

@Seelengrab
Copy link
Contributor Author

Does this need anything else (rebase, rerun of CI) or can this be merged, if there are no other comments?

@oscardssmith oscardssmith merged commit 60219d6 into JuliaLang:master Jul 8, 2022
ffucci pushed a commit to ffucci/julia that referenced this pull request Aug 11, 2022
pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:display and printing Aesthetics and correctness of printed representations of objects. domain:error messages Better, more actionable error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants