Skip to content

Conversation

gabritto
Copy link
Member

@gabritto gabritto commented Oct 1, 2025

Renaming line map-related functions to make the distinction clear between ECMA and LSP lines.
ECMA line maps/line starts consider characters other than \n and \r\n as line breaks (see stringutil.IsLineBreak), unlike LSP.
This should help avoid future incorrect usages of those functions, and should make it easier to do #1578.

@gabritto gabritto requested review from Copilot and jakebailey October 1, 2025 17:23
Copy link
Contributor

@Copilot 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 renames line map-related functions to distinguish between ECMA line maps and LSP line maps. ECMA line maps consider additional characters (like various Unicode line breaks) as line separators beyond just \n and \r\n, while LSP uses only standard line terminators.

Key changes:

  • Renamed functions from GetLineAndCharacterOfPosition to GetECMALineAndCharacterOfPosition
  • Updated ComputeLineStarts to ComputeECMALineStarts
  • Changed LineMap interface methods to ECMALineMap

Reviewed Changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/transformers/utilities.go Updates function calls to use ECMA line position functions
internal/transformers/jsxtransforms/jsx.go Updates JSX transformer to use ECMA line positioning
internal/testutil/tsbaseline/*.go Updates baseline test utilities to use ECMA line functions
internal/sourcemap/*.go Renames LineInfo to ECMALineInfo and updates related functions
internal/scanner/scanner.go Renames core line mapping functions with ECMA prefix
internal/project/*.go Updates project files to use LSP line maps where appropriate
internal/ls/*.go Renames LineMap to LSPLineMap for language server components
internal/format/*.go Updates formatting code to use ECMA line functions
internal/printer/*.go Updates printer utilities to use ECMA line positioning
internal/fourslash/*.go Updates test framework to use LSP line maps
internal/diagnosticwriter/diagnosticwriter.go Updates diagnostic output to use ECMA line positioning
internal/core/core.go Renames core line computation functions with ECMA prefix
internal/compiler/program.go Updates compiler to use ECMA line functions
internal/checker/grammarchecks.go Updates grammar checking to use ECMA line positioning
internal/astnav/tokens_test.go Updates test to use ECMA line map
internal/ast/ast.go Renames SourceFile LineMap method to ECMALineMap

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Boy do I see a lot of stuff we need to rethink 😅


func GetIndentationForNode(n *ast.Node, ignoreActualIndentationRange *core.TextRange, sourceFile *ast.SourceFile, options *FormatCodeSettings) int {
startline, startpos := scanner.GetLineAndCharacterOfPosition(sourceFile, scanner.GetTokenPosOfNode(n, sourceFile, false))
startline, startpos := scanner.GetECMALineAndCharacterOfPosition(sourceFile, scanner.GetTokenPosOfNode(n, sourceFile, false))
Copy link
Member

Choose a reason for hiding this comment

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

The formatter using these instead of the client encoding is definitely spooky

lineMapMu sync.RWMutex
lineMap []core.TextPos
ecmaLineMapMu sync.RWMutex
ecmaLineMap []core.TextPos
Copy link
Member

Choose a reason for hiding this comment

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

Should we give []core.TextPos a name like ECMALineMap or does a change like that cause problems elsewhere?

@gabritto gabritto added this pull request to the merge queue Oct 1, 2025
Merged via the queue into main with commit 6277711 Oct 1, 2025
22 checks passed
@gabritto gabritto deleted the gabritto/renamelinemaps branch October 1, 2025 18:28
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.

3 participants