Remove uses of inline overriding inline

[nicolasstucki]

No description provided.

[nicolasstucki]

This change was motivated by #8543 (comment)

[liufengyun]

I echo the concern about extensibility raised by @odersky :

#8543 (comment)

Now it is impossible for a third-party developer to create customized assertions (I don't think there are actually any such extensions).

Maybe, the trick to use is overloading instead of overridding by creating a dummy implicit?

trait Assertions {
  inline def assert(inline e: Boolean): Unit = println("base")
}

trait DiagramAssertions extends Assertions {
  implicit object UseDiagram
  inline def assert(inline e: Boolean)(implicit x: UseDiagram.type): Unit = println("derived")
}

class TestSuite extends DiagramAssertions {
  assert(3 == 4)
}

@main
def Test = new TestSuite   // "derived"

[nicolasstucki]

The overriding would be disabled with #8564. Overloading sounds like a better pattern for those use cases. It might be simpler to encode that with extension methods. I will create a prototype and add it in another PR.

[smarter]

I think the cleaner solution would be to keep assert abstract in the base trait and have different concrete implementations, the user then chooses which concrete implementation to mix-in or make his own. But this is likely to require a bigger refactoring of scalatest, so I think that this PR is good enough for now.

[LPTK]

@smarter in general, with such a solution, wouldn't it mean that everyone using the base abstract type (instead of the concrete type) would get normal function calls to the abstract assert method, without benefitting from any inlining?

This could even happen in Scalatest, even though it which relies on mixins and using assert from inside the class. For example, if I have a mixin component like:

trait MyTestMixin { self: Assertions =>
  ...
  assert(...)  // calls the abstract method; doesn't inline (surprising behavior)
  ...
}

Would there be a way to get an error there instead?

[sjrd]

Would there be a way to get an error there instead?

This is the behavior of the brand new abstract inline methods. Precisely.

[LPTK]

@sjrd ah thanks, perfect. I had missed that one.

[ohze]

With the changes in this PR, IDK how to migrate scalatestplus-junit:

trait AssertionsForJUnit extends Assertions {
  inline override def assert(inline condition: Boolean)
                            (implicit prettifier: Prettifier, pos: source.Position): Assertion =
    ${ AssertionsForJUnitMacro.assert('{condition}, '{prettifier}, '{pos}, '{""}) }
  ...

So I think the overloading way by @liufengyun is better?
Can someone pls help me :(

[nicolasstucki]

inline def are final, as such, you won't be able to override an inline def that is not abstract.

There is an alternative using abstract inline def

trait AbstractAssertions {
   inline def assert(...): Assertion
}

trait Assertions extends AbstractAssertions {
   inline def assert(...): Assertion = ...
}

trait AssertionsForJUnit extends AbstractAssertions {
   inline def assert(...): Assertion = ...
}

@ohze that is probably what you need in your use case.

[ohze]

using abstract inline def that way => all trait that extends a sub trait of Assertions will not able to use AssertionsForJUnit.
Ex:

trait SomeSuite extends org.scalatest.Suite ...

Note org.scalatest.Suite is a sub trait of Assertions.
This is true for class, object too.

[liufengyun]

Hi @ohze could you please elaborate a little bit what is the problem with the solution proposed by @nicolasstucki ?

[smarter]

The main problem I think is that scalatestplus-junit depends on scalatest, so it can't be fixed without changing scalatest itself.

[neko-kai]

@liufengyun All scalatest traits extend Assertion, not AbstractAssertion, this makes it impossible to mixin an AssertionsForJUnit after-the-fact. If these traits are forced to extend AbstractAssertion than they will have to always be completed by the user when declaring a new test

class Test extends WordSpec

class Test extends WordSpec with Assertions

[smarter]

So I think the overloading way by @liufengyun is better?

Looks like you figured it out: giabao/scalatestplus-junit@ee3e08a, any issue with that solution ?

[ohze]

So I think the overloading way by @liufengyun is better?

Looks like you figured it out: giabao/scalatestplus-junit@ee3e08a, any issue with that solution ?

No issue.
And I have created a PR: scalatest/scalatestplus-junit#12