Skip to content

Correctly split line endings for // @testOption: value parsing - #62987

Merged
Daniel Rosenwasser (DanielRosenwasser) merged 5 commits into
mainfrom
correctlySplitLineEndingsInHarness
Jan 15, 2026
Merged

Correctly split line endings for // @testOption: value parsing#62987
Daniel Rosenwasser (DanielRosenwasser) merged 5 commits into
mainfrom
correctlySplitLineEndingsInHarness

Conversation

@DanielRosenwasser

@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser) commented Jan 14, 2026

Copy link
Copy Markdown
Member

We have a handful of tests that have mixed line endings - a combination of \r\nand\n (and who knows, maybe someone added standalone \rs in there too).

Our test harness tries to split by \r\n first, and then by \n if that doesn't produce multiple lines. It's documented as a hack for Internet Explorer/JScript I guess, but we haven't run on wscript/cscript in at least half a decade.

But with this strategy, what this means is that if you have a comment like // @option: value\n where a \r\n appears anywhere in the file, the option is (possibly?) lost and you will accidentally have some test contents swallowed up in a comment.

This was discovered while trying to fix#62333 - I had a script that added "// @strict: false" + firstNewlineOfFile and that caused some changes in JavaScript emit where the file contents were missing. Somewhere else in the file, we had a \r\n which caused the issue mentioned above.

This change just splits across the reasonable set of line terminators - arguably we should do line separator (U+2028) and paragraph separator (U+2029) too, but I guess we never touched those until now anyway.

Comment threadsrc/harness/harnessUtils.ts Outdated

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 fixes a parsing issue in the test harness where test option comments like // @declaration: true were being incorrectly parsed when files contained mixed line endings. The old implementation tried to split by \r\n first, then by \n if that didn't work, which could cause options with \n to be swallowed when \r\n appeared elsewhere in the file. The fix uses a regex pattern /\r\n?|\n/ to properly split on all reasonable line terminators.

Changes:

  • Replaced conditional string splitting logic with a single regex-based split that handles all line ending types (\r\n, \r, \n)
  • Updated baseline test files to reflect correct line numbering after the fix

Reviewed changes

Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
FileDescription
src/harness/harnessUtils.tsRefactored splitContentByNewlines to use regex pattern instead of conditional string splitting
tests/baselines/reference/sourceMap-LineBreaks.typesUpdated line numbers in baseline due to correct line splitting
tests/baselines/reference/sourceMap-LineBreaks.sourcemap.txtUpdated line numbers in baseline due to correct line splitting
tests/baselines/reference/sourceMap-LineBreaks.js.mapUpdated source map baseline due to correct line splitting
tests/baselines/reference/sourceMap-LineBreaks.jsUpdated emitted JS baseline with correct line breaks
tests/baselines/reference/genericArray0.typesRemoved erroneous blank line in baseline
tests/baselines/reference/genericArray0.symbolsCorrected symbol declaration line numbers
tests/baselines/reference/genericArray0.jsRemoved erroneous blank line in baseline
tests/baselines/reference/decoratedClassExportsCommonJS1.typesAdded missing content that was previously swallowed
tests/baselines/reference/decoratedClassExportsCommonJS1.symbolsCorrected file name and line numbers
tests/baselines/reference/decoratedClassExportsCommonJS1.jsCorrected file name in baseline

Comment threadsrc/harness/harnessUtils.ts Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

var stringLiteralWithLineFeed = "line 1\
line 2";
var stringLiteralWithCarriageReturnLineFeed = "line 1\
line 2";
var stringLiteralWithCarriageReturn = "line 1\line 2";
var stringLiteralWithCarriageReturn = "line 1\

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Maybe sticking with \r?\n would be less...weird.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The test was never actually testing what it thought it was testing, and it sort of still isn't, so I don't know how to square that. I think the new behavior is probably worse in that our tests normalize /\r\n?/ to \n, so we've lost some test coverage.

So I'll switch it back to the regex every normal person would assume.

@jakebaileyJake Bailey (jakebailey) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does corsa do the bad thing here too, accidentally?

@github-project-automationgithub-project-automationBot moved this from Not started to Needs merge in PR BacklogJan 14, 2026
@DanielRosenwasser

Copy link
Copy Markdown
MemberAuthor

Corsa does

varlineDelimiter=regexp.MustCompile("\r?\n")

@DanielRosenwasser

Copy link
Copy Markdown
MemberAuthor

for(constfileofallFiles){
const{ unitName }=file;
lettypeLines="=== "+unitName+" ===\r\n";
constcodeLines=ts.flatMap(file.content.split(/\r?\n/),e=>e.split(/[\r\u2028\u2029]/));

I guess it's somewhat intentional that we normalize all CRLF into LF but not the other ones.

Merged via the queue into main with commit a9f534fJan 15, 2026
33 checks passed
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) deleted the correctlySplitLineEndingsInHarness branch January 15, 2026 03:25
@github-project-automationgithub-project-automationBot moved this from Needs merge to Done in PR BacklogJan 15, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: TeamFor Milestone BugPRs that fix a bug with a specific milestone

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Enable --strict by default

6 participants

@DanielRosenwasser@jakebailey@RyanCavanaugh@andrewbranch@typescript-bot