Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Extract Variable" doesn't properly work with use and case #4269

Open
GearsDatapacks opened this issue Feb 20, 2025 · 7 comments
Open

"Extract Variable" doesn't properly work with use and case #4269

GearsDatapacks opened this issue Feb 20, 2025 · 7 comments
Labels
good first issue Good for newcomers help wanted Contributions encouraged priority:high

Comments

@GearsDatapacks
Copy link
Member

GearsDatapacks commented Feb 20, 2025

The "extract variable" code action is offered in a couple of places where it shouldn't be. Inside use callbacks:

import gleam/result

pub fn main() {
  use x <- result.try(todo)
  Ok(x + 1)
}

Which transforms into:

import gleam/result

pub fn main() {
  let value = Ok(x + 1)
  use x <- result.try(todo)
  value
}

And also inside a case body:

pub fn main(result: Result(Int, String)) {
  case result {
    Ok(value) -> value + 1
    Error(_) -> panic
  }
}

Which becomes:

pub fn main(result: Result(Int, String)) {
  let int = value + 1
  case result {
    Ok(value) -> int
    Error(_) -> panic
  }
}

In both of these cases, a variable is referenced outside of the scope which it is created in, producing invalid Gleam code.

@lpil lpil added help wanted Contributions encouraged good first issue Good for newcomers priority:high labels Feb 21, 2025
@lpil
Copy link
Member

lpil commented Feb 21, 2025

Oh dear! Thank you

@lpil lpil removed the bug label Feb 27, 2025
@matiascr
Copy link
Contributor

I would like to take a look at this. I've also noticed that it's reproducible in blocks.

@lpil
Copy link
Member

lpil commented Mar 19, 2025

Thank you

@matiascr
Copy link
Contributor

I'm going to assume that (if the code action is allowed in this position) this selection

pub fn main(result: Result(Int, String)) {
  case result {
    Ok(value) -> value + 1
                 ^^^^^^^^^
    Error(_) -> panic
  }
}

Should extract variable in the following way

pub fn main(result: Result(Int, String)) {
  case result {
    Ok(value) -> {
      let int = value + 1
      int
    }
    Error(_) -> panic
  }
}

Currently, "extract variable" is not allowed top level statements. Perhaps the first statemet in a block (or the right hand side of the case clause) should also be considered a top level statement, and thus, not get extracted.

Does it still makes sense to offer the code action that creates the block (i. e. to start a refactor)? If we allow that, maybe creating an "add braces to arm expression" like the one in Rust is also a possibility.

@lpil
Copy link
Member

lpil commented Mar 20, 2025

That would be a good code action to have, please open an issue for it

@matiascr
Copy link
Contributor

Then, should we still offer the code action for these example?

pub fn main(result) {
  case result {
    Ok(value) -> value + 1
                 ^^^^^^^^^
    Error(_) -> panic
  }
}

I ask, because the code action is not offered (on purpose) when selecting the whole right-hand side of the assignment in such cases for assignments, like in

pub fn main() {
  let v = value + 1
          ^^^^^^^^^
  let w = 1
          ^
  v + w
}

I would suggest that this selection should not be allowed inside case clauses either when selecting the whole expression

case result {
  Ok(value) -> value + 1
               ^^^^^^^^^
  Error(_) -> panic
}

But the following should (and wrap it in the block and all that)

case result {
  Ok(value) -> 2 * value + 1
                   ^^^^^^^^^
  Error(_) -> panic
}

@lpil
Copy link
Member

lpil commented Mar 22, 2025

I think the most useful thing would be to always offer it, and to wrap the expression in a block if-need-be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Contributions encouraged priority:high
Projects
None yet
Development

No branches or pull requests

3 participants