1199: Error when as_forecast_<type>() renaming would create duplicate columns

[nikosbosse]

Description

This PR closes #1199.

as_forecast_generic() called data.table::setnames() without checking whether a rename target already existed as a column in the data. Calling e.g. as_forecast_binary(dt, predicted = "prob") while a stale predicted column was present silently created a forecast object with two predicted columns. That object passed assert_forecast() and was then scored on the wrong (stale) column, producing silently wrong scores. All eight as_forecast_<type>() constructors route through this function.

The fix (per the settled maintainer decision — error, no silent overwrite, no deprecation period):

• as_forecast_generic() now errors with a clear cli_abort() message when a rename would collide with an existing column that is not itself being renamed away. Identity renames (observed = "observed") and simultaneous swaps of renameable columns still work.
• Defense in depth: assert_forecast_generic() now rejects data with duplicate column names, so corrupt objects reached by any other construction route fail validation instead of passing silently.

Tests were written first and failed against the unfixed code:

── 1. Failure ('test-class-forecast.R:16:3'): as_forecast_generic() errors when
Expected `as_forecast_binary(dt, predicted = "prob")` to throw a error.

── 2. Failure ('test-class-forecast.R:30:3'): as_forecast_generic() errors when
Expected `as_forecast_quantile(quantile_dt, quantile_level = "q")` to throw a error.

── 3. Failure ('test-class-forecast.R:93:3'): assert_forecast_generic() errors o
Expected `assert_forecast_generic(dt)` to throw a error.

With the fix, the affected test files and the full test suite pass (0 failed, 910 passed, 0 skipped).

Dev note: bug identified by an LLM audit (#1189); fix and tests implemented with LLM support, directed by @nikosbosse.

Checklist

• My PR is based on a package issue and I have explicitly linked it.
• I have included the target issue or issues in the PR title as follows: issue-number: PR title
• I have tested my changes locally.
• I have added or updated unit tests where necessary.
• I have updated the documentation if required (N/A — only an internal @importFrom roxygen tag was added; devtools::document() produced no changes to man/ or NAMESPACE).
• I have built the package locally and run rebuilt docs using roxygen2 (no doc changes resulted).
• My code follows the established coding standards and I have run lintr on the changed files; no new lints were introduced.
• I have added a news item linked to this PR.
• I have reviewed CI checks for this PR and addressed them as far as I am able (CI pending at time of opening; will be monitored).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.17%. Comparing base (06ed56d) to head (2bbbe8e).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1212      +/-   ##
==========================================
+ Coverage   98.15%   98.17%   +0.01%     
==========================================
  Files          41       41              
  Lines        2225     2244      +19     
==========================================
+ Hits         2184     2203      +19     
  Misses         41       41              

☔ View full report in Codecov by Harness.
📢 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.

[nikosbosse]

Automated review (Claude Fable, directed by @nikosbosse):

Verdict: approve.

Verified independently in an isolated checkout of the branch (7a93d80):

• Fail-before/pass-after confirmed. With the branch's tests but main's R/, exactly the 3 new expectations fail (no error raised on rename collision; assert_forecast_generic() silently accepts duplicate columns). With the fix restored, the changed file and the full suite pass: 0 failed, 910 passed, 0 errors, 0 skipped.
• Edge cases probed beyond the PR's tests, all correct: original repro (stale predicted + prob) errors clearly and leaves the input data.table unmutated; two simultaneous collisions render a correctly pluralised cli message; swapping observed/predicted still works; stale observed collision caught; NULL rename args mixed with a collision handled; sample_id collision caught; minimal 1-row input unaffected; corrupt duplicate-column objects from other routes now rejected by assert_forecast_generic().
• Brief compliance: matches the settled maintainer decision (error via cli_abort, no silent overwrite, no deprecation; defense-in-depth check in assert_forecast_generic()). Identity renames preserved as required.
• Scope, NEWS, title: diff touches only R/class-forecast.R, the test file, and NEWS. NEWS entry accurate and references as_forecast_*() renaming onto an existing column creates duplicate columns and silently wrong scores #1199. Title follows the NNNN: title convention. importFrom(cli, cli_abort) already present in NAMESPACE, so no doc changes needed — checklist claims check out.

Minor (non-blocking): the collision error names only the target column (e.g. predicted), not the source column being renamed; including both could speed up debugging, but the message is clear as-is.

No must-fix findings.

[nikosbosse]

We're now enforcing unique column names, which is a slight behaviour change, but I think it's a good one

[nikosbosse]

Follow-up (Claude Fable, directed by @nikosbosse): addressed the review's minor finding — the rename collision error in as_forecast_generic() now names both the source column being renamed and the existing target column, instead of only the target.

Before:

! Cannot rename to "predicted": a column with this name already exists in the data.

After:

! Cannot rename column "prob" to "predicted": a column with this name already exists in the data.
i Rename or remove the existing column first.

Pluralisation via cli is preserved for multiple simultaneous collisions ('Cannot rename columns "obs2" and "pred2" to "observed" and "predicted": columns with these names already exist in the data.'), and the tests now assert that both the source and target names appear in the message, including a new multi-collision case.

Verified: devtools::test(filter = "class-forecast") 0 failed / 279 passed; full suite 0 failed / 914 passed; CI-equivalent lintr::lint_package() on the changed files reports no lints.

Commit: 2bbbe8e