swift-log icon indicating copy to clipboard operation
swift-log copied to clipboard

Logging function autoclosures aren't "rethrows"

Open sjmadsen opened this issue 1 year ago • 8 comments

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

sjmadsen avatar Apr 26 '23 19:04 sjmadsen

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

tomerd avatar Apr 26 '23 20:04 tomerd

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...)

weissi avatar Apr 27 '23 12:04 weissi

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 avatar Jun 23 '23 02:06 ktoso

Major releases aren't a limited resource.

sjmadsen avatar Jun 23 '23 13:06 sjmadsen

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.

ktoso avatar Jun 26 '23 13:06 ktoso

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 {
  ...
}

tomerd avatar Jun 26 '23 17:06 tomerd

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 🤔

ktoso avatar Jun 28 '23 01:06 ktoso

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

sjmadsen avatar Jun 28 '23 13:06 sjmadsen