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

logging: string->String #44567

Merged
merged 2 commits into from
Mar 17, 2022
Merged

logging: string->String #44567

merged 2 commits into from
Mar 17, 2022

Conversation

timholy
Copy link
Member

@timholy timholy commented Mar 11, 2022

This is from #44500. Note it's against backports-release-1.8

This is good material for 1.8 because of the introduction of LazyString, which will probably be used heavily by the logging framework.

@timholy timholy added this to the 1.8 milestone Mar 11, 2022
@JeffBezanson
Copy link
Member

Do we know what types message can have? Otherwise this might not work.

@timholy
Copy link
Member Author

timholy commented Mar 11, 2022

It has to be AbstractString because that's required for eachsplit. Before I added any types this was just msglines = eachsplit(message, '\n') (EDIT: that probably isn't right, it was probably msglines = eachsplit(string(message,) '\n').) With this change, you can do @warn LazyString(args...).

For Julia 2.0 we should rework the logging framework to specify some types on at least some of its variables. Code from the logging framework gets inserted into everything, and having it wide-open without any type annotations whatsoever causes so many vulnerabilities for invalidation. Either that or (1) write it in C, (2) hide it in a module like Core that won't invalidate, or (3) all logging code has to be hidden behind runtime dispatch.

@fredrikekre
Copy link
Member

message can be anything:

julia> @info sum
[ Info: sum

but with this PR this would throw a MethodError. Non-AbstractString messages are used quite a lot in code in the wild.

@timholy
Copy link
Member Author

timholy commented Mar 12, 2022

Didn't know that, thanks @fredrikekre. Unfortunate, then, that the entire test suite (esp. corelogging.jl) passed with the change I made.

@timholy
Copy link
Member Author

timholy commented Mar 14, 2022

I'll wait to merge this until #44550#44500 goes in.

@KristofferC
Copy link
Member

Hm, I don't see how that PR is related to this.

@timholy
Copy link
Member Author

timholy commented Mar 14, 2022

Good point. Fixed.

Base automatically changed from backports-release-1.8 to release-1.8 March 15, 2022 13:00
@timholy timholy merged commit e191c6e into release-1.8 Mar 17, 2022
@timholy timholy deleted the teh/logging_strings branch March 17, 2022 11:35
@DilumAluthge
Copy link
Member

This is from #44500. Note it's against backports-release-1.8

@KristofferC @timholy Note that this PR was not actually merged into backports-release-1.8. Instead, it was merged into release-1.8. This is because GitHub automatically change the base (target) branch of this PR two days ago.

@timholy
Copy link
Member Author

timholy commented Mar 17, 2022

Ugh, is this something we need to revert? Or what's the right way to handle it? FWIW I don't think this is a very risky PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants