Skip to content
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.

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

Bug in URI.isUri() that fails thing type comparison #114971

Closed
enagic opened this issue Jan 26, 2021 · 1 comment · Fixed by #114972
Closed

Bug in URI.isUri() that fails thing type comparison #114971

enagic opened this issue Jan 26, 2021 · 1 comment · Fixed by #114972
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders uri verified Verification succeeded
Milestone

Comments

@enagic
Copy link
Contributor

enagic commented Jan 26, 2021

  • VSCode Version: 1.53.0
  • OS Version: macOS 11.1

Steps to Reproduce:

  1. Create custom URI class that implements the default URI
  2. Pass in the custom URI into URI.isUri(myCustomUri) and it returns false

Background

This is a proposed fix for monaco-editor.

If a model is created using a custom URI, it will fail the URI.isUri() check because it "tries" to check thing against various possible types, but it will fail at fsPath.

class URI {
    static isUri(thing) {
        if (thing instanceof URI) {
            return true;
        }
        if (!thing) {
            return false;
        }
        return typeof thing.authority === 'string'
            && typeof thing.fragment === 'string'
            && typeof thing.path === 'string'
            && typeof thing.query === 'string'
            && typeof thing.scheme === 'string'
            && typeof thing.fsPath === 'function' // this guy right here should be 'string'
            && typeof thing.with === 'function'
            && typeof thing.toString === 'function';
    }
}

The side-effect is that markerService behaves a bit weird and shows markers for all resources, not just for a particular resource/URI.

Example:

class MyUri implements Uri {
  ...
  // rest of class to satisfy TS
  ...
}

const myUri = MyUri.parse('foo://example.com:8042/over/there');

// Note:
// - myUri is _not_ an instanceof Uri
// - typeof myUri.fsPath === 'string', not 'function'

const model = editor.createModel('', 'plaintext', myUri);
const { resource } = model;

// `resourceMarkers` ends up returning all markers of `markerService._data`,
// not just the resource's markers because `DataResourceMap.values()`
// fails the `URI.isUri()` check.
const resourceMarkers = markerService.read({ resource });

Does this issue occur when all extensions are disabled?: Yes

@jrieken jrieken added bug Issue identified by VS Code Team member as probable bug uri labels Jan 26, 2021
@jrieken jrieken added this to the January 2021 milestone Jan 26, 2021
@jrieken
Copy link
Member

jrieken commented Jan 26, 2021

Indeed. Nice find.

jrieken added a commit that referenced this issue Jan 26, 2021
@JacksonKearl JacksonKearl added the verified Verification succeeded label Jan 28, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Mar 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders uri verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@enagic @jrieken @JacksonKearl and others