Skip to content

chore: Re-enable unused-parameter warnings in the strict-warnings profile #6164

Description

@andygrove

#5876 made -Pstrict-warnings pass on the Scala 2.12 profiles, but it left -Ywarn-unused:params out of both the main and the test flag lists. Most of what that lint reports are parameters a signature requires, not dead ones. That covers the @native declarations in Native.scala, the cross-version shims under src/main/spark-*, overridable defaults and serde helpers, and test fakes with fixed signatures. @nowarn can't silence them one at a time, because the set differs between Scala 2.12 and 2.13, so an annotation one profile needs is flagged as unused on the other.

-Wconf filters don't have that problem. In the #5876 thread, @athlcode measured that these two filters silence Native and every shim source on Scala 2.12.18:

-Wconf:cat=unused-params&site=org\.apache\.comet\.Native\..*:s,cat=unused-params&src=.*/src/[a-z]+/spark-[^/]+/.*:s

On -Pspark-3.5 at the time, that covered 96 of the 163 unused-parameter warnings. Of the other 67, eight are genuinely unused (four on private methods in main, four in tests) and can simply be removed. The remaining 59 fall into three groups:

  • Public extension points such as getSupportLevel and CometScanContrib.tryTransformV1.
  • Serde helpers such as QueryPlanSerde.createBinaryExpr, whose expr parameter is never read even though all 13 callers pass it.
  • Test fixtures, almost all of them fakes of Celeborn's client API.

The work:

  • Turn -Ywarn-unused:params back on in both executions of the profile, with the two filters above.
  • Remove the parameters that are genuinely unused.
  • Drop the unused expr parameter from createBinaryExpr and any other serde helper that carries it, and update the callers.
  • Cover what is left with per-site -Wconf filters, or change the signature where that is cleaner.
  • Rewrite the -Ywarn-unused:params paragraph of the POM comment to match. While there, fix the flag it names. It says unused @nowarn annotations are reported "via -Xlint:nowarn", but that is not a valid -Xlint choice on either 2.12 or 2.13. The warning comes from -Xlint:unused, which turns on -Ywarn-unused:nowarn.

One unrelated cleanup from the same review can go in the same PR. The strict-scala-warnings job in pr_build_linux.yml has its own Bootstrap Maven step. But since #5881, setup-builder runs maven-bootstrap as its last step, so the extra step just repeats it.

The Scala 2.13 profiles are tracked separately in #5893.

Done when ./mvnw test-compile -Pspark-3.5 -Pstrict-warnings -DskipTests passes with -Ywarn-unused:params enabled for both main and test sources.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area:ciCI/CD, GitHub Actions, build toolingenhancementNew feature or request

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions