Skip to content

Distinguish lookup failures from genuine not-found#61

Merged
rochala merged 3 commits intomainfrom
worktree-issue-55
Apr 23, 2026
Merged

Distinguish lookup failures from genuine not-found#61
rochala merged 3 commits intomainfrom
worktree-issue-55

Conversation

@rochala
Copy link
Copy Markdown
Contributor

@rochala rochala commented Apr 22, 2026

Fixes #55.

Summary

  • Adds LookupResult.LookupFailed(cause: Throwable) — a distinct result for when a tastyquery lookup throws an unexpected exception, separate from NotFound (symbol genuinely absent)
  • Adds CellarError.SymbolLookupFailed with a human-readable message surfacing the exception type and detail
  • tryTopLevel now captures the first unexpected exception and returns LookupFailed instead of silently falling through to NotFound
  • Both handlers (GetHandler, GetSourceHandler) raise SymbolLookupFailed on this new case
  • NearMatchFinder and AllSymbolsStream log unexpected classpath-scan exceptions at FINE level (with isLoggable guard 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 produce NotFound.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.SymbolLookupFailed and raise it from get / get-source handlers on lookup failures.
  • Log unexpected classpath-scan exceptions at FINE level in NearMatchFinder and AllSymbolsStream.

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.

Comment thread lib/src/cellar/SymbolResolver.scala Outdated
try Some(thunk)
catch
case _: MemberNotFoundException => None
case e: Exception =>
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
case e: Exception =>
case scala.util.control.NonFatal(e) =>

Copilot uses AI. Check for mistakes.
Comment on lines +42 to +47
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(_)))
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.
@@ -30,6 +30,8 @@ object GetSourceHandler:
NearMatchFinder.findNearMatches(fqn, classpath).flatMap { nearMatches =>
IO.raiseError(CellarError.SymbolNotFound(fqn, coord, nearMatches))
}
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
}
}
case partialMatch: LookupResult.PartialMatch =>
IO.raiseError(CellarError.PartialResolution(fqn, partialMatch))

Copilot uses AI. Check for mistakes.
Comment thread lib/src/cellar/NearMatchFinder.scala Outdated
.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}")
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)

Copilot uses AI. Check for mistakes.
Comment thread lib/src/cellar/AllSymbolsStream.scala Outdated
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}")
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)

Copilot uses AI. Check for mistakes.
Comment on lines 12 to +15
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
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
@rochala rochala merged commit f9f6163 into main Apr 23, 2026
5 checks passed
@rochala rochala deleted the worktree-issue-55 branch April 23, 2026 10:15
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.

Symbol resolution silently swallows exceptions, masking lookup failures

2 participants