fix: disable conflicting IfBraceChecker

[AshishGhodvinde]

Which issue does this PR close?

Closes #5711

Rationale for this change

make format uses Scalafmt, which can wrap long brace-less if/else expressions across multiple lines without adding braces. Scalastyle's IfBraceChecker then rejects the formatted code.

Since Scalafmt is the canonical formatter and cannot automatically add braces, the IfBraceChecker rule can conflict with the formatter and leave code that fails Scalastyle after formatting.

What changes are included in this PR?

• Disabled IfBraceChecker in dev/scalastyle-config.xml.
• No Scala source files were changed.

How are these changes tested?

• Ran Scalastyle directly against the Spark Scala sources:
  mvnw.cmd scalastyle:check -pl spark -Dscalastyle.failOnViolation=true -Dscalastyle.verbose=true
• Scalastyle scanned 153 Scala source files and reported no IfBraceChecker violations.
• The check still reports an unrelated existing Class.forName violation in spark/src/main/scala/org/apache/spark/sql/comet/util/Utils.scala:131.
• A full test-compile could not complete locally because of unrelated missing generated protobuf/dependency classes.

[AshishGhodvinde]

Heyy, I've completed the implementation for this issue and opened this PR with the proposed fix.

The change is limited to disabling the conflicting IfBraceChecker in dev/scalastyle-config.xml. The PR is ready for review, and the CI workflows are currently awaiting maintainer approval.

Please let me know if any changes are needed. Thankss!

[comphead]

Thanks @AshishGhodvinde lets consider disable as a last resort. Can we investigate if formatter itself is able to wrap multi line ifs into brackets?

[sunchao]

Correctness

Reviewed 20a99c01 against authoritative base a88288b6. This changes only dev/scalastyle-config.xml: it disables IfBraceChecker and adds a comment explaining the formatting conflict. With the current configuration, Scalafmt can move an unbraced branch body onto a new line, which Scalastyle then rejects. Disabling the checker removes that failure, but also stops checking other unbraced next-line bodies.

The XML is valid. I verified the exact Scalastyle 1.0.0 configuration loader and ran the checker with both configurations. Only one of the 59 configured checks changes, reducing enabled checks from 45 to 44. Other rules and their parameters remain identical. Negative fixtures still produce the same Class.forName and println violations.

The authored change contains no Scala/Rust implementation, expression, operator or serialization change. Spark's values, nulls, ANSI errors, overflow and fallback semantics are unaffected by this lint configuration. Maintained Spark 3.4/4.1 source gaps remain recorded but are not needed to establish that scope. The branch is one commit behind the authoritative base. CI's merge preserves that base-only shuffle-metrics change.

Validation and CI

I ran the pinned Scalafmt 3.6.1 and Scalastyle 1.0.0 on eight focused fixtures. All 24 formatted outputs were stable on a second formatting pass. Both old and new checker configurations also passed a direct scan of the head's 153 main Scala files. These are real formatter/checker component runs, not a full Maven build or Spark/native execution.

At 2026-09-05 21:46 UTC, the refreshed head checks report 1 neutral, 12 skipped, 4 successful, 1 cancelled. All checks had completed at that cutoff. The Linux/macOS build matrices were skipped because the existing path filters do not select dev/scalastyle-config.xml. The successful umbrella CI result therefore does not establish that those lint/build jobs ran. Preflight, change detection and Delta build-gate logs identify merge 04054b3e, whose parents are the authoritative base a88288b6 and head 20a99c01. All 11 relevant configuration/build blobs match the head. No workflow approval or rerun was performed.

Performance

This affects development-time linting, with no authored runtime hot-path change. Removing one checker avoids its lint work, but no meaningful build-time improvement was measured or claimed. A query microbenchmark would not validate this configuration fix.

Design

Existing concern: preserve brace enforcement if the formatter can satisfy it

[P2] I verified the alternative raised in comphead's review. The pinned formatter does support brace insertion: Scalafmt 3.6.1 exposes rewrite.insertBraces.minLines. Adding rewrite.insertBraces.minLines = 1 to a copy of the current formatter config fixed the long-if, long-else and next-line single-statement fixtures while the existing IfBraceChecker remained enabled. The default formatter produced all three violations. A threshold of 2 fixed the long branches but left the single-statement case.

Could we evaluate that supported option before dropping the brace policy? It needs a formatting-scope check because it also adds method-body braces in the long fixtures. Also, make format currently invokes compile/test-compile before spotless:apply, and Scalastyle is bound to compile. An already failing tree may therefore need standalone formatting first or an explicit ordering adjustment. I have not claimed that the setting alone makes the entire format target repair every input. This adds evidence to the existing discussion, with no duplicate inline comment.

Abstraction & complexity

The proposed boolean change is simple and introduces no new abstraction, dependency or exception mechanism. Its tradeoff is broad removal of one existing style policy. A supported formatter setting may preserve that policy without a custom rewrite or scattered suppressions, subject to the scope and command-order checks above. No additional complexity finding.

[AshishGhodvinde]

Thanks for the suggestion! I looked into rewrite.insertBraces.minLines using the pinned Scalafmt 3.6.1 version.

I tested both minLines = 1 and minLines = 2 with IfBraceChecker still enabled.

What I found

• minLines = 1 does fix the IfBraceChecker violations, but it causes quite a lot of unrelated formatting changes: 295 Scala files were modified, with around 6,158 lines of churn, and approximately 565 single-expression method bodies gained braces.
• minLines = 2 reduces the churn somewhat (249 files, around 4,166 lines), but it still leaves 4 IfBraceChecker violations when scanning all relevant main, test, and Spark-version source directories.

The minLines = 2 violations are cases where Scalafmt wraps a single-statement if/else branch across a line break but doesn't insert braces.

I also found that my initial minLines = 2 check was incomplete because the default Scalastyle Maven configuration doesn't cover all of the repository's test and Spark-version source directories. I reran the check explicitly across those directories and reproduced the 4 violations.

So after testing this more broadly, neither minLines = 1 nor minLines = 2 seems like a good fit without introducing significant formatting churn or, in the case of minLines = 2, still leaving the original issue unresolved.

I haven't changed the PR implementation yet. For now, the IfBraceChecker change remains as-is.

[sunchao]

Correctness

Re-reviewed unchanged head 20a99c01 against base a88288b6 after the follow-up investigation. Fresh runs with Scalafmt 3.6.1 and Scalastyle 1.0.0 covered nine focused fixtures and 27 idempotent formatted outputs. Disabling IfBraceChecker removes the reproduced brace failures; the Class.forName and println negative controls remain identical. No remaining P1/P2 correctness finding.

These are component checks. No full Maven or Spark/native run was performed. At 2026-09-08 01:28 UTC, Linux/macOS build checks remain skipped, and three newer CI runs require maintainer action; the earlier successful umbrella result does not provide full build coverage.

Performance

The broader investigation reports 295 changed files, 6,158 lines of churn and 565 method bodies gaining braces with minLines = 1; minLines = 2 still changes 249 files and leaves four violations. Those repository-wide counts are author-reported, not independently reproduced here. The focused runs independently confirm both the unrelated method-body changes and the residual next-line branch failure at threshold 2. No runtime performance change is involved.

Design

I withdraw the P2 concern in my previous review. The requested formatting-scope evaluation has now been supplied. The pinned formatter's brace-insertion options expose minLines and allBlocks; method bodies remain eligible with allBlocks = false. That setting does not restrict insertion to conditional branches. The checker's existing singleLineAllowed and doubleLineAllowed options are already enabled, so they cannot provide a further relaxation.

Abstraction & complexity

Given the demonstrated limits of those alternatives, disabling this conflicting checker is a reasonable scoped change. A broad formatting rewrite or custom conditional-only rule is not necessary to unblock it. No remaining P1/P2 finding; approving this revision.