fix(51225): Go-to-definition on case or default should jump to the containing switch statement if available.

[sviat9440]

Fixes #51225

[sviat9440]

@microsoft-github-policy-service agree

[DanielRosenwasser]

Can you add one more test?

switch (null) {
    case null:
        export /*start*/default 123;
}

Validate that it doesn't jump to the switch statement.

[sviat9440]

/////*c*/export default { [|/*a*/case|] };
////[|/*b*/default|];

Is it normal that b and goes to c? This is how it works now and looks like a bug. It is expected that there will be no go to anywhere.

[DanielRosenwasser]

It's questionable, but I wouldn't get hung up on fixing that specific case.

[sheetalkamat]

This looks good but the tests dont verify contextSpan is coming out correct.

Also should context be switch keyword plus expression or just expression.. Probably expression is fine but i am not sure if editor works out ok if context span is mutually exclusive with definition span.. (dont think we have had that)

[sviat9440]

@sheetalkamat I changed contextSpan to keyword + expression, but i can't find any examples for testing contextSpan with fourslash

[DanielRosenwasser]

Good to go @sheetalkamat?

[sviat9440]

Any progress? @sheetalkamat

[sheetalkamat]

@typescript-bot pack this

[sheetalkamat]

I am working on framework to test contextSpan with goto def which we should merge and then rebase this PR so its clearer on what is the context that is being set for contextSpan. Will link PR here once i have it ready

[sheetalkamat]

@sviat9440 #52576 is up and once that is merged you should be able to update your PR with the baselines that verify context,
Having said that, i completely mis-read context Span to be first line of declaration. It is infact the complete declaration. With that in mind i think your earlier change of having contextSpan as complete switch statement was correct and you would want to go back to that behavior. Sorry for misdirecting you on that one.

[sheetalkamat]

#52576 is merged. Feel free to merge changes from main and baseline

[sheetalkamat]

i think contextSpan should be switch (null) instead of just null

[sandersn]

Was the previous commit correct then?

[sheetalkamat]

kind of.. probably needs closing paren as well part of context ?

[sheetalkamat]

This needs to be moved to getContextNode which handled SwitchStatementthen ...FindAllReferences.getContextSpan() so that all places where we will use context span are in single place.

[sandersn]

Looking at createDefinitionInfoFromName above, it looks like the right way to use this is first to call getContextNode to get the start/end tokens, then to use toContextSpan to turn it into a context span (see code below). Unfortunately, my getFullStart hack (which grabs the closing paren) gets lost since getContextNode returns a node pair, not a location pair. And toContextSpan currently always calls getEnd, but my hack uses getFullStart.

    const keyword = FindAllReferences.getContextNode(statement)!;
    const textSpan = createTextSpanFromNode(isContextWithStartAndEndNode(keyword) ? keyword.start : keyword, sourceFile)
    return {
        fileName: sourceFile.fileName,
        textSpan,
        kind: ScriptElementKind.keyword,
        name: "switch",
        containerKind: undefined!,
        containerName: "",
        ...FindAllReferences.toContextSpan(textSpan, sourceFile, keyword),
        isLocal: true,
        isAmbient: false,
        unverified: false,
        failedAliasResolution: undefined,
    };

Any ideas? I'm going to try hacking toContextSpan to special case switches to see how bad that is.

[sandersn]

OK, the hack isn't too bad; there's a special case for strin gliterals there already, so I added it after that.