Skip to content

NoPrematureLetComputation: Move variables inside Maybe.map function #21

@jfmengels

Description

@jfmengels

In the following example and in the current version of the NoPrematureLetComputation rule, the variable toBeMoved will be moved to the Just branch:

-- Before
let
	toBeMoved = 1
in
case x of
  Just y ->
    Just toBeMoved
  Nothing ->
    Nothing

-- After
case x of
  Just y ->
	let
		toBeMoved = 1
	in
    Just toBeMoved
  Nothing ->
    Nothing

Another way of writing this code would be with a Maybe.map, where the rule doesn't report an error but it should probably.

-- Before
let
	toBeMoved = 1
in
x
  |> Maybe.map
	(\y ->
		Just toBeMoved
	)

-- After, ideally but not currently
x
  |> Maybe.map
	(\y ->
		let
			toBeMoved = 1
		in
		Just toBeMoved
	)

There are a bunch of other functions where this would make sense, yet also plenty where it wouldn't (because we don't want to introduce multiple recomputations of the let variable).

For instance, we would like to do it for Task.map ✅ :

let
	toBeMoved = 1
in
x
  |> Task.map
	(\y ->
		Just toBeMoved
	)

but we don't for List.map, as the value would be recomputed multiple times ❌

let
	toBeMoved = 1
in
x
  |> List.map
	(\y ->
		Just toBeMoved
	)

Functions I think would be okay to move it to:

  • Maybe.map, Maybe.map2-5, Maybe.andThen
  • Result.map, Result.map2-5, Result.andThen, Result.mapError
  • Task.map, Task.andThen

I'm wondering about lambdas given to Task.perform, since this is more about delaying the computation (which I'm not sure is always desirable), and probably not so much about branching.

Although if the variable to be moved is used only in a branch of the lambda given to Task.perform, then I'd say it makes sense:

let
	toBeMoved = 1
in
x
  |> Task.perform
	(\result ->
		case result of
          Ok _ -> toBeMoved
          Err _ -> somethingElse
	)

Currently, if I remember correctly, variables are never moved across a lambda, specifically because we don't know how often the lambda will be called. So I think it makes sense for Task.perform, but only if there's branching.

For all the other ones, including custom functions, we should not do it, to avoid false positives.

I wonder if we can somehow detect which custom functions will evaluate a callback multiple times and which ones will only call it once... 🤔

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requesthelp wantedExtra attention is needed

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions