search

bazelbuild / rules_scala

Scala rules for Bazel
Apache License 2.0
360 stars 273 forks source link

Fix diagnostic outputs for Scala 2.12.13 #1532

Closed aishfenton closed 8 months ago

aishfenton commented 9 months ago

Description

Adds the correct overrides in DepsReporter to keep diagnostic reporting working in Scala 2.12.13 <= x < 2.13.12.

This is due to SemanticdbReporter calling doReport on it's underlying reporter, rather info0.

Motivation

Currently if you have SemanticDB enabled, and are using Scala 2.12.13 <= x < 2.13.12, you get empty diagnostic report protos.

Related to this issue: https://github.com/bazelbuild/rules_scala/issues/1484

eed3si9n commented 9 months ago

Is the current test coverage (https://github.com/bazelbuild/rules_scala/tree/master/test/shell has a bunch of shell scripts) missing the test for Scala 2.12.x on this issue?

aishfenton commented 9 months ago

@eed3si9n I think it doesn't test the combination of Semanticdb + Diagnostics. I can add something in to test that combo.

Harder to test is the combinations of < 2.12.13 and >= 2.12.13, as the repo's default dep_providers are for 2.12.18.

aishfenton commented 9 months ago

And cc @scoquelin, since this is a follow up to https://github.com/bazelbuild/rules_scala/pull/1522

aishfenton commented 9 months ago

@liucijus I'm adding tests for the cross-product of versions having semanticdb + diagnostics enabled. But I'm finding that having semanticdb enabled also changes the lines numbers that are output (I have no idea how... they're still correct just more similar to 2.13's way of doing it ... but not exactly — (╯°□°）╯︵ ┻━┻).

The cross product of (version X semanticdb x diagnosticReports) is getting a bit nuts to test for. I'm tempted to remove the lines numbers from DiagnosticsReporterTest and instead test for severity + error message. I'm thinking that the specific lines number outputs are just too unstable between plugins/scala-versions to test against.

How would you feel about that?

aishfenton commented 9 months ago

Added that change in to support cross-testing @liucijus, but let me know if you want to change it.