Skip to content

Update contains method to include start position (#34)#37

Merged
mattmassicotte merged 2 commits into
ChimeHQ:mainfrom
tothambrus11:patch-1
May 26, 2026
Merged

Update contains method to include start position (#34)#37
mattmassicotte merged 2 commits into
ChimeHQ:mainfrom
tothambrus11:patch-1

Conversation

@tothambrus11

Copy link
Copy Markdown
Contributor

Fixes #34

@tothambrus11 tothambrus11 mentioned this pull request May 7, 2026
@mattmassicotte

Copy link
Copy Markdown
Contributor

Ahh yes, this is a tricky one. Ranges are always subtle. I was planning on adding some testing around this to make sure I understood what "correct" was. What do you think about that?

@tothambrus11

tothambrus11 commented May 7, 2026

Copy link
Copy Markdown
Contributor Author

Ranges are half-open, as specified by the LSP. When implementing language servers to resolve a node under a cursor, we would typically not use this, but something that matches on both ends inclusively though. E.g. if we have an identifier x, the cursor should match on both sides of it.

import FrontEnd
import Logging

extension Program {

  /// - Requires: The source file of `position` is present in `self`.
  public func innermostTree(
    containing position: SourcePosition, reportingLogsTo logger: Logger, in f: SourceFile.ID
  ) -> AnySyntaxIdentity? {
    var v = NodeFinder(position)
    visit(topLevelDeclarations(in: f), calling: &v)
    return v.deepestMatch
  }

}

/// Requires that the visiting happens in a depth-first order.
private struct NodeFinder: SyntaxVisitor {

  // todo use binary search for efficiency if we can assume AST entries are sorted by position (probably they aren't though)

  let targetPosition: SourcePosition
  private(set) var deepestMatch: AnySyntaxIdentity?
  private var deepestMatchDepth: Int = -1
  private var currentDepth: Int = 0

  public init(_ targetPosition: SourcePosition) {
    self.targetPosition = targetPosition
  }

  mutating func willEnter(_ n: AnySyntaxIdentity, in program: Program) -> Bool {
    if program[n].site.region.containsInclusive(targetPosition.index) {
      if currentDepth > deepestMatchDepth {
        deepestMatchDepth = currentDepth
        deepestMatch = n
      }
    } else {
      if program.isScope(n) {
        // If it's a scope, we know its children's sites are strictly subsumed, so we can skip them.
        return false
      }
    }

    currentDepth += 1

    return true  // continue visiting children
  }

  public mutating func willExit(_ node: AnySyntaxIdentity, in program: Program) {
    currentDepth -= 1
  }

}

extension Range {

  func containsInclusive(_ i: Bound) -> Bool {
    return self.lowerBound <= i && i <= self.upperBound
  }

}

@mattmassicotte

Copy link
Copy Markdown
Contributor

I think this is very reasonable. Sorry for keeping you waiting. Let's just go for it.

@mattmassicotte mattmassicotte merged commit 5625146 into ChimeHQ:main May 26, 2026
4 checks passed
@tothambrus11

Copy link
Copy Markdown
Contributor Author

Thanks you very much! 🚀

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.

LSPRange contains method is not inclusive at the start

2 participants