microsoft / sarif-visualstudio-extension

SARIF Microsoft Visual Studio Viewer Extension
MIT License
46 stars 19 forks source link

SARIF viewer code finder issue #609

Closed yongyan-gh closed 1 year ago

yongyan-gh commented 1 year ago

related Bug 2116514: SARIF viewer pops up code finder, even when file is available

I think there are 2 issues mentioned in the bug:

  1. SARIF viewer pops up file open dialog immediately after opening a SARIF log, tries to re-map the results file location to your local file path. Its pops up the dialog for each result even the result file location point your local file path already.

  2. This re-mapping process usually happens when user navigate to the result in ErrorList (double clicking).

Attached the repro SARIF log file. repros.zip

yongyan-gh commented 1 year ago

@EasyRhinoMSFT / @edkazcarlson-ms , a question about code finder:

In ErrorListService.WriteRunToErrorList() method, it calls code finder to try to find best match for each SARIF result after loading a SARIF log.

Is it a hard requirement to make the code finder work? Or can it be happened when user navigates to the particular results (double clicking the error item)? the viewer used to do the file location remapping when user navigates to the result, not after loading a SARIF result.

https://github.com/microsoft/sarif-visualstudio-extension/blob/83f587986b4cd0e17f64d4410f5123b406a7cc1c/src/Sarif.Viewer.VisualStudio.Core/ErrorList/ErrorListService.cs#L705

edkazcarlson-ms commented 1 year ago

I'm slightly confused about the repro steps/ what is expected On my machine when I opened the .sarif file I got these 2 errors in my list

image

from there when I clicked on the 2nd one (SEC101/166) it was able to automatically jump me to the 1_Run Terratest file with no issues/remap popup. When I tried to click the WRN999.... error I was greeted with a popup about remapping, however when looking at the sarif file I don't see any indication of what file the invocation is tied with (I'm assuming it's supposed to be the same file but I don't see anything in the .sarif file indicating this). When looking at the specs, it looks like invocations don't have a particular location they're tied w/ so I don't understand what the expected behavior is.

If possible can I get:


While running the code finder and other remapping logic at load time is not necessarily a hard requirement, if I remember correctly the previous setup where the remapping occurred later made some extremely nasty bugs as well as being less efficient since we can optimize some of the mapping logic if we do it in batches.

yongyan-gh commented 1 year ago

@edkazcarlson-ms thanks for looking! lets look at the first issue for now. Below is my repro envionrment and steps:

Commit: 83f587986b4cd0e17f64d4410f5123b406a7cc1c VS version: 17.7.6

Repro:

  1. Download the attached zip file and unzip the file to your local disk, e.g. "C:\SarifLog". Then the sarif log file path should be "C:\SarifLog\repros\testthis\repro.sarif".

  2. Edit above repro.sarif file, make sure the only result's artifactLocation.Uri path points to the file in your local path. e.g. "file:///C:/SarifLog/repros/testthis/Azure/azure-data-labs-modules/5690656642/1_Run%20Terratest.txt".

  3. Launch VS and open the repro.sarif log file.

Expected behavior:

Actual behavior:

Let me know if any question to repro. Thanks!

edkazcarlson-ms commented 1 year ago

I see the issue now, I was initially opening to the folder and then opened the .sarif file. If I have a "blank" vs editor state and open the .sarif file only I repro the bug you mentioned. Thank you, I'll look into the issue at hand.

edkazcarlson-ms commented 1 year ago

@yongyan-gh I believe I have fixed the bug, please confirm that the changes in https://github.com/microsoft/sarif-visualstudio-extension/pull/611 fix the issue you are seeing :)

edkazcarlson-ms commented 1 year ago

Also usually Chris reviews my PR's but it seems like he's OOF, could you review https://github.com/microsoft/sarif-visualstudio-extension/pull/610 along with https://github.com/microsoft/sarif-visualstudio-extension/pull/611 when you get the chance? Thank you.