add more scalac 2.12 warnings #6798

S11001001 · 2020-07-20T18:00:06Z

Sometimes codifying guidelines that have already come up in code review, sometimes checking probably-wrong constructs. I chose the lints to add based on tpolecat's 2.12 list, picking the ones that either I was familiar with or sounded good, leaving out a couple. Examples fixed here:

package.scala:18: warning: it is not recommended to define classes/objects inside of package objects.
If possible, define trait NoCopy in package data instead.
  trait NoCopy {
        ^

Note that package is itself a scoping construct, so if your reason is the apparent aesthetic of placing a bunch of things in one package object, that is easily remedied by simply deleting the object keyword, such as I do in c5e8c3b.

ContractsService.scala:197: warning: method searchDb in class ContractsService references private class ContractsFetch.
Classes which cannot access ContractsFetch may be unable to override searchDb.
  def searchDb(dao: dbbackend.ContractDao, fetch: ContractsFetch)(
      ^

This should be especially useful given modularization efforts such as @rautenrieth-da 's in #6695 and other PRs.

EventsTableFlatEventsRangeQueries.scala:11: warning: type parameter
 Offset defined in trait EventsTableFlatEventsRangeQueries shadows class
 Offset defined in package v1. You may want to rename your type
 parameter, or possibly remove it.
private[events] sealed trait EventsTableFlatEventsRangeQueries[Offset] {
                                                               ^

I noticed this in #6658. I'm generally in favor of sensible name-shadowing, following the "deliberately hide variables that should not be accessed here" school of thought. But I think type name shadowing isn't quite as valuable and more likely to confuse than general variable shadowing, so have experimentally linted it out; see dad17ec for the results. We can easily revert that part of this PR if we don't like the rule, though.

Pull Request Checklist

Read and understand the contribution guidelines
Include appropriate tests
Set a descriptive title and thorough description
Add a reference to the issue this PR will solve, if appropriate
Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

CHANGELOG_BEGIN CHANGELOG_END

package.scala:18: warning: it is not recommended to define classes/objects inside of package objects. If possible, define trait NoCopy in package data instead. trait NoCopy { ^

- note that `package` is itself a scoping construct, so if your reason is the apparent aesthetic of placing a bunch of things in one `package object`, that is easily remedied by deleting the `object` keyword

- I'm generally in favor of sensible name-shadowing, following the "deliberately hide variables that should not be accessed here" school of thought. But I think type name shadowing isn't quite as valuable and more likely to confuse than general variable shadowing, so have experimentally linted it out. Example warning: EventsTableFlatEventsRangeQueries.scala:11: warning: type parameter Offset defined in trait EventsTableFlatEventsRangeQueries shadows class Offset defined in package v1. You may want to rename your type parameter, or possibly remove it. private[events] sealed trait EventsTableFlatEventsRangeQueries[Offset] { ^

ContractsService.scala:197: warning: method searchDb in class ContractsService references private class ContractsFetch. Classes which cannot access ContractsFetch may be unable to override searchDb. def searchDb(dao: dbbackend.ContractDao, fetch: ContractsFetch)( ^

S11001001 · 2020-07-20T18:00:37Z

bazel_tools/scala.bzl

@@ -49,6 +49,16 @@ common_scalacopts = [
    "-Xfatal-warnings",
    # catch missing string interpolators
    "-Xlint:missing-interpolator",
+    "-Xlint:by-name-right-associative",  # will never be by-name if used correctly


This is fixed by scala/scala#5969 for 2.13, so we can remove it if we're 2.13 only.

- continuing the saga of #6116, #6132

…lac-2.12-warnings

S11001001 · 2020-07-20T18:56:49Z

Commentary on lints not enabled by this PR.

  adapted-args               Warn if an argument list is modified to match the receiver.

Already disabled by -Yno-adapted-args.

  unused                     Enable -Ywarn-unused:imports,privates,locals,implicits.

Already enabled for lf_scalacopts; if you like, we can move it in a separate PR.

  nullary-unit               Warn when nullary methods return Unit.
  nullary-override           Warn when non-nullary `def f()' overrides nullary `def f'.

I never thought these options were very useful when they were under -Ywarn-, no reason to think differently now.

  delayedinit-select         Selecting member of DelayedInit.

I'd rather not enable a warning "just 'cause".

  stars-align                Pattern sequence wildcard must align with sequence component.

This disables a bunch of useful matches like the following; no thanks.

PackageManagementServiceIT.scala:53: warning: A repeated case parameter or extracted sequence is not matched by a sequence wildcard (_*), and may fail at runtime.
    case Participants(Participant(ledger, party)) =>
                      ^

… file

S11001001 · 2020-07-20T18:59:51Z

bazel_tools/scala.bzl

+    "-Xlint:constant",  # / 0
+    "-Xlint:doc-detached",  # floating Scaladoc comment
+    "-Xlint:inaccessible",  # method uses invisible types
+    "-Xlint:infer-any",  # less through but less buggy version of the Any wart


Continuing the saga of #6116 and #6132.

remyhaemmerle-da

Looks nice.

leo-da · 2020-07-20T19:57:39Z

...ava/codegen/src/main/scala/com/digitalasset/daml/lf/codegen/backend/java/inner/package.scala

+package inner {
+  case class FieldInfo(damlName: String, damlType: Type, javaName: String, javaType: TypeName)
+}
+


@S11001001
what exactly does it buy us? Before this change all definitions were inside package object inner, now we have package inner.

why public type alieas type Fields = IndexedSeq[FieldInfo] is not part of the package inner?

what exactly does it buy us?

A class or object in a package object must be accessed by reference to that package object. If it is in a package, you can access it directly. This difference is mostly transparent to Scala code, but not to the JVM, or Java code (where that matters).

Additionally, because package objects have been historically buggy, it is best to minimize what you put in them.

why public type alieas type Fields = IndexedSeq[FieldInfo] is not part of the package inner?

This is not allowed.

By the way, in Scala, the Java-style package declaration[s] at top of file are just syntactic sugar for the full package x { ... } form; the { } are implicitly placed around the rest of the file. That's what I mean by "package is itself a scoping construct" 🙂

navigator/backend/src/main/scala/com/digitalasset/navigator/console/package.scala

leo-da · 2020-07-20T20:01:30Z

navigator/backend/src/main/scala/com/digitalasset/navigator/dotnot/package.scala

-  type NameMatcher = String => Boolean
-  type ValueMatcher = String => Boolean
-  type Action[T, R, C] = (T, PropertyCursor, String, C) => Either[DotNotFailure, R]
-


does it mean you cannot have type aliases defined in the non-object package?

Indeed. 🙂

@leo-da

- there are a lot of these fixes, and they are noisy, so shifting to a separate PR - thanks to @leo-da for pointing out

- reverts 1feae96 from #6798

S11001001 · 2020-07-20T21:40:24Z

Split -Xlint:doc-detached off to #6802.

cocreature

Thanks!

ghost

Looks good to me.

ghost · 2020-07-21T07:30:38Z

bazel_tools/scala.bzl

@@ -49,6 +51,16 @@ common_scalacopts = [
    "-Xfatal-warnings",
    # catch missing string interpolators
    "-Xlint:missing-interpolator",
+    "-Xlint:by-name-right-associative",  # will never be by-name if used correctly
+    "-Xlint:constant",  # / 0


What does "/ 0" mean?

Divide by 0.

I had guessed that much. It doesn't tell me what the linting rule does though. 😛

After reading the definition of -Xlint:constant, I guess this catches cases where we divide by 0 that only use constants? Still not sure though.

* add -Xlint:doc-detached - reverts 1feae96 from #6798 * attach several scaladocs where they'll actually be included * no changelog * attach several more scaladocs where they'll actually be included * no changelog CHANGELOG_BEGIN CHANGELOG_END

S11001001 added 6 commits July 20, 2020 13:13

set many extra scalac -Xlint options for all Scala projects

aef2bc3

CHANGELOG_BEGIN CHANGELOG_END

move NoCopy to its own file

0340606

package.scala:18: warning: it is not recommended to define classes/objects inside of package objects. If possible, define trait NoCopy in package data instead. trait NoCopy { ^

move more traits, classes, and objects to proper packages

c5e8c3b

- note that `package` is itself a scoping construct, so if your reason is the apparent aesthetic of placing a bunch of things in one `package object`, that is easily remedied by deleting the `object` keyword

fix more package-object-classes warnings

c32460c

S11001001 added this to the Maintenance milestone Jul 20, 2020

S11001001 requested review from aherrmann-da, cocreature, gerolf-da, hurryabit, leo-da and rautenrieth-da as code owners July 20, 2020 18:00

S11001001 requested a review from a user July 20, 2020 18:00

S11001001 requested a review from stefanobaghino-da as a code owner July 20, 2020 18:00

S11001001 self-assigned this Jul 20, 2020

S11001001 commented Jul 20, 2020

View reviewed changes

S11001001 added 3 commits July 20, 2020 14:32

enable -Xlint:infer-any

3171f26

- continuing the saga of #6116, #6132

enable -explaintypes for more detailed type errors

178c714

Merge commit 'a70da3478c1fcbc475b60fd9fabaca315cc5e9c5' into more-sca…

6b6ae0a

…lac-2.12-warnings

missed header for NoCopy; probably should have left it in the package…

20ad657

… file

S11001001 commented Jul 20, 2020

View reviewed changes

remyhaemmerle-da approved these changes Jul 20, 2020

View reviewed changes

leo-da reviewed Jul 20, 2020

View reviewed changes

leo-da approved these changes Jul 20, 2020

View reviewed changes

S11001001 added 2 commits July 20, 2020 17:31

misspelling in comment

11ccd89

revert -Xlint:doc-detached

1feae96

- there are a lot of these fixes, and they are noisy, so shifting to a separate PR - thanks to @leo-da for pointing out

S11001001 added a commit that referenced this pull request Jul 20, 2020

add -Xlint:doc-detached

d67d250

- reverts 1feae96 from #6798

S11001001 mentioned this pull request Jul 20, 2020

check for scaladoc comments that are not actually used #6802

Merged

6 tasks

cocreature approved these changes Jul 21, 2020

View reviewed changes

ghost approved these changes Jul 21, 2020

View reviewed changes

S11001001 merged commit 4355406 into master Jul 21, 2020

S11001001 deleted the more-scalac-2.12-warnings branch July 21, 2020 12:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add more scalac 2.12 warnings #6798

add more scalac 2.12 warnings #6798

S11001001 commented Jul 20, 2020

S11001001 Jul 20, 2020

S11001001 commented Jul 20, 2020

S11001001 Jul 20, 2020

remyhaemmerle-da left a comment

leo-da Jul 20, 2020

S11001001 Jul 20, 2020 •

edited

Loading

S11001001 Jul 20, 2020 •

edited

Loading

leo-da Jul 20, 2020

S11001001 Jul 20, 2020

S11001001 commented Jul 20, 2020

cocreature left a comment

ghost left a comment

ghost Jul 21, 2020

S11001001 Jul 21, 2020

ghost Jul 21, 2020

add more scalac 2.12 warnings #6798

add more scalac 2.12 warnings #6798

Conversation

S11001001 commented Jul 20, 2020

Pull Request Checklist

Choose a reason for hiding this comment

S11001001 commented Jul 20, 2020

Choose a reason for hiding this comment

remyhaemmerle-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S11001001 Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

S11001001 Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S11001001 commented Jul 20, 2020

cocreature left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S11001001 Jul 20, 2020 •

edited

Loading

S11001001 Jul 20, 2020 •

edited

Loading