Scalafix: forbidding imports and methods from being used

Libraries
1

I wonder if scalafix supports forbidding imports and methods.

For the use case, I want to force our engineers to not use java.time.Instant but use our Instant instead because it supports mocking time.

Thank you.

1 Like
2

I wonder what you’re trying to do here…

  1. java.time.Instant has immutability semantics, much the same as String and the various arithmetic types (I’m assuming you’re coming from an imperative Java background if mocking is your thing). So if you’re mocking (as opposed to stubbing), that would mean you’d want to change the state of the thing being mocked, both in tests and in production. Do you want to change 2 to become 3? "Hello, world" to become "Servus, Welt"? false to become true (some do find this very useful, actually). Unless you’re British and its spring or autumn, would you want 9:00am to suddenly become 10:00am or vice-versa?

  2. So maybe you’re stubbing instead of mocking? Why not simply supply the time instance using the real implementation? It’s not hard to make an instant from a raw long value, or parse a formatted date / time etc. Again, would you stub the value 2 or just simply supply it…

  3. Perhaps there is some odd thing such as listening to invocations on your time objects that you want to achieve? If so, aren’t you really testing the structure of your SUT’s implementation and not the outcome of using it? That is an antipattern in imperative code anyway, but it is worse in pure functional code, because one of the tenets of that approach is that it’s OK to refactor an implementation to either recompute identical values or share / cache them. Such a refactoring will probably break that style of testing, but the refactoring would be valid.

  4. It seems a real shame to not use core standard components and write your own. You have to test your own stuff to presumably the same degree as the components you’re replacing. What happens if your code is not a walled garden and has to interoperate with other code that uses java.time.Instant?

  5. You can abstract your SUT by defining it with a generic parameter type so that you can substitute a java.time.Instant in the production code and a MyMockableTime in your tests. I often do this using integers as simple throwaway data in tests, putting in much more complex things in production.

  6. If mocking or stubbing really is your thing, how about using Mockito 5? I thought that supported mocking final classes. I’m guessing that’s why you couldn’t mock java.time.Instant…

This is tangential to the topic, but in my experience, teams that use this approach of circumscribing their engineers by force rather than persuasion don’t achieve good results. Generally the starting assumption is that the workforce is a bit unskilled / uneducated / downright stupid and has to herded into the correct thought pen to achieve results. Have to say, some of those assumptions can turn out to be true, but the solution is either to improve the talent pool you have by working with them, or replacing the talent pool altogether…

… or reconsider the validity of the starting assumption.

3

This is a commonly requested feature. I hope someone chimes in with their solution.

A specially crafted ct.sym could delete undesirable API. (Perhaps no one has tried that yet.) (It would be created at build time from your “rules”, to avoid “I forgot to install our ct.sym!”)

That would avoid the tooling problem, that your editor or IDE doesn’t know about a downstream rule forbidding an API.

I have experienced that annoyance on a Java project. It is akin to being restricted to JDK 8 as a target platform, but you want to use the current JDK API in tests.

I like the phrase from the other reply: “thought pen”, as in a pen for animals. To me, “Don’t use that thing that exists because it is useful” is like a “cattle prod”.

1 Like
4

I think that you could enforce this with archunit — something like:


import com.tngtech.archunit.core.domain.JavaClasses
import com.tngtech.archunit.core.importer.ClassFileImporter
import com.tngtech.archunit.lang.syntax.ArchRuleDefinition
import org.scalatest.wordspec.AnyWordSpec

class NoJavaTimeInstantSpec extends AnyWordSpec:

  "Classes in the project" should:
    "not depend on java.time.Instant" in:
      val importedClasses = ClassFileImporter().importPackages("com.yourcompany")

      val rule = ArchRuleDefinition.noClasses()
        .should()
        .dependOnClassesThat()
        .areAssignableTo(classOf[java.time.Instant]) // Or: haveFullyQualifiedName("java.time.Instant")
        .because("Usage of java.time.Instant is prohibited in this project")

      rule.check(importedClasses)
2 Likes
6

Thank you so much. This might work. GitHub - TNG/ArchUnit: A Java architecture test library, to specify and assert architecture rules in plain Java is an interesting tool!

1 Like
7

Thank you for your comment. We are on the same page that we should strive for doing things the right way, but the situation is a bit nuanced.

I use Playframework (older version) which uses older Mockito. I made it work using mockito-inline 4.11 but somehow it only worked in testcode but not in an injected class instance. Playframework uses Guice heavily, not sure if that comes into play somehow.

This is a commonly requested feature. I hope someone chimes in with their solution.

I imagine this is useful for other situations like “don’t use java.util.Date”.

I’m looking for an out-of-the-box scalafix rule since scalafix seems to process the semantics tree. The shape of the problem is a good fit for scalafix. Or otherwise I might write one; it seems simple enough.

Also, thank you for being polite.

1 Like
8

Not an answer to your question and I suppose it depends on your definition of “supports mocking” but java.time.Instant does have support for mocking: see java.time.Instant.now(Clock clock). I have used this successfully in our unit tests.

P.S. Replying directly in email is not supported? I did that earlier but it doesn’t show up here.

1 Like
9

Yes. You can create a rule that produces an error diagnostic if a particular symbol is referenced. Works well. I’ve written something similar for certain symbols. I can probably find some sample code in a bit.

What defines good development practice for your specific domain? I don’t know. Could be totally different than my domain. I do know scalafix is an effective tool for enforcing your teams development standards. Setting up local rules is a bit annoying but well worth the trouble.

3 Likes
10

@ninat

I saw your comment in another reply, point taken.

I wasn’t aiming to be rude, but I shouldn’t have got up on my high horse with that last comment, at least not without waiting for further context. I stand by what I wrote, but whether that’s applicable in your situation is another matter.

Perhaps the rest of what I wrote can of use to you, although given what @Hilco wrote, I may have misunderstood what you were trying to achieve.

Anyway, please accept my apologies.

I’m sorry.

1 Like
11

LocalRule seems much more convenient than making a published rule. I will do that. Would love an example if you have one. Thank you so much.

2 Likes
12

This is not real code - I wrote this inline for this post. Should be close enough to demonstrate bits that are useful for this.

The first bit is to know the symbol identifier that should not be used:

val javaUtilDate = "java/util/Date#"

Within the scope of the semantic document implicit:

def patch(implicit doc: SemanticDocument): Patch = {
  val code = doc.tree
  val invalidSymbol = Symbol(javaUtilDate)
  ???
}

Now we have the actual symbol for that id. Cool. A bunch of options here. One is to remove the import:

val removeInvalidPatch = code.collect {
  case importee: Importee if importee.symbol == invalidSymbol => Patch.removeGlobalImport(invalidSymbol)
}

If this rule is run after RemoveUnused then that will result in the code failing to compile as that removed a used symbol.

Other options are like: Create a Diagnostic that highlights the import; replace the import with a different import.

Speaking of coding standards on time. I was irked by a pattern in one project that was new Timestamp(someInstant.toEpochMilli() + someMilliseconds)

where are my nanos at? what is this java with the new?

so replacing we go:

code.collect {
  case t @ q"new Timestamp($expr.toEpochMilli() + $value)"
    if t.symbol == Symbol("java/sql/Timestamp#") => 
      Patch.replaceTree(t, s"$expr.plusMillis($value).toTimestamp")
}.asPatch

And done: That pattern is banished to the shadow realm every format run. I never need to worry about that pattern again. From me, other engineers, or over-zealous auto complete. Add in rules after rules after rules…

Breaking that down a bit more and more on why I’m a fan of scalafix:

That is not working purely syntactically. This is a semantic patch so the AST has a valid semantic meaning for Scala. For instance, this avoids a problem purely syntactic AST patching can run into: is that new Timestamp actually the java.sql.Timestamp? Or is it some other type? Maybe that expression is totally fine for that other type.

A purely syntactic patch would not be able to check t.symbol == Symbol("java/sql/Timestamp#"). That avoids patching any expression that is not actually the Timestamp of java.sql.

Nice.

3 Likes
13

Thank you so much for this!