Skip to content

Polyfill: Remove dead code#3289

Merged
ptomato merged 1 commit intotc39:mainfrom
jessealama:temporal-coverage-gaps
Mar 5, 2026
Merged

Polyfill: Remove dead code#3289
ptomato merged 1 commit intotc39:mainfrom
jessealama:temporal-coverage-gaps

Conversation

@jessealama
Copy link
Collaborator

After digging into code coverage following tc39/test262#4964, we found several unreachable code paths in the polyfill's non-ISO calendar and duration rounding logic, even after flossing for edge cases.

We verified this on Node only, so it's possible some of these paths are reachable in other engines. Happy to revert any individual removal if that turns out to be the case.

@jessealama jessealama requested a review from ptomato March 3, 2026 16:08
@jessealama jessealama marked this pull request as draft March 3, 2026 16:08
@codecov
Copy link

codecov bot commented Mar 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.64%. Comparing base (be4e0ef) to head (b6d60f2).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3289      +/-   ##
==========================================
+ Coverage   97.40%   97.64%   +0.23%     
==========================================
  Files          22       22              
  Lines       10771    10746      -25     
  Branches     1866     1855      -11     
==========================================
+ Hits        10492    10493       +1     
+ Misses        258      235      -23     
+ Partials       21       18       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jessealama jessealama force-pushed the temporal-coverage-gaps branch from bfb52ce to 560ef50 Compare March 3, 2026 16:22
@jessealama jessealama marked this pull request as ready for review March 3, 2026 16:36
Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

Thanks!

Good in principle, but needs a few adjustments. There are a few places where I think it'd be good to keep an assertion, and a few places where it looks like the /* c8 ignore next */ magic comments aren't working as intended so maybe those can get a closer look.

There are a couple of deletions that don't look obviously deleteable to me, so would appreciate hearing your analysis.

Comment on lines -1011 to -1014
if (increment > 1) {
// If the estimate overshot the target, try again with a smaller increment
// in the reverse direction.
increment /= 2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing this looks wrong at first glance — as if it might be just happenstance that the bisection never overshoots in any of our tests.

(if removing it is correct, then the loop can be simplified a bit more — it looks like sign = 0 can become break, and the comment above the loop needs to be updated since it's no longer a bisection)

@jessealama
Copy link
Collaborator Author

Thanks!

Good in principle, but needs a few adjustments. There are a few places where I think it'd be good to keep an assertion, and a few places where it looks like the /* c8 ignore next */ magic comments aren't working as intended so maybe those can get a closer look.

There are a couple of deletions that don't look obviously deleteable to me, so would appreciate hearing your analysis.

Thanks for taking a look! The analysis of these was based on nothing deeper than looking at the coverage and noticing that they appear unused. I'm happy to take a more conservative approach and replace some of the deleted bits with assertions. I agree that some of these unused blocks are large enough to be better viewed as missing tests than as dead code. I'll take care of some of the lower-hanging fruit and try to turn those larger bits into tests. Until then, I'll mark this as a draft PR.

@jessealama jessealama marked this pull request as draft March 3, 2026 20:11
@ptomato
Copy link
Collaborator

ptomato commented Mar 5, 2026

@jessealama I realize now that Tim had looked at 2 of these and opened issues for them: #3235, #3217

@jessealama jessealama force-pushed the temporal-coverage-gaps branch 2 times, most recently from 9929b00 to c4d0d35 Compare March 5, 2026 11:05
@jessealama
Copy link
Collaborator Author

There are a couple of deletions that don't look obviously deleteable to me, so would appreciate hearing your analysis.

I've updated things a bit to use assertions in a couple places, and restored the code for the more aggressive deletions. Let's tackle those in separate issues.

@jessealama
Copy link
Collaborator Author

I'm marking this as ready for (re-)review. I made a couple of standalone PRs that track the aggressive deletions we originally had in this PR, thereby trimming down this PR.

@ptomato ptomato force-pushed the temporal-coverage-gaps branch from f205f00 to b6d60f2 Compare March 5, 2026 20:00
Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

Thanks!

@ptomato ptomato merged commit 13f6fab into tc39:main Mar 5, 2026
10 checks passed
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.

2 participants