Unused argument rule

[veelenga]

A rule that reports unused arguments. For example, this is considered invalid:

def method(a, b, c) # <-- unused argument `c`
  a + b
end

and should be written as:

def method(a, b)
  a + b
end

or

def method(a, b, _c)
  a + b
end

Also it detects unused arguments in blocks and procs:

3.times do |i|   # <-- unused argument `i`
  a + b
end

->(s : Int32) { puts "hi" } # <-- unused argument `s`

# this is valid
def method(&block)
  block.call
end

def method(&block)  # <-- unused argument `block`
  yield 20
end

but in this case &block arg is still just a dummy argument and can be omitted.

[straight-shoota]

I don't think it makes sense to enforce removing unused arguments. They might be part of a public API and sometimes need to be there even if they're unused.

method(a, b, _c) is a different signature than method(a, b, c) and would not match if c was used as a named argument at callsite. This could be prevented using c as external name, but that would add way to much unnecessary noise.

With inlined blocks, it is fine to remove unused arguments. But blocks that are cought in a Proc likely need the arguments to match the proc type.

It is certainly useful to identify unused arguments, especially in internal methods. But in public API's they're probably more often explicitly wanted than not. They obviously can't be removed but also shouldn't trigger an error (or require manual disabling) every time.

I don't know what would be a good way to do this. Maybe limit to private methods, but private methods can also be part of a defined API (for example the unbufferd_* methods in subclasses of IO::Buffered). And you couldn't catch unused arguments in public methods that are only internal implementation.

[veelenga]

@straight-shoota

But blocks that are caught in a Proc likely need the arguments to match the proc type.

And in this case renaming the argument (prepending the _) would work (since it doesn't change the signature), wouldn't it?

method(a, b, _c) is a different signature than method(a, b, c) and would not match if c was used as a named argument at callsite. This could be prevented using c as external name, but that would add a way to much unnecessary noise.

This is a very good argument. What I think now is making each of these types (def, block and proc) be disableable. This is very easy to do within the configuration file. For example, if the user would want to disable the verification of the unused arguments in defs, he will need to do:

# .ameba.yml
UnusedArgument:
  check_def: false
  # check_block: true
  # check_proc: true

do you think we need to make this default? Enabling it may be good for new projects, where the API is not standardized yet.

[veelenga]

@asterite sorry, but need your help :)

Could you please explain why there is &block syntax. Why Crystal needs such an argument? Since it is not required and can't be used as a variable it looks completely useless:

def method(a)
  yield a # => no &block needed
end

def method(a, &block)
  p block        # => doesn't compile
  block.call(a)  # => doesn't compile
  &block.call(a) # => doesn't compile
end

is there any usecase?

[straight-shoota]

And in this case renaming the argument (prepending the _) would work (since it doesn't change the signature), wouldn't it?

Yes, renaming should be fine there.

do you think we need to make this default?

Yes, definitely. False positives would produce lots of annoying noise otherwise.

&block is for capturing a block as a Proc and passing it around, basically as an anonymous function (see docs). You can certainly call it. However, in your example, the block argument needs an explicit type declaration in order to work:

def method(a, &block : String ->)
  block.call(a)
end

method("foo") { |s| puts s }

[veelenga]

TODO:

• unused method args & super:

def foo(a) # <-- `a` is used implicitly  
  super
end