Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Any wart has too many false positives in Scala code #6116

Closed
S11001001 opened this issue May 26, 2020 · 3 comments · Fixed by #6132
Closed

Any wart has too many false positives in Scala code #6116

S11001001 opened this issue May 26, 2020 · 3 comments · Fixed by #6132
Labels
discussion Things to be discussed and decided

Comments

@S11001001
Copy link
Contributor

S11001001 commented May 26, 2020

The Any wart has multiple false positives that are pervasive through our code. Therefore, we should disable it.

The false positives fall into a few categories.

The first category is caused by various aspects of Scalatest. The ironic outcome of this is that we substitute safe constructs like xs shouldBe empty for unsafe constructs like xs shouldBe 'empty because the latter don't trigger the Any wart, even though the former are well-typed and the latter are not.

The second category is wartremover/wartremover#463. This is the most pervasive, and unlike the prior category can't be fixed by simply limiting the wart to main code. In fact, even code that merely infers the values of higher-kinded type parameters can trigger Any.

The third category is the standard collections view construct. Again, we use it as a refactoring-safe, 2.13-safe variant of iterator, as in #6075. But since view triggers Any in some cases but iterator does not, we ironically choose the latter, less safe construct.

I have confirmed that Wartremover 2.4.8 does not fix any of these categories of false positives.

I suspect that the vast majority, albeit not all, of the 229 warts.Any suppressions in our codebase fall into one of these three categories. I don't suggest disabling a safety-enhancing check lightly, but make an exception in this case, as we are counterproductively drawn to less type-safe constructs as described above, as things stand. The goal is to allow us to use more safe constructs conveniently, not to expand our usage of unsafe Scala constructs (i.e. uses of Any "in anger" that are actually unsafe).

I only suggest disabling the Any wart. The other warts are quite good.

@S11001001 S11001001 added the discussion Things to be discussed and decided label May 26, 2020
@stefanobaghino-da
Copy link
Contributor

I agree with your take. Do you think there are advantages that we may lose by dropping this? Do you see ways we can retain those without necessarily using the Any wart?

@ghost
Copy link

ghost commented May 27, 2020

I agree with this. I don’t like disabling warnings but I’m one of the people who ends up writing less safe code to make this checker happy.

@S11001001
Copy link
Contributor Author

Do you think there are advantages that we may lose by dropping this?

Yes, for sure.

Do you see ways we can retain those without necessarily using the Any wart?

Not at all.

@mergify mergify bot closed this as completed in #6132 May 28, 2020
S11001001 added a commit that referenced this issue Jul 20, 2020
- continuing the saga of #6116, #6132
S11001001 added a commit that referenced this issue Jul 21, 2020
* set many extra scalac -Xlint options for all Scala projects

CHANGELOG_BEGIN
CHANGELOG_END

* move NoCopy to its own file

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

- 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 some type-parameter-shadow warnings

- 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] {
                                                               ^

* fix more package-object-classes warnings

* fix an inaccessible warning

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)(
      ^

* enable -Xlint:infer-any

- continuing the saga of #6116, #6132

* enable -explaintypes for more detailed type errors

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

* misspelling in comment

* revert -Xlint:doc-detached

- there are a lot of these fixes, and they are noisy, so shifting to a
  separate PR
- thanks to @leo-da for pointing out
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Things to be discussed and decided
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants