Skip to content

URI comparisons and resources.ts #93368

@jrieken

Description

@jrieken

This is the continuation of: 7d4cd4a#r37940098.

Background: We can classify uris into two categories

  1. Those that have posix-paths, like file:///foo/bar.bazz or vscode-remote://bing+bong/boo/far/fazz, and
  2. those with paths without special/known semantics, like untitled:unitiled-1 or command:deleteLeft

Uris of both categories exist in our system, generally uris that represent files are posix style, e.g all file-uris and all uris from contributed file systems. However, uris for virtual documents, command uris, and others don't fall into this category.

The URI class knows a little about this and for instance enforces that posix style uri paths start with a slash, e.g URI.parse('file:foo').toString() === 'file:///foo' whereas that "fixing" doesn't happen for arbitrary schemes (only file, http, https and arguably we should add vscode-remote here)

Now, the following problems exist:

  1. We have the resources.ts-utility which offers many convenience functions when working with uris, e.g ways to compare them. But these utilities are all written with the assumption that it works on uris with posix-paths, e.g it states that command:/deleteLeft equals command:deleteLeft (which breaks compatibility with uri.toString-usages). To make the confusion perfect it doesn't check, enforce, nor document that it work only works on a subset of uris, e.g. blind adoptions like these dc0ab50 made a perfect mess
  2. We have alternative, also fishy, ways to compare uris of which I know ResourceMap<T> and the widespread use of map.set(uri.toString(), 42)

I think we need to first decide if

  1. the resources.ts utility should only work on uris with posix-paths. That means we need at least documentation, better enforcement, and we need to review or revert @mjbvz's "adoption".
  2. alternatively we make the resources.ts utility be fit for all uris, esp with respect to uri comparisons. That means we need to know when posix rules apply and when not.

I am strongly suggesting to use variant 2 because anything else will be confusing.

As a next step we need to use the same URI-equivalence checks, e.g it's bad that the renderer uses a different identity for documents then the extension host. The ResourcesMap<T> also uses a different identity than for instance users of getComparisonKey. We should align that and ideally have them all root getComparsionKey and/or isEqual. So, this is what I think needs to be done

  • use getComparisonKey in ResourceMap<T> @bpasero
  • use ResourceMap whenever storing something by URI
  • use the same logic in TernarySearchTree@forPaths which actually is forUris @jrieken
  • define what makes uris equal, e.g. scheme, authority are case insensitive, path is case sensitive or not (depends on the OS, scheme), query is case sensitive, fragment is ignored. Also don't massage paths during compare but during creation of uris. @aeschli

Metadata

Metadata

Assignees

Labels

debtCode quality issuesuri

Type

No type

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions