Skip to content

Gracefully handle span mapping failing #78520

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 13, 2025
Merged

Gracefully handle span mapping failing #78520

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/Tools/ExternalAccess/Razor/Features/RazorMappingServiceWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,15 @@ public async Task<ImmutableArray<MappedSpanResult>> MapSpansAsync(
CancellationToken cancellationToken)
{
var razorSpans = await _razorMappingService.MapSpansAsync(document, spans, cancellationToken).ConfigureAwait(false);
var roslynSpans = new MappedSpanResult[razorSpans.Length];
var roslynSpans = new MappedSpanResult[spans.Count()];

if (roslynSpans.Length != razorSpans.Length)
{
// Span mapping didn't succeed. Razor can log telemetry but this still needs to be handled so return all defaults
// to indicate mapping didn't succeed.
return roslynSpans.ToImmutableArray();
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. so it's ok that you're returning an array that is the same length as 'spans', but is full of 'default' values?

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make more sense to just do the same logic as before, just have the caller decide 'the lengths are different, so i'll ignore the result'?

basically, do we need this here? (i don't have a good sense for the best layer for this sort of thing). alternative question, should _razorMappingService.MapSpansAsync fail if it can't return the same number of spans as given?

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(i'm ignorant of this area. just trying to understand it better).

Copy link
Contributor Author

@ryzngard ryzngard May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with the caller handling that too. In general we don't want this happening at all and I'm going to add some telemetry on the razor side for this

Copy link
Member

@dibarbet dibarbet May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think if we're going to handle it, we can handle it here to avoid updating a bunch of different callers (and I don't think the callers would do anything different). But I am also OK with this continuing to throw if we expect to fix it. It isn't taking down the server here, and we are in an error case, so an exception seems appropriate.

Copy link
Contributor Author

@ryzngard ryzngard May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dotnet/vscode-csharp#8271 draft for now because I have to head out but I'll test later tonight that the exception goes to the right place and update the razor code to catch

}

for (var i = 0; i < razorSpans.Length; i++)
{
var razorSpan = razorSpans[i];
Expand Down
Loading