Skip to content

Lift-4: remove simple warnings#2027

Merged
farmdawgnation merged 2 commits intolift:lift-4.0from
fanf:lift4/remove-simple-warnings
Nov 21, 2024
Merged

Lift-4: remove simple warnings#2027
farmdawgnation merged 2 commits intolift:lift-4.0from
fanf:lift4/remove-simple-warnings

Conversation

@fanf
Copy link
Contributor

@fanf fanf commented Nov 11, 2024

The goal of that PR is to decrease the number of compilation warnings in branch lift-4.0 so that it will be easier to see real problem when real changes will be introduced.

There is 3 categories of warnings:

  • 1/ the easy one, that I corrected in the PR. I listed all the actual changes done to help understand (there's a lot of noise)
  • 2/ the ones that are likely bugs and should be investigated: non exhaustif match, etc.
  • 3/ finaly, the one that are likely just change to be done, most likely introduced by deprecation, and which were not self-evident. For these one, it's likely that a lift dev can know what to do easily. I listed them for motivation :)

1/ Changed

Type ascription

Scala 3 wants A LOT of type ascription. It clutters a lot sources, but well, that horse is far now.
I've added some, millions remain.

adding ()

Auto-application to `()` is deprecated. Supply the empty argument list `()` explicitly to invoke method destroySession,
or remove the empty argument list from its definition (Java-defined methods are exempt).
In Scala 3, an unapplied method like this will be eta-expanded into a function. [quickfixable]
              S.session.foreach(_.destroySession)

=> Add a lot of ()

function call of method without ()

Method without () can't be transformed into function with method _, we must now use () => method.

Uppercase for numeric long

Lowercase el for long is not recommended because it is easy to confuse with numeral 1; use uppercase L instead [quickfixable]
          0l, 1l, true,

=> use L

### + on char/any deprecated

method any2stringadd in object Predef is deprecated (since 2.13.0): Implicit injection of + is deprecated. Convert to String to call +
  def urlFor(path: String) = baseUrl + path

Use .toString, or string interpolation depending what is the most readable

org.scalatest

type FlatSpec in package scalatest is deprecated (since 3.1.0): The org.scalatest.FlatSpec trait has been moved and renamed. Please use org.scalatest.flatspec.AnyFlatSpec instead. This can be rewritten automatically with autofix: https://github.com/scalatest/autofix/tree/master/3.1.x
class LineTokenizerTest extends FlatSpec with Matchers {

type Matchers in package scalatest is deprecated (since 3.1.0): The org.scalatest.Matchers trait has been moved and renamed. Please use org.scalatest.matchers.should.Matchers instead. This can be rewritten automatically with autofix: https://github.com/scalatest/autofix/tree/master/3.1.x
class LineTokenizerTest extends FlatSpec with Matchers {

=> Use the provided import

Array: _*

Passing an explicit array value to a Scala varargs method is deprecated (since 2.13.0) and will result in a defensive copy; Use the more efficient non-copying ArraySeq.unsafeWrapArray or an explicit toIndexedSeq call
    this.apply(kids :_*)

=> call .toseq()

### longToTimeSpan

method longToTimeSpan in trait TimeHelpers is deprecated (since 3.0.0): Long to TimeSpan conversion will be removed for possibility of ambiguous behaviours, use TimeSpan(in) instead if you are using in.millis
        override def lifespan = Full(LiftRules.clientActorLifespan.vend.apply(this))

=> add TimeSpan(...)

newInstance

method newInstance in class Class is deprecated (since 9)
      clz.getMethod(meth).invoke(clz.newInstance)

Use what is advised in newInstance:

 * The {@link
 * java.lang.reflect.Constructor#newInstance(java.lang.Object...)
 * Constructor.newInstance} method avoids this problem by wrapping
 * any exception thrown by the constructor in a (checked) {@link
 * java.lang.reflect.InvocationTargetException}.
 *
 * <p>The call
 *
 * {@snippet lang="java" :
 * clazz.newInstance()
 * }
 *
 * can be replaced by
 *
 * {@snippet lang="java" :
 * clazz.getDeclaredConstructor().newInstance()
 * }

Procedure syntax

procedure syntax is deprecated: instead, add `: Unit =` to explicitly declare `boot`'s return type [quickfixable]
  def boot {

=> use : Unit =

A pure expression does nothing in statement position

a pure expression does nothing in statement position
  def setSessionTimeout(x$1: Int): Unit = 5

Remove the expression (each time in tests)


2/ Not changed, potential real bugs

There is several real warning that can be important and should be corrected.

Shadowing a parent nested class is deprecated

shadowing a nested class of a parent is deprecated but trait Field shadows trait Field defined in trait AbstractScreen; rename the class to something else
    trait Field extends super.Field with ConfirmField {

Not sure what we can do for that one. I don't know if the support is removed, but if so, it will likely change an API.
Perhaps can be nowarned until real port to a version of scala not supporting it anymore.

Outer reference not checked at runtime

LiftScreen and SHtml produce:

The outer reference in this type test cannot be checked at run time.
  final case class BasicElemAttr(name: String, value: String) extends ElemAttr {

It might be a bug if used in pattern matching, or a useless warning against a bug of scalac that could be ignore, see:

match may not be exhaustive.

There is several case of non exhaustive match that should be investigated.

a == null is always false

lift-web/framework/core/util/src/main/scala/net/liftweb/util/CssSel.scala:874:15
comparing values of types Double and Null using `==` will always yield false
        if (a == null) Nil else List(Text(a.toString)))

Not sure if it reveals something important, I let it as it was


3/ Not changed, need understanding of Lift intent

### DateMidnight

class DateMidnight in package time is deprecated
case class Dates(dt: DateTime, dm: DateMidnight)

### URL

constructor URL in class URL is deprecated (since 20)
      newUrl = new java.net.URL(url.toExternalForm.split("!")(0) + "!" + "/META-INF/MANIFEST.MF")

This is only for JDK 20+, change will need to work on 11+. Since it's central to Lift, I didn't want to pass it in a general warning correction PR.

### TimeHelpersSpec

There is several deprecation in that class, I'm not sure about what it tests so I let them like that.

Stream

type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
                case StreamRoundTrip(_, func) =>

Stream is often use to avoid duplicate evaluation, and I'm not sure about LazyList semantic, need to be checked by someone with more knowledge than me in Lift intent.

@fanf fanf changed the base branch from main to lift-4.0 November 11, 2024 20:58
Copy link
Member

@farmdawgnation farmdawgnation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. I'll want to confirm test passage locally before we merge, but I left you some comments!

Thank you. This really is thankless work and I deeply appreciate it.

@fanf fanf force-pushed the lift4/remove-simple-warnings branch from cf4afd7 to e71876b Compare November 12, 2024 06:42
Copy link
Member

@farmdawgnation farmdawgnation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thank you for this!

@farmdawgnation farmdawgnation merged commit 3d20bc6 into lift:lift-4.0 Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants