This is a bigger change than I thought it would be, so I would really appreciate some eyes and feedback on it to ensure:
This is spec-compliant
I'm not breaking existing behavior
The crux of the change is removing all URI parsing logic from parseArtifactLocation and instead updating UriRebaser.translateArtifactToLocal to handle raw SARIF artifact paths.
Since translateArtifactToLocal now acts on the verbatim artifact URIs, that allows us to easily prepend extension-provided uriBases. However, as a side effect, artifactUri is no longer guaranteed to be an actual URI, so lots of the other changes were updating supporting codepaths (e.g. all the base translation) to handle simple filesystem paths, not just URIs.
Validation:
Relative path and an extension-provided uriBase ✅
Relative path and a uriBase defined within the SARIF file ✅
Absolute path that exists on disk ✅
Relative path that has a distinct project item match ✅
Relative path that needs location lookup ✅
Relative path with workspace concatenation ✅
Relative path that has an open doc match ✅
Absolute path that doesn't exist on disk but does have a distinct project item match ✅
Sidenotes.
Since we're operating on FS paths, seems like we should be using Uri.fsPath instead of Uri.path everywhere, but that's even more potential to breakage so I didn't touch that. Something to keep in mind though.
It would be nice if artifactUri could be renamed to something that didn't imply it was Uri, because it isn't (whereas localUriis a Uri)
I couldn't run the test suite because it's failing to initialize due to a myriad of pre-existing TypeScript errors.
Fixes #506
This is a bigger change than I thought it would be, so I would really appreciate some eyes and feedback on it to ensure:
The crux of the change is removing all URI parsing logic from
parseArtifactLocationand instead updatingUriRebaser.translateArtifactToLocalto handle raw SARIF artifact paths.Since translateArtifactToLocal now acts on the verbatim artifact URIs, that allows us to easily prepend extension-provided uriBases. However, as a side effect,
artifactUriis no longer guaranteed to be an actual URI, so lots of the other changes were updating supporting codepaths (e.g. all the base translation) to handle simple filesystem paths, not just URIs.Validation:
Sidenotes.
Uri.fsPathinstead ofUri.patheverywhere, but that's even more potential to breakage so I didn't touch that. Something to keep in mind though.artifactUricould be renamed to something that didn't imply it wasUri, because it isn't (whereaslocalUriis aUri)