Mark logger lambda parameters as crossinline

This prevents accidental non-local returns inside the lambda, which
would drop the log.

Author: hermannm

src/commonMain/kotlin/dev/hermannm/devlog/LogLevel.kt

@@ -103,8 +103,8 @@ public class LogLevel private constructor() {
    *     - We make the method `inline`, so we don't pay a cost for the lambdas
    * - In the unreachable else branch, we throw an exception
    *
-   * We may be able to use an enum/sealed class instead if this issue moves along:
-   * https://youtrack.jetbrains.com/issue/KT-38750/Support-declaration-site-nonexhaustiveness-for-enums-and-sealed-classes
+   * We may be able to use an enum/sealed class instead if
+   * [this issue moves along](https://youtrack.jetbrains.com/issue/KT-38750/Support-declaration-site-nonexhaustiveness-for-enums-and-sealed-classes).
    */
   @Suppress("LocalVariableName") // We want the argument names here to be the same as the constants
   internal inline fun <ReturnT> match(

src/commonMain/kotlin/dev/hermannm/devlog/Logger.kt

@@ -178,8 +178,14 @@ internal constructor(
    *
    *   The [LogBuilder] receiver lets you call [field][LogBuilder.field] in the scope of the lambda,
    *   to add structured key-value data to the log.
+   *
+   *   We mark the lambda as `crossinline`, so you don't accidentally do a non-local return in it,
+   *   which would drop the log.
    */
-  public inline fun info(cause: Throwable? = null, buildLog: LogBuilder.() -> String) {
+  public inline fun info(
+      cause: Throwable? = null,
+      crossinline buildLog: LogBuilder.() -> String,
+  ) {
     if (isInfoEnabled) {
       log(LogLevel.INFO, cause, buildLog)
     }
@@ -225,8 +231,14 @@ internal constructor(
    *
    *   The [LogBuilder] receiver lets you call [field][LogBuilder.field] in the scope of the lambda,
    *   to add structured key-value data to the log.
+   *
+   *   We mark the lambda as `crossinline`, so you don't accidentally do a non-local return in it,
+   *   which would drop the log.
    */
-  public inline fun warn(cause: Throwable? = null, buildLog: LogBuilder.() -> String) {
+  public inline fun warn(
+      cause: Throwable? = null,
+      crossinline buildLog: LogBuilder.() -> String,
+  ) {
     if (isWarnEnabled) {
       log(LogLevel.WARN, cause, buildLog)
     }
@@ -272,8 +284,14 @@ internal constructor(
    *
    *   The [LogBuilder] receiver lets you call [field][LogBuilder.field] in the scope of the lambda,
    *   to add structured key-value data to the log.
+   *
+   *   We mark the lambda as `crossinline`, so you don't accidentally do a non-local return in it,
+   *   which would drop the log.
    */
-  public inline fun error(cause: Throwable? = null, buildLog: LogBuilder.() -> String) {
+  public inline fun error(
+      cause: Throwable? = null,
+      crossinline buildLog: LogBuilder.() -> String,
+  ) {
     if (isErrorEnabled) {
       log(LogLevel.ERROR, cause, buildLog)
     }
@@ -315,8 +333,14 @@ internal constructor(
    *
    *   The [LogBuilder] receiver lets you call [field][LogBuilder.field] in the scope of the lambda,
    *   to add structured key-value data to the log.
+   *
+   *   We mark the lambda as `crossinline`, so you don't accidentally do a non-local return in it,
+   *   which would drop the log.
    */
-  public inline fun debug(cause: Throwable? = null, buildLog: LogBuilder.() -> String) {
+  public inline fun debug(
+      cause: Throwable? = null,
+      crossinline buildLog: LogBuilder.() -> String,
+  ) {
     if (isDebugEnabled) {
       log(LogLevel.DEBUG, cause, buildLog)
     }
@@ -358,8 +382,14 @@ internal constructor(
    *
    *   The [LogBuilder] receiver lets you call [field][LogBuilder.field] in the scope of the lambda,
    *   to add structured key-value data to the log.
+   *
+   *   We mark the lambda as `crossinline`, so you don't accidentally do a non-local return in it,
+   *   which would drop the log.
    */
-  public inline fun trace(cause: Throwable? = null, buildLog: LogBuilder.() -> String) {
+  public inline fun trace(
+      cause: Throwable? = null,
+      crossinline buildLog: LogBuilder.() -> String,
+  ) {
     if (isTraceEnabled) {
       log(LogLevel.TRACE, cause, buildLog)
     }
@@ -409,17 +439,36 @@ internal constructor(
    *
    *   The [LogBuilder] receiver lets you call [field][LogBuilder.field] in the scope of the lambda,
    *   to add structured key-value data to the log.
+   *
+   *   We mark the lambda as `crossinline`, so you don't accidentally do a non-local return in it,
+   *   which would drop the log.
    */
   public inline fun at(
       level: LogLevel,
       cause: Throwable? = null,
-      buildLog: LogBuilder.() -> String
+      crossinline buildLog: LogBuilder.() -> String,
   ) {
     if (isEnabledFor(level)) {
       log(level, cause, buildLog)
     }
   }
 
+  @PublishedApi
+  internal inline fun log(
+      level: LogLevel,
+      cause: Throwable?,
+      crossinline buildLog: LogBuilder.() -> String,
+  ) {
+    val builder = LogBuilder(createLogEvent(level, cause, underlyingLogger))
+    val message = builder.buildLog()
+    if (cause != null) {
+      // Call this after buildLog(), so cause exception fields don't overwrite LogBuilder fields
+      builder.addFieldsFromCauseException(cause)
+    }
+
+    builder.logEvent.log(message, underlyingLogger)
+  }
+
   /**
    * Returns true if [LogLevel.ERROR] is enabled for this logger.
    *
@@ -486,18 +535,6 @@ internal constructor(
         TRACE = { isTraceEnabled },
     )
   }
-
-  @PublishedApi
-  internal inline fun log(level: LogLevel, cause: Throwable?, buildLog: LogBuilder.() -> String) {
-    val builder = LogBuilder(createLogEvent(level, cause, underlyingLogger))
-    val message = builder.buildLog()
-    if (cause != null) {
-      // Call this after buildLog(), so cause exception fields don't overwrite LogBuilder fields
-      builder.addFieldsFromCauseException(cause)
-    }
-
-    builder.logEvent.log(message, underlyingLogger)
-  }
 }
 
 /**
@@ -508,11 +545,11 @@ internal constructor(
 internal expect interface PlatformLogger {
   fun getName(): String
 
-  fun isInfoEnabled(): Boolean
+  fun isErrorEnabled(): Boolean
 
   fun isWarnEnabled(): Boolean
 
-  fun isErrorEnabled(): Boolean
+  fun isInfoEnabled(): Boolean
 
   fun isDebugEnabled(): Boolean
 

src/commonTest/kotlin/dev/hermannm/devlog/LoggerTest.kt

@@ -311,29 +311,6 @@ internal class LoggerTest {
     disabledLogger.isErrorEnabled.shouldBeFalse()
     disabledLogger.isEnabledFor(LogLevel.ERROR).shouldBeFalse()
   }
-
-  @Test
-  fun `non-local return works in all logger methods`() {
-    // This won't compile if the methods aren't inline, and we want to verify that
-    log.info {
-      return
-    }
-    log.warn {
-      return
-    }
-    log.error {
-      return
-    }
-    log.debug {
-      return
-    }
-    log.trace {
-      return
-    }
-    log.at(LogLevel.INFO) {
-      return
-    }
-  }
 }
 
 private val loggerOutsideClass = getLogger()

src/jvmTest/kotlin/dev/hermannm/devlog/LoggerTest.jvm.kt

@@ -16,6 +16,7 @@ import io.kotest.matchers.date.shouldBeBetween
 import io.kotest.matchers.nulls.shouldBeNull
 import io.kotest.matchers.shouldBe
 import io.kotest.matchers.types.shouldBeInstanceOf
+import java.lang.invoke.MethodHandles
 import java.time.Instant
 import kotlin.test.AfterTest
 import kotlin.test.Test
@@ -98,8 +99,8 @@ internal class LoggerJvmTest {
   companion object {
     /** We use a ListAppender from Logback here so we can inspect log events after logging. */
     val logAppender = ListAppender<ILoggingEvent>()
-    const val TEST_LOGGER_NAME = "LoggerTest"
-    val logbackLogger = Slf4jLoggerFactory.getLogger(TEST_LOGGER_NAME) as LogbackLogger
+    val logbackLogger = Slf4jLoggerFactory.getLogger("LoggerTest") as LogbackLogger
+    val log = Logger(logbackLogger)
 
     init {
       logAppender.start()
@@ -146,4 +147,35 @@ internal class LoggerJvmTest {
   fun `Logback is loaded in tests`() {
     LOGBACK_IS_ON_CLASSPATH shouldBe true
   }
+
+  @Test
+  fun `lambda arguments to logger methods are inlined`() {
+    // We verify that the lambdas are inlined by calling `lookupClass()` inside of them.
+    // If they are inlined, then the calling class should be this test class - otherwise, the
+    // calling class would be a generated class for the lambda.
+    log.info {
+      MethodHandles.lookup().lookupClass() shouldBe LoggerJvmTest::class.java
+      "Test"
+    }
+    log.warn {
+      MethodHandles.lookup().lookupClass() shouldBe LoggerJvmTest::class.java
+      "Test"
+    }
+    log.error {
+      MethodHandles.lookup().lookupClass() shouldBe LoggerJvmTest::class.java
+      "Test"
+    }
+    log.debug {
+      MethodHandles.lookup().lookupClass() shouldBe LoggerJvmTest::class.java
+      "Test"
+    }
+    log.trace {
+      MethodHandles.lookup().lookupClass() shouldBe LoggerJvmTest::class.java
+      "Test"
+    }
+    log.at(LogLevel.INFO) {
+      MethodHandles.lookup().lookupClass() shouldBe LoggerJvmTest::class.java
+      "Test"
+    }
+  }
 }