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 function autoclosures aren't "rethrows" #265

Open
sjmadsen opened this issue Apr 26, 2023 · 8 comments
Open

Logging function autoclosures aren't "rethrows" #265

sjmadsen opened this issue Apr 26, 2023 · 8 comments
Labels
⚠️ semver/major Breaks existing public API.

Comments

@sjmadsen
Copy link

Expected behavior

If I have a throwing function fn() or a throwing property accessor, it would be nice if these two log statements work:

try logger.info("calling fn = \(fn())")
try logger.info("value of property = \(object.property)")

Actual behavior

Right now it does not compile, giving the error Property access can throw, but it is executed in a non-throwing autoclosure.

I believe marking the autoclosures with rethrows will resolve this problem.

Steps to reproduce

  1. Per above, try a logging function with a string interpolation containing a throwing function call or property access.

If possible, minimal yet complete reproducer code (or URL to code)

I can provide something if it's really necessary, but I think there is enough information above.

SwiftLog version/commit hash

1.5.2

Swift & OS version (output of swift --version && uname -a)

swift-driver version: 1.75.2 Apple Swift version 5.8 (swiftlang-5.8.0.124.2 clang-1403.0.22.11.100)
Target: arm64-apple-macosx13.0
Darwin smadsen-MacBook-Pro 22.4.0 Darwin Kernel Version 22.4.0: Mon Mar 6 20:59:28 PST 2023; root:xnu-8796.101.5~3/RELEASE_ARM64_T6000 arm64

@tomerd
Copy link
Member

tomerd commented Apr 26, 2023

@ktoso @weissi iirc this change would be API breaking?

@weissi
Copy link
Member

weissi commented Apr 27, 2023

Technically it's API breaking because if you have

func foo(_ x: @autoclosure () throws -> String) rethrows {
    print(try x())
}

func oldFoo(_ x: @autoclosure () -> String) {
    print(x())
}

then you can do

let a: (@autoclosure () -> String) -> Void = oldFoo

but not

let a: (@autoclosure () -> String) -> Void = foo

(because it would drop the rethrows...)

@ktoso
Copy link
Member

ktoso commented Jun 23, 2023

Hmm, we also can't introduce this via more overloads, that gets ambiguous very quickly.

So it's either risking it, or calling a major... This probably isn't worth a major release though.

@ktoso ktoso added the ⚠️ semver/major Breaks existing public API. label Jun 23, 2023
@sjmadsen
Copy link
Author

Major releases aren't a limited resource.

@ktoso
Copy link
Member

ktoso commented Jun 26, 2023

They are problematic though for a package such as swift-log that is depended on by almost all high level frameworks and libraries in the server ecosystem, so we need to consider bumping major versions carefully. This isn't quite the same as it is in "leaf" packages.

@tomerd
Copy link
Member

tomerd commented Jun 26, 2023

we could add a new set of [re]throwing APIs but that will double the API surface area. or we can just add a single log(throwing: ...) one with no sugar, e.g

 public func log(
  level: Logger.Level,
  throwing: @autoclosure () throws -> Logger.Message,
  metadata: @autoclosure () throws -> Logger.Metadata? = nil,
  source: @autoclosure () throws -> String? = nil,
  file: String = #file, function: String = #function, line: UInt = #line
) rethrows {
  ...
}

@ktoso
Copy link
Member

ktoso commented Jun 28, 2023

Hm yes... I wonder if that's helpful enough, wdyt @sjmadsen ?

Personally I'd be interested to see if we'd actually break any oss package.. though ofc closed source users are a risk 🤔 if we did introduce rethrows... we could try to do a "community build" and see what breaks 🤔

@sjmadsen
Copy link
Author

log(throwing: ...) is obviously not ideal, but it is certainly better than nothing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ semver/major Breaks existing public API.
Projects
None yet
Development

No branches or pull requests

4 participants