Casts can have a side-effect and thus their object can be used

[nobodywasishere]

No description provided.

[Sija]

What does this change is meant to achieve, since as far as I understand this visitor class is meant to capture the return value of expression?

[nobodywasishere]

There are methods such as this in the stdlib that use the fact that as raises if the cast cannot be performed. This means any value of the obj should be counted as being used. Without this change, the below with Lint/UnusedInstanceVariableAccess marks @raw as being unused (due to the Nil return type)

  # Checks that the underlying value is `Nil`, and returns `nil`.
  # Raises otherwise.
  def as_nil : Nil
    @raw.as(Nil)
  end

[Sija]

It seems to me there are some logic faults in here:

a) Any object that's being used in a Call should be counted as used
b) Last expression in methods with Nil return type restriction shouldn't be disregarded

[nobodywasishere]

Any object that's being used in a Call should be counted as used

Casts aren't Calls, but fair enough. We know that (most) pseudo methods don't have side-effects, so I was fine with not counting their objects as used. This may not be necessary anymore once Lint/UnusedPseudoMethod is added, so I'll change that here for the rest of the pseudo methods.

Last expression in methods with Nil return type restriction shouldn't be disregarded

What's your reasoning for this? With this, I've found a handful of places in the stdlib where the return type was Nil but there was still an attempt to return a local variable's value. It can be useful to catch these cases. If it's about wanting semantic analysis in order to verify it's actually the stdlib Nil, fair enough and I can change that too.

[Sija]

What's your reasoning for this? With this, I've found a handful of places in the stdlib where the return type was Nil but there was still an attempt to return a local variable's value. It can be useful to catch these cases. If it's about wanting semantic analysis in order to verify it's actually the stdlib Nil, fair enough and I can change that too.

It's pretty common to simply leave the last expression in such methods as is - which is not a mistake, since the Nil return type restriction enforces the return value to nil, anyway.

[Sija]

I'm still not sure if the casts should increment the stack - i.e. what cases does that cover?

[nobodywasishere]

Incrementing the stack before visiting the cast object tells the visitor that the object's return value is used by the cast. It means 1.as(Int32) won't flag the 1 as unused if it's in a void context.

[Sija]

Ok, I guess my confusion stems from the fact that it technically is still unused, yet as might have side-effects. What about as? though? It can't raise so it should still count as unused, doesn't it?

[nobodywasishere]

Lint/UnusedPseudoMethods will handle marking as? and the like as unused (outside of this PR), but even if the as? is unused, it's object is still used by the as? itself. Otherwise we'll end up with things like this, which doesnt make sense (only the unused pseudo method should be reported imo):

"my string".as?(String)
# ^^^^^^^^^ error: Unused literal
          # ^^^^^^^^^^^ error: Unused pseudo method

This was my mistake when originally implementing the ImplicitReturnVisitor.

[Sija]

It's pretty common to simply leave the last expression in such methods as is - which is not a mistake, since the Nil return type restriction enforces the return value to nil, anyway.

I probably should've had add that this is only relevant to the expressions with side-effects and not literals and such, sorry for the confusion.

[nobodywasishere]

So if there's a literal or an unused variable access on the last line of a method with a Nil return type restriction, are you okay with flagging it as unused?

[Sija]

@nobodywasishere That's correct.