Keep track of the associated text snapshot when diagnotsics are created. #24721

CyrusNajmabadi · 2018-02-09T00:26:17Z

This allows us to accurately map the span of a diagnostic returned by the diagnostic service forward to whatever snapshot we currently have.

Customer scenario

User codes C#/VB/Typscript/F# and squiggles shows up in wrong places and take sometime before it gets corrected.

Bugs this fixes

Fixes #24714

Workarounds, if any

N/A

Risk

I don't see any major risk due to this change.

Performance impact

we use a map to associates buffer to right events, but it is small map and entries go away once either errors are fixed or files are closed. so no big issue here.

Is this a regression from a previous update?

Yes. this is due to this change (#23448) which fixed one of biggest memory issue we had.

Root cause analysis

when one of diagnostics got updated, our existing tagger updated all diagnostics currently marked on open files. and that is an issue since some of diagnostics are not synced to latest source yet. only one of them has. so except that one, all other diagnostics can point to wrong span which cause the issue above.
the fix make sure when we use diagnostics from old source, we map that to current text correctly.

How was the bug found?

Dogfooding

CyrusNajmabadi · 2018-02-09T00:27:00Z

tagging @heejaechang

CyrusNajmabadi · 2018-02-09T00:28:29Z

src/EditorFeatures/Core/Implementation/Diagnostics/AbstractDiagnosticsTaggerProvider.cs

+            }
+
+            var document = e.Solution.GetDocument(e.DocumentId);
+            if (document != null && document.IsOpen() && document.TryGetText(out var sourceText))


@jasonmalinowski TryGetText should pretty much always work for Documents that are open right?

I realize there is a slight race here in that IsOpen might return true, but then the file might lose, and TryGetText could fail. but that's ok in terms of how this is coded up.

What i want to make sure is that in the case where a Document really is open, we should always feel confident hat TryGetText succeeds. Is that something one can depend on?

Personally, I'd delete those Try* methods before depending on them. 😄

Note: i could also just hold onto the Document. The theory there being that since this is only for open Documents it's not expensive to hold onto. While this could root some solutions, it would only be for a short while as the diagnostic subsystem would then recompute the diagnostics for those open documents quickly, replacing any exiting diagnostics. however, it seems riskier.

@heejaechang what do you think?

@CyrusNajmabadi Any reason not to just call GetText()? If your theory that TryGetText works is correct, it's instant. If not, well, you will still work?

i can also just do a synchronous call to GetTextAsync().WaitAndGetResult(). But i feel very skeezy doing that. i believe TryGetText should be safe, and i believe that's what rename uses and the same assumption it makes, right @jasonmalinowski ?

I guess given right now there's a PR (#24693) out fixing that function from complexity like this, I'm not sure how much I'd trust the invariants of the system.

Don't we have GetTextSynchronously() anyways? We have it for fetching trees, which implies we should be able to fetch text synchronously!

SourceText for open document is referenced strongly in solution itself. so TryGetText should always return BUT!! Document.IsOpen() is mutable state not tied to Solution IsOpen is called off. so in other words, you could ask Solution that is created before Document is opened with Workspace which currently think Document is opened :)

so, TryGetText can return null... BUT! then text is most likely just came from closed file. so your fallback logic most likely give right text... :)

I just moved to GetTextSynchronously.

CyrusNajmabadi · 2018-02-09T01:04:40Z

src/EditorFeatures/Core/Implementation/Diagnostics/AbstractDiagnosticsTaggerProvider.cs

+        /// we're tagging.
+        /// </summary>
+        private static readonly ConditionalWeakTable<object, ITextSnapshot> _diagnosticIdToTextSnapshot = 
+            new ConditionalWeakTable<object, ITextSnapshot>();


@heejaechang I think the lifetimes here are exactly what we want. Namely, as long as the diagnostics subsystem is holding onto these IDs, then we'll hold onto the snapshot here. And the diagnostic subsystem holds onto this ID until it is replaced/removed with the most up to date version of the diagnostics for that analyzer/document-id pair, right?

basically, as long as we could observe the diagnostics associated with an id, then we'll have the snapshot for it. And once we can't observe them anymore (because they were replaced/removed), and thus GetDiagnosticsUpdatedEventArgs won't return them, then this will release the snapshot as well.

heejaechang · 2018-02-09T04:22:36Z

integration tests failed with same timeout not sure whether it is related to this PR or not though.

heejaechang · 2018-02-09T04:25:58Z

src/EditorFeatures/Core/Implementation/Diagnostics/AbstractDiagnosticsTaggerProvider.cs

+                return;
+            }
+
+            if (_diagnosticIdToTextSnapshot.TryGetValue(e.Id, out var snapshot))


there could be issue where build only error staying long time (until next build) and we holding snapshot for it quite long. but at the same time, this will make those build error to show up in correct place even after open/close/modification of a file. so trade off I guess.

Is it worth clearing the entry on final tagger disposal or something? At that point that buffer ain't useful anymore.

jinujoseph · 2018-02-09T05:32:27Z

@heejaechang , should we consider to bring this for 15.6 ?

CyrusNajmabadi · 2018-02-09T18:26:25Z

@heejaechang , should we consider to bring this for 15.6 ?

IMO, this needs to go in asap. This is a pretty severe regression. That's ok for a preview, but unacceptable for the real release.

CyrusNajmabadi · 2018-02-09T18:27:59Z

@jasonmalinowski @dotnet/roslyn-ide can we get another pair of eyes here. This is a pretty severe regression that needs fixing asap. Thanks!

CyrusNajmabadi · 2018-02-09T18:42:08Z

Note: i've validated this fix. But it would be good to get secondary smoke testing on it as well.

jasonmalinowski · 2018-02-09T19:15:30Z

@CyrusNajmabadi Can you retarget this to the dev15.6.x branch? Not sure what bar checks this would need, but it'll be good to get that out of the way.

heejaechang · 2018-02-09T19:19:25Z

@jinujoseph can we ask vendor for verification?

jinujoseph · 2018-02-09T20:04:12Z

Sure , we can get it validated ... @heejaechang can you get a signed build for this.

Add comment. Add comment. Switch to getting the text synchronously. Add comment.

CyrusNajmabadi · 2018-02-09T20:08:21Z

Base has been changed.

jasonmalinowski

There's one possible reliability problem I've called out in individual comments we should get answered.

Otherwise, mostly suggestions. Switching holding onto ITextVersion instead of ITextBuffer might be a nice improvement (might add a line or two of code at most) and at least demonstrates how to be good editor citizens.

jasonmalinowski · 2018-02-09T20:30:44Z

src/EditorFeatures/Core/Implementation/Diagnostics/AbstractDiagnosticsTaggerProvider.cs

+            if (document != null && document.IsOpen())
+            {
+                // This should always be fast since the document is open.
+                var sourceText = document.State.GetTextSynchronously(cancellationToken: default);


Sorry I was missing full context on the previous conversation: should this just be GetTextAsync? In theory still instant, but if we're wrong it's less terrible?

(still OK with this more than TryGetText)

Yup, will fix up.

jasonmalinowski · 2018-02-09T20:33:34Z

src/EditorFeatures/Core/Implementation/Diagnostics/AbstractDiagnosticsTaggerProvider.cs

+            // a small risk that between calling .IsOpen the file may then close, which then would
+            // cause TryGetText to fail.  However, that's ok.  In that case, if we do need to tag this
+            // document, we'll just use the current editor snapshot.  If that's the same, then the tags
+            // will be hte same.  If it is different, we'll eventually hear about the new diagnostics 


jasonmalinowski · 2018-02-09T20:39:25Z

src/EditorFeatures/Core/Implementation/Diagnostics/AbstractDiagnosticsTaggerProvider.cs

+                // and apply them directly to the snapshot we have.  Either no new changes will
+                // have happened, and these spans will be accurate, or a change will happen
+                // and we'll hear about and it update the spans shortly to the right position.
+                _diagnosticIdToTextSnapshot.TryGetValue(id, out var diagnosticSnapshot);


Is it possible for a file to be closed/reopened so quickly that _diagnosticIdToTextSnapshot will still have a valid ID but a snapshot from the previous buffer? If so, all the mapping stuff below will crash.

(Going to flag this "changes required" to get a confirmation on this. If it's fine then obviously no changes required.)

Yes, that's a possible problem. Question about this. If i switch to ITextImage:

how do i validate that it's for the same 'buffer' as the buffer i currently have?

how do i use that to map spans forward?

Ah, i see you answered that below. Thanks!

So not sure which approach you're doing now. Maybe you have to stay using ITextSnapshot but then you can just compare buffers. Maybe it's best to do that and we can research more efficient ways than me sidetracking you down a crazy long path.

So not sure which approach you're doing now. Maybe you have to stay using ITextSnapshot but then you can just compare buffers. Maybe it's best to do that

Yup. THat's the approach i went with and have included in the PR.

jasonmalinowski · 2018-02-09T20:46:16Z

src/EditorFeatures/Core/Implementation/Diagnostics/AbstractDiagnosticsTaggerProvider.cs

+        /// diagnostics, we don't know how to map the span of the diagnostic to the current snapshot
+        /// we're tagging.
+        /// </summary>
+        private static readonly ConditionalWeakTable<object, ITextSnapshot> _diagnosticIdToTextSnapshot = 


A possible perf optimization here is to hold onto ITextVersion instead. Your .ToSnapshotSpan().TranslateTo() then would become a call to Tracking.TrackSpanForwardInTime.ToSnapshotSpan() to the current version. Should be pretty easy to change.

(Versions are cheaper than snapshots in that they don't hold onto the actual text structure, but just the deltas. A snapshot always exposes it's version from the .Version property.)

to the current version. Should be pretty easy to change.

sounds good. However, still not sure how i validate that the snapshots are from teh same buffer. i.e. that the open/close/open scenario doesn't cause problems.

Ok. I couldn't get this to work. The issue is that diagnostics store their information as line/cols. Which means i need the actual original snapshot in order to map that information back to a reasonable span in roslyn coordinates. I could attempt to store an ITextImage. However, then i also need a way to map that to a SourceText (since that's the currency that DiagnosticData expects). I could do all the work to provide a SourceText around an ITextImage, but this seems like a lot of extra gunk just to get this work.

Thoughts @jasonmalinowski

Oh, that's lame. Why don't we just hold onto it in regular span form to start? Was it just to work around issues here that we're now making better?

That's a good question. @heejaechang Do you know why diagnostics are line/column instead of just TextSpan?

note: it may be because of external diagnostics where some tool is reporting the problem... but i don't know for certain.

DiagnosticData has line/column data if it came from build. but if it came from live, it will have TextSpan.

the way you do GetExistingOrCalculatedTextSpan should return TextSpan if it is already there or create one if it is not.

I dont calculate text span from build error right away because it requires reading in Text and a lot of time, those errors stay in error list (where it doesn't need span) and never shown to users in editor (which require span)

Right. But since these errors may have come from build, and not from live, we can't assume there will be TextSpans for them, and we have to provide the SourceText so that the diagnostic data can be mapped to a span. Correct?

jasonmalinowski · 2018-02-09T20:51:43Z

src/EditorFeatures/Core/Implementation/Diagnostics/AbstractDiagnosticsTaggerProvider.cs

+                return;
+            }
+
+            if (_diagnosticIdToTextSnapshot.TryGetValue(e.Id, out var snapshot))


Is it worth clearing the entry on final tagger disposal or something? At that point that buffer ain't useful anymore.

jasonmalinowski · 2018-02-09T20:52:18Z

src/EditorFeatures/Core/Implementation/Diagnostics/AbstractDiagnosticsTaggerProvider.cs

@@ -135,7 +191,8 @@ private void ProduceTags(TaggerContext<TTag> context, DocumentSnapshotSpan spanT
                        //    editorSnapshot.

                        var diagnosticSpan = diagnosticData.GetExistingOrCalculatedTextSpan(sourceText)
-                                                           .ToSnapshotSpan(editorSnapshot);
+                                                           .ToSnapshotSpan(diagnosticSnapshot)
+                                                           .TranslateTo(editorSnapshot, SpanTrackingMode.EdgeExclusive);


Would it make sense to bump _diagnosticIdToTextSnapshot to hold onto the newer editor snapshot? Consider a case where lots of edits are happening and the diagnostic engine is backed up. Eventually that DiagnosticsUpdated will get triggered and update it no matter what, but is it worth doing it here too?

("no" is a perfectly valid answer, just asking.)

Is it worth clearing the entry on final tagger disposal or something? At that point that buffer ain't useful anymore.

The provider stays around forever, and is shared across all taggers. We don't currently keep track in any way if there are absolutely no taggers around anymore.

Would it make sense to bump _diagnosticIdToTextSnapshot to hold onto the newer editor snapshot?

Definitely not. Because then the spans the DiagnosticService has for that ID would not be correct wrt to that snapshot.

CyrusNajmabadi · 2018-02-09T22:36:31Z

@jasonmalinowski Are you ok with this in its current form?

CyrusNajmabadi · 2018-02-09T23:07:27Z

Since htis is 15.6 now, can someone write up whatever ask-mode stuff might be necessary? Thanks!

vsccarl · 2018-02-13T22:35:02Z

I've completed a test pass for this and can sign off.

CyrusNajmabadi · 2018-02-13T22:41:31Z

@jinujoseph is this ready to go for shiproom approval?

heejaechang · 2018-02-13T22:53:10Z

@jinujoseph can you take a look? I updated ask mode thing.

jinujoseph · 2018-02-13T23:30:14Z

yes , waiting for approval from shiproom

jinujoseph · 2018-02-14T00:59:24Z

approved to merge via Link

CyrusNajmabadi · 2018-02-14T01:48:31Z

Thanks all!

CyrusNajmabadi requested a review from a team as a code owner February 9, 2018 00:26

CyrusNajmabadi mentioned this pull request Feb 9, 2018

Tagging gets very out of sync when typing, and takes a long time to get into a consistent state. #24714

Closed

CyrusNajmabadi commented Feb 9, 2018

View reviewed changes

CyrusNajmabadi force-pushed the diagnosticSpans2 branch from 3b038b8 to c890a13 Compare February 9, 2018 00:30

CyrusNajmabadi commented Feb 9, 2018

View reviewed changes

heejaechang approved these changes Feb 9, 2018

View reviewed changes

heejaechang reviewed Feb 9, 2018

View reviewed changes

jinujoseph assigned heejaechang Feb 9, 2018

CyrusNajmabadi force-pushed the diagnosticSpans2 branch from 26ed60c to 2fae908 Compare February 9, 2018 06:36

CyrusNajmabadi force-pushed the diagnosticSpans2 branch from 2fae908 to a088cce Compare February 9, 2018 20:01

Keep track of the associated text snapshot when diagnotsics are created.

146f8c5

Add comment. Add comment. Switch to getting the text synchronously. Add comment.

CyrusNajmabadi force-pushed the diagnosticSpans2 branch from a088cce to 146f8c5 Compare February 9, 2018 20:07

CyrusNajmabadi changed the base branch from master to dev15.6.x February 9, 2018 20:08

CyrusNajmabadi mentioned this pull request Feb 9, 2018

Compute accurate diagnostic spans in Tagger.ProduceTags. #24719

Closed

jasonmalinowski requested changes Feb 9, 2018

View reviewed changes

CyrusNajmabadi added 2 commits February 9, 2018 13:43

Be resilient to the buffer being different.

3906c94

Merge checks.

5c392b9

jasonmalinowski approved these changes Feb 9, 2018

View reviewed changes

jinujoseph added the Needs Shiproom Approval label Feb 9, 2018

jinujoseph added Approved to merge and removed Needs Shiproom Approval labels Feb 14, 2018

jinujoseph merged commit f28a27a into dotnet:dev15.6.x Feb 14, 2018

CyrusNajmabadi deleted the diagnosticSpans2 branch February 14, 2018 01:48

jasonmalinowski mentioned this pull request Feb 12, 2022

Support live diagnostics in source generated files #58363

Merged

Keep track of the associated text snapshot when diagnotsics are created. #24721

Keep track of the associated text snapshot when diagnotsics are created. #24721

Conversation

CyrusNajmabadi commented Feb 9, 2018 • edited by heejaechang Loading

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

CyrusNajmabadi commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang commented Feb 9, 2018

heejaechang Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinujoseph commented Feb 9, 2018

CyrusNajmabadi commented Feb 9, 2018

CyrusNajmabadi commented Feb 9, 2018

CyrusNajmabadi commented Feb 9, 2018

jasonmalinowski commented Feb 9, 2018

heejaechang commented Feb 9, 2018

jinujoseph commented Feb 9, 2018

CyrusNajmabadi commented Feb 9, 2018

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Feb 9, 2018

CyrusNajmabadi commented Feb 9, 2018

vsccarl commented Feb 13, 2018

CyrusNajmabadi commented Feb 13, 2018

heejaechang commented Feb 13, 2018

jinujoseph commented Feb 13, 2018

jinujoseph commented Feb 14, 2018

CyrusNajmabadi commented Feb 14, 2018

CyrusNajmabadi commented Feb 9, 2018 •

edited by heejaechang

Loading

CyrusNajmabadi Feb 9, 2018 •

edited

Loading

heejaechang Feb 9, 2018 •

edited

Loading