Distinguish lookup failures from genuine not-found#61
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves debuggability of symbol resolution by distinguishing “symbol genuinely not found” from “lookup failed due to an unexpected tastyquery/classpath exception”, and surfaces those failures via a dedicated error type and targeted debug logging.
Changes:
- Add
LookupResult.LookupFailed(cause: Throwable)and propagate it out of top-level symbol resolution. - Add
CellarError.SymbolLookupFailedand raise it fromget/get-sourcehandlers on lookup failures. - Log unexpected classpath-scan exceptions at
FINElevel inNearMatchFinderandAllSymbolsStream.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/src/cellar/handlers/GetSourceHandler.scala | Raises SymbolLookupFailed when resolution returns LookupFailed. |
| lib/src/cellar/handlers/GetHandler.scala | Raises SymbolLookupFailed when resolution returns LookupFailed. |
| lib/src/cellar/SymbolResolver.scala | Introduces LookupFailed and captures first unexpected exception during top-level lookup. |
| lib/src/cellar/NearMatchFinder.scala | Adds FINE-level logging when classpath entry scanning throws. |
| lib/src/cellar/CellarError.scala | Adds SymbolLookupFailed error with a human-readable message. |
| lib/src/cellar/AllSymbolsStream.scala | Adds FINE-level logging when classpath entry scanning throws. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try Some(thunk) | ||
| catch | ||
| case _: MemberNotFoundException => None | ||
| case e: Exception => |
There was a problem hiding this comment.
LookupResult.LookupFailed stores a Throwable, but the helper t only catches Exception. If tastyquery/classpath resolution throws a non-Exception (e.g., some Error/Throwable that you still want to surface as a lookup failure), it will bypass this logic and crash instead of returning LookupFailed. Consider catching scala.util.control.NonFatal (or aligning the LookupFailed payload type to Exception) so the behavior matches the intent.
| case e: Exception => | |
| case scala.util.control.NonFatal(e) => |
| t(ctx.findStaticClass(fqn)).map(s => LookupResult.Found(List(s))) | ||
| .orElse(t(ctx.findStaticModuleClass(fqn)).map(s => LookupResult.Found(List(s)))) | ||
| .orElse(t(ctx.findStaticTerm(fqn)).map(s => LookupResult.Found(List(s)))) | ||
| .orElse(t(ctx.findStaticType(fqn)).map(s => LookupResult.Found(List(s)))) | ||
| .orElse(t(ctx.findPackage(fqn)).map(_ => LookupResult.IsPackage)) | ||
| .orElse(caught.map(LookupResult.LookupFailed(_))) |
There was a problem hiding this comment.
resolve can fall back to tryNestedLookup, but that path still relies on tryOrNone (e.g., in findTopLevelRoot) which swallows all non-MemberNotFoundException exceptions. As a result, unexpected tastyquery failures during nested resolution will still present as NotFound/PartialMatch instead of LookupFailed. To fully address #55, propagate the first unexpected exception through the nested lookup path as well (similar to what you did in tryTopLevel).
| @@ -30,6 +30,8 @@ object GetSourceHandler: | |||
| NearMatchFinder.findNearMatches(fqn, classpath).flatMap { nearMatches => | |||
| IO.raiseError(CellarError.SymbolNotFound(fqn, coord, nearMatches)) | |||
| } | |||
There was a problem hiding this comment.
This match is still non-exhaustive: SymbolResolver.resolve can return LookupResult.PartialMatch, but GetSourceHandler doesn’t handle it. That will result in a MatchError and an unhelpful error message for inputs like Foo.nonExistentMember. Add a PartialMatch case (likely raising CellarError.PartialResolution, similar to GetHandler).
| } | |
| } | |
| case partialMatch: LookupResult.PartialMatch => | |
| IO.raiseError(CellarError.PartialResolution(fqn, partialMatch)) |
| .flatMap(entry => try ctx.findSymbolsByClasspathEntry(entry).toList catch | ||
| case e: Throwable => | ||
| if log.isLoggable(java.util.logging.Level.FINE) then | ||
| log.fine(s"Unexpected exception scanning classpath entry: ${e.getClass.getName}: ${e.getMessage}") |
There was a problem hiding this comment.
The new debug log drops key context and the throwable itself: it doesn’t include the entry being scanned and it logs only e.getMessage (which can be null) without a stack trace. For diagnosability, include the classpath entry in the message and log the exception (e.g., via log.log(Level.FINE, msg, e)) while keeping the isLoggable guard.
| log.fine(s"Unexpected exception scanning classpath entry: ${e.getClass.getName}: ${e.getMessage}") | |
| log.log(java.util.logging.Level.FINE, s"Unexpected exception scanning classpath entry: $entry", e) |
| catch | ||
| case e: Throwable => | ||
| if log.isLoggable(java.util.logging.Level.FINE) then | ||
| log.fine(s"Unexpected exception scanning classpath entry: ${e.getClass.getName}: ${e.getMessage}") |
There was a problem hiding this comment.
Same logging issue here as in NearMatchFinder: the log message doesn’t identify the failing entry and omits the throwable/stack trace (and e.getMessage may be null). Logging the ClasspathEntry and the exception object (e.g., log.log(Level.FINE, msg, e)) will make classpath-scan failures actionable while still remaining FINE-level.
| log.fine(s"Unexpected exception scanning classpath entry: ${e.getClass.getName}: ${e.getMessage}") | |
| log.log(java.util.logging.Level.FINE, s"Unexpected exception scanning classpath entry: $entry", e) |
| case object IsPackage extends LookupResult | ||
| case object NotFound extends LookupResult | ||
| final case class PartialMatch(resolvedFqn: String, missingMember: String) extends LookupResult | ||
| final case class LookupFailed(cause: Throwable) extends LookupResult |
There was a problem hiding this comment.
New LookupFailed behavior isn’t covered by tests. There are existing SymbolResolverTest and CellarErrorTest suites, so it would be good to add coverage that (1) an unexpected tastyquery exception is translated into LookupResult.LookupFailed, and (2) the surfaced CellarError.SymbolLookupFailed message includes the exception type and detail while genuine misses still return NotFound.
Fixes #55.
Summary
LookupResult.LookupFailed(cause: Throwable)— a distinct result for when a tastyquery lookup throws an unexpected exception, separate fromNotFound(symbol genuinely absent)CellarError.SymbolLookupFailedwith a human-readable message surfacing the exception type and detailtryTopLevelnow captures the first unexpected exception and returnsLookupFailedinstead of silently falling through toNotFoundGetHandler,GetSourceHandler) raiseSymbolLookupFailedon this new caseNearMatchFinderandAllSymbolsStreamlog unexpected classpath-scan exceptions at FINE level (withisLoggableguard to avoid string allocation when debug logging is off)Before / After
Before:
cellar get-external <coord> SpanData$→Symbol 'SpanData$' not found(ambiguous — could be a genuine miss or a swallowed exception)After: unexpected tastyquery exceptions produce
Symbol lookup for 'SpanData$' failed unexpectedly: SomeException: detail, while genuine misses still produceNotFound.