I'm getting a warning for a <recursive call in pro...

# intellij

[eygraber]

I'm getting a warning for a recursive call in property accessor for this code:

private val HtmlElement.isBlockTag: Boolean get() = when(this) {
  is HtmlElement.HtmlTag.Header -> true
  is HtmlElement.HtmlTag.Paragraph -> isExplicit
  is HtmlElement.HtmlTag.HtmlList -> true
  is HtmlElement.HtmlTag.ListItem -> children.firstOrNull()?.isBlockTag == true
  else -> false
}

I can't tell if this is a general false positive, or only because of my specific logic (since the current HtmlElement will never be present in its own children).

[Daniel Pitts]

Seems like a false positive. btw it might be clearer to replace your ListItem handling with:

children.take(1).any { it.isBlockTag }

[twisterrob]

I think it's a valid finding. It's only false positive because of your data structure and domain knowledge. E.g. if it was a real directed graph, there could easily be a cycle (even 0 length), remember that the name "children" is just a label for humans. So it correctly identifies the recursion based on what's known from the code. IMO this is what Suppress is designed for, to help the bot understand the intricacies In the end a warning is to call attention, which it did and you decided the logic is fine. Wouldn't it be funny if a few months later you get a stack overflow?

[eygraber]

I agree, although the fact that it's called on a completely different property makes it less likely. Sure it's possible that there could end up being a circular reference, but even though it's unlikely there's no way to know that so I guess better safe than sorry 😅

[twisterrob]

Yep. Re take(1), I think that creates a temporary list, and an iterator, both of these are not necessary just to get the object and call a method. Eliezer, is there a possibility that the second one is a block, but the first is not?

[Daniel Pitts]

take(1) returns either an emptyList or a singleton list. any{...} then does iterate over it that list. In practice, the difference in efficiency in negligible, but the readability is higher IMO.

[twisterrob]

I would argue that "take 1 any" deserves a single head-scratch while reading the code. I didn't know it could be empty as well. I would expect take(1) to take 1.

[Daniel Pitts]

I suppose I'm used to working with it. the firstOrNull()?.isBlockTag == true adds a head scratcher as well though.

[Daniel Pitts]

Honestly, I'd probably create a val Iterable<Element>.isFirstBlockTag get() method of its own, just to keep it separate.

[Daniel Pitts]

iterator().run { hasNext() && next().isBlockTag }

[Daniel Pitts]

Though this isn't my codebase, so its totally up to Eliezer how they want to implement it.

[eygraber]

is there a possibility that the second one is a block, but the first is not

No, not in my domain

firstOrNull().isBlockTag == true

I think Boolean? == true is pretty standard for Kotlin. Would be better to not use qBool but for one off cases like this in private implementation I think it's fine

[Daniel Pitts]

Yeah, its not uncommon in Kotlin, but it still takes a second for me to decide whether null means true or false in these situations.

[Daniel Pitts]

Could also do firstOrNull()?.isBlockTag ?: false which I think is slightly clearer.

[Daniel Pitts]

In any case, its a minor difference. I wouldn't hold up a PR for it 😉

[eygraber]

I used to do that but then the IDE and detekt started flagging that and saying to use an equals comparison ¯\_(ツ)_/¯

[Daniel Pitts]

Hah, okay.

[twisterrob]

It happens, don't hold Detekt as holy script :) I also think?: is cleaner.