Don't delete comments when deleting unused declarations

[jessetrinity]

Fixes #34568

Do not delete preceding comments when removing an unused declaration.

// section comment
const ununsed = 0;
// other comment
const used = 0;

becomes

// section comment
// other comment
const used = 0;

I've only addressed the variable statement case here because I would like feedback on which other cases this should apply to. In particular, should we do the same for imports?

[jessetrinity]

This should maybe be called Include vs the existing IncludeAll, whatever reads best to others.

[andrewbranch]

I think the start line thing is a reasonably good heuristic (better than all/nothing, anyway), but with the big exception of JSDoc comments, because those really have a semantic meaning tied to the nodes they precede:

// This comment is almost certainly not specific to `unused1`, but still belongs to it
// so your heuristic definitely does the right thing here.

// this comment looks more likely to be specific to `unused1` but it’s probably
// still fine to leave it; better to be safe.
const unused1 = 0;

/**
 * This comment is _definitely_ specific to `unusedLogFunction` and should
 * be deleted with it.
 *
 * @deprecated
 * @param x The thing to log
 */
function unusedLogFunction(x: any) {
  console.log(x);
}

[andrewbranch]

I've only addressed the variable statement case here because I would like feedback on which other cases this should apply to. In particular, should we do the same for imports?

My JSDoc example wasn’t a variable statement, but it applies to variable statements. I can’t really think of any reason not to extend the logic to all kinds of declarations... definitely function and class declarations. Notably, a file-header-like comment is already preserved, so for import declarations you’d only be changing the behavior of ones that aren’t the first non-comment syntax in the file. But I think if the logic you wrote holds for variable statements, I don’t see why it wouldn’t for imports too?

One additional idea would be to try to look at where blank lines occur and use that as a heuristic. Consider the difference between these three:

const a = 0;
// comment here
const unused = 1;
if (a) {
  return;
}

const a = 0;

// comment here
const unused = 1;
if (a) {
  return;
}

const a = 0;

// comment here
const unused = 1;

if (a) {
  return;
}

• First example. This one gives me basically no information about the relevance of the comment. Best to leave it alone (which this PR currently does).

• Second example. Maybe the comment is about unused, but it’s quite possible it’s about the next group of non-blank lines as a whole, so it’s safer to leave it (which this PR does).

• Third example. To me, this looks clearly like the comment belongs to unused. I would delete it along with the declaration.

I don’t know if that logic is too fuzzy; I’ll leave it to you to consider.

[mjbvz]

Thanks for taking a look. A few thoughts on the behavior:

• I believe this should apply to imports as well

• It's probably best to be conservative when choosing to delete a comment (JSDoc being the one clear example where we should delete comments). I would not mind preserving the comments in all three cases @andrewbranch outlined. I'm not sure if it would feel odd to have different behavior in these three cases or if could make it just feel natural

[jessetrinity]

Thanks for the feedback!

I agree that the single line comment directly preceding the unused declaration seems like it belongs to it, and I would probably want it deleted. I'm hesitant about it though, because I think it does assume judicious use of whitespace. Surely both of these people exist:

// Section Header
function f1(){
}

function f2(){
}

// Section Header

function f1(){
}

function f2(){
}

I wonder if making this decision goes against the spirit of the issue, which is that a user expects this fix to remove unused code, not comments (JSDoc comments being the exception).

[andrewbranch]

I think that’s fine, and like Matt said, deleting less is better when in doubt.

[jessetrinity]

The only issue that I'm still running into is convertFunctionToEs6Class-server.ts failing because the \r after // Comment gets deleted:

// Comment
class fn {\r
    constructor() {\r
        this.baz = 10;\r
    }\r
    bar() {\r
        console.log('hello world');\r
    }\r
}\r

There is probably some issue with the whitespace on the synthetic node because the new logic to preserve the start line shouldn't touch the new line characters from the previous line.

EDIT: It looks like the carriage return is not getting deleted after all. It looks like only the synthesized node in this test is expected to have the carriage returns. Since I changed the behavior of deleting unused declarations to not include leading comments, the // Comment line won't be touched. I added some other content before the comment to show this behavior:

Expected:
function·other()·{¶↓
····¶↓
}¶↓
//·Comment¶↓
class·fn·{¶↓
····constructor()·{¶↓
········this.baz·=·10;¶↓
····}¶↓
····bar()·{¶↓
········console.log('hello·world');¶↓
····}¶↓
}¶↓


Actual:
function·other()·{↓
}↓
//·Comment↓
class·fn·{¶↓
····constructor()·{¶↓
········this.baz·=·10;¶↓
····}¶↓
····bar()·{¶↓
········console.log('hello·world');¶↓
····}¶↓
}¶↓

I'm guessing that this test just wanted to make sure it could synthesize nodes with carriage returns and that it shouldn't touch any non-synthesized nodes. @andy-ms do you happen to remember if this is why the \rs are there?

[jessetrinity]

The type I added did change the public api. I don't know how strict we are about that...

[andrewbranch]

The conditional between IncludeAll and StartLineOnly makes me think you might need to take a look at cases like this:

// don't want to delete this comment

/** @param p delete this comment */
function unusedFunction(p) { }

[jessetrinity]

Good catch - I'm thinking about this wrong.

[jessetrinity]

I added another leading trivia option JSDoc to remove all JSDoc comments which belong to a node.

// singleLine1
/**
* JSDoc1
*/
// singleLine2
/**
* JSDoc2
*/
function f(){}

now becomes

// singleLine1

Seems like removing singleLine2 is appropriate if we remove all JSDoc comments belonging to that node. Otherwise, only deleting JSDoc2 (leaving JSDoc1 and singleLine2) also seems sane...

[andrewbranch]

Yeah, the weirder the scenario, the more approaches there are that seem reasonable. If in doubt I’d go conservative and delete less, but 🤷‍♂