New Check: enforce pattern matching by structs on function parameters

[gabrielperales]

With the upcoming Elixir releases introducing type inference, a rule to enforce pattern matching by struct will be beneficial.

This is the behavior I would expect from this rule:

🛑 Accessing to variable atom property:

def get_role(user) do
  user.role # If we are accessing an atom key it should be a Struct.
end

🛑 Pattern-matching by an atom key of a map:

def get_role(%{role: role}) do # if the map keys are atoms it must be a Struct.
  role
end

✅ Pattern-matching by Struct helps the developer know the type and shape of the parameters, it also helps the code editor with autocomplete.

def get_role(user = %User{}) do
  user.role
end

Maybe the rule can be split in two, one enforcing pattern-matching by the property you are using in the function body to avoid access to unexistent fields, and another enforcing using structs when you are pattern-matching by maps with atom fields.

First Check

Prefer pattern-match by map property the body of a function access to a map field:

🛑 Accessing a map field in the body:

def get_role(user) do
  user["role"]
end

✅ Pattern-matching by an atom key of a map:

def get_role(%{"role" => role}) do
  role
end

🛑 Accessing a map field in the body:

def get_role(user) do
  user.role
end

✅ Pattern-matching by an atom key of a map:

def get_role(%{role: role}) do
  role
end

Maybe it can be called Credo.Check.Warning.SafeParamAccess? 🤔

Second Check

Prefer pattern-match by struct when pattern-matching by map atom:

🛑 Pattern-matching by an atom key of a map:

def get_role(%{role: role}) do # if the map keys are atoms it must be a Struct.
  role
end

✅ Pattern-matching by struct

def get_role(user = %User{}) do
  user.role
end

Maybe it can be called Credo.Check.Refactor.FunctionMapParams? 🤔

[gabrielperales]

👋 @rrrene I'm implementing these checks, but I would like your opinion. Do you think it makes sense? The second check can be useful for the upcoming Elixir 1.18 to take advantage of the type inference.

[rrrene]

Hi,

what would be the benefit of using

def get_role(user = %User{}) do
  user.role
end

over

def get_role(%User{role: role}) do
  role
end

Wouldn't this be equally telling to the compiler?

[gabrielperales]

That's not what I said. The benefit is def get_role(user = %User{}) do or def get_role(%User{role: role}) do over def get_role(%{role: role}) do

I want to enforce that if you are pattern-matching by a map with atoms, it probably should be a struct.

[rrrene]

I want to enforce that if you are pattern-matching by a map with atoms, it probably should be a struct.

Okay, I can see how this might be a rule that you could align on in a specific situation.

But why should this be a standard Credo check? People use maps with atom keys all the time or am I misunderstanding something about the proposal?

[gabrielperales]

Why would you use a map with atoms instead of a Struct? The only case I can think about is a configuration object/map, but configurations don't have to be passed as parameters to functions because you can access them using Application.fetch_env! inside the function, or in a private function (we could disable this rule for private functions).

On the other hand, this rule could help take advantage of the new type inference coming in Elixir 1.18