crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.2k stars 1.61k forks source link

Forbid variable assignment in function call #14722

Open SamantazFox opened 2 weeks ago

SamantazFox commented 2 weeks ago

While reviewing some code, and with the help of @Blacksmoke16, I learned that it is possible to assign a local variable in a function call, like so:

def foo(param1, bar = true, baz = false)
  # Some logic
end

def some_other_foo(param)
  # `baz` is local to this function
  # but ends up passed as `bar` inside `foo`
  foo(param, baz = true)
end

some_other_foo()

In my opinion, this is a potential footgun, and should result in a compiler error.

The confusion between param = value (for function declaration) and param: value (for function calls) is easy to make, especially when coming from/switching with other languages, such as Python. The reason why I'm mentioning that is that it happened to an experienced Crystal programmer.

Blacksmoke16 commented 2 weeks ago

My 2 cents is I think this is somewhat unlikely to be changed, but maybe a "no assignment in call" rule would be a possibility for ameba.

EDIT: My reasoning is this would ofc be a breaking change so would have to wait for Crystal 2.0, and because I think having this is somewhat helpful. E.g. in my specs I frequently do like:

collection.add "foo", foo = ART::Route.new "/foo"

so that later on in the spec I can use foo. Really only saves a line but keeps things cleaner imo :shrug:.

HertzDevil commented 2 weeks ago

This syntax is necessary for very basic things like the getter macro:

class Foo
  # this is how the macro call really looks like, even though
  # most of the time `getter` is never used with parentheses
  getter(x = 1)
end

Unfortunately this also means Ameba shouldn't detect this, as it takes a semantic pass to determine whether getter is a macro or def cell, and Ameba is purely syntactic.

Instead, when there are multiple optional parameters, personally I'd make all of them named:

def foo(*, bar = true, baz = false)
end

foo(baz = true)      # not allowed
foo(baz: true)       # okay
foo(baz: baz = true) # okay
foo(bar: baz = true) # okay
SamantazFox commented 2 weeks ago

This syntax is necessary for very basic things like the getter macro ...

Crap, I didn't think of that. Is it possible to get that at the compiler level then? Maybe at least a warning that is triggered when the local variable has the name of one of the function parameters, but doesn't match it position-wise:

def foo(bar = true, baz = false)
end

foo(bar = false) # no warning
foo(baz = true)  # compiler warning

Instead, when there are multiple optional parameters, personally I'd make all of them named:

That's what we will use in the future, I guess.

straight-shoota commented 2 weeks ago

This seems like a perfect feature for a linter. The linter needs access to the semantic stage, so ameba currently cannot implement that. But it could be a future expansion (if not for ameba, then as a separate tool). Semantic linting is certainly useful in general.