microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.75k stars 12.46k forks source link

Go-to-definition on `continue` and `break` should jump around(?) corresponding statement #51224

Open DanielRosenwasser opened 2 years ago

DanielRosenwasser commented 2 years ago

Similar to #51222 and #51223.

/*END*/while (true) {
  if (Math.random()) /*START*/break;
}
/*END1*/label: while (true) {
  while (true) {
    if (Math.random()) /*START*/break label;
  }
}
/*END2*/
/*END1*/while (true) {
  if (Math.random()) /*START*/continue;
}
/*END2*/
/*END1*/switch (Math.random() < 0.5) {
  case true: /*START*/break;
}
/*END2*/

Given how go-to-definition on return, await, and yield might work, it's tempting to jump up to the top of the corresponding statement.

However, I could also see us jumping to wherever the break or continue itself would jump. That's a bit at odds with the original intent of go-to-definition on return since the point of that was to figure out "who owns the current return statement?"

If someone wants to send a PR and discuss more in that PR, they're welcome to.

sviat9440 commented 2 years ago

Both options have a reason to be. Maybe take it out in the config? But then this option should also be applicable for the return for uniformity.

If we still go to the definition on return, then it is more logical to do the same in these cases.

sviat9440 commented 2 years ago

Additional test cases:

Should works

/*END*/label: switch (null) {
  case null: {
    test: switch (null) {
      case null: /*START*/break label;
    }
  }
}
/*END*/label: while (true) {
  while (true) {
    if (Math.random()) /*START*/continue label;
  }
}

Should not fail:

label: switch (null) {
  case null: [|/*START*/break|] test;
}
sviat9440 commented 2 years ago

okay, my pr is ready. Waiting for closure #51236

SamB commented 1 year ago

However, I could also see us jumping to wherever the break or continue itself would jump. That's a bit at odds with the original intent of go-to-definition on return since the point of that was to figure out "who owns the current return statement?"

Well, personally I would prefer if go-to-definition on return would go to the call. 😉