microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
162.69k stars 28.69k forks source link

Spurious `provideDefinition()` call when pressing `Ctrl` #197120

Closed adam-coster closed 10 months ago

adam-coster commented 10 months ago

Type: Bug

Why this is an issue

I discovered this problem in my extension, where my provideDefinition function would either return a location for userland symbols or open the browser to the docs for native symbols. I was surprised to find that simply pressing Ctrl while hovering over native symbols would cause the browser to open to the docs for that symbol.

This caused a lot of chaos, since Ctrl gets hit a lot while the cursor is essentially hovering over random symbols.

I thought maybe VSCode was grabbing those results so that it would already have them if the user was going to Ctrl+Click on the symbol, but provideDefinition just gets called again on that click.

So as far as I can tell, there is no reason for VSCode to be calling provideDefinition on Ctrl.

If this is intended behavior, provideDefinition should include an additional "context" argument describing how it got called, so that extensions can differentially handle that case.

Steps to Reproduce

In an extension, implement a DefinitionProvider.

In the provideDefinition method, add a console.log or similar so you can see when provideDefinition is being called.

When running the extension on Windows, hover the mouse over some symbol and press Ctrl. Observe that provideDefinition gets called, even though nothing is displayed in the editor.

VS Code version: Code - Insiders 1.84.0-insider (60182c7e1a666961ded4d0319c154f52d85daf30, 2023-10-30T23:41:49.215Z) OS version: Windows_NT x64 10.0.22631 Modes:

System Info |Item|Value| |---|---| |CPUs|12th Gen Intel(R) Core(TM) i7-12700KF (20 x 3610)| |GPU Status|2d_canvas: enabled
canvas_oop_rasterization: enabled_on
direct_rendering_display_compositor: disabled_off_ok
gpu_compositing: enabled
multiple_raster_threads: enabled_on
opengl: enabled_on
rasterization: enabled
raw_draw: disabled_off_ok
video_decode: enabled
video_encode: enabled
vulkan: disabled_off
webgl: enabled
webgl2: enabled
webgpu: enabled| |Load (avg)|undefined| |Memory (System)|63.82GB (39.77GB free)| |Process Argv|--crash-reporter-id d4e489a3-d7b4-40b6-8af5-10e59d0bb772| |Screen Reader|no| |VM|0%|
Extensions (60) Extension|Author (truncated)|Version ---|---|--- codesnap|adp|1.3.4 TabOut|alb|0.2.2 vscode-color-picker|Ant|0.0.4 tasks-shell-input|aug|1.8.2 change-color-format|bbu|1.2.0 comment-tagged-templates|bie|0.3.2 jsdoc-markdown-highlighting|bie|0.0.1 lit-html|bie|1.11.1 mermaid-markdown-syntax-highlighting|bpr|1.5.3 vscode-tailwindcss|bra|0.10.1 vscode-toggle-quotes|Bri|0.3.6 bscotch-stitch-vscode|bsc|1.52.0 path-intellisense|chr|2.8.5 vscode-eslint|dba|2.4.2 javascript-ejs-support|Dig|1.3.3 python-environment-manager|don|1.2.4 xml|Dot|2.5.1 vscode-html-css|ecm|1.13.1 prettier-vscode|esb|10.1.0 file-icons|fil|1.1.0 ghost-text|fre|1.4.0 copilot|Git|1.132.0 copilot-chat|Git|0.9.2023103102 copilot-labs|Git|0.15.1019 vscode-github-actions|git|0.26.2 todo-tree|Gru|0.0.226 rainbow-csv|mec|3.8.0 render-crlf|med|1.7.1 template-string-converter|meg|0.6.1 fluent-icons|mig|0.0.18 dotenv|mik|1.0.1 ecdc|mit|1.8.0 vscode-json5|mrm|1.0.0 vscode-scss|mrm|0.10.0 playwright|ms-|1.0.17 remote-containers|ms-|0.320.0 remote-wsl|ms-|0.81.8 vscode-typescript-next|ms-|5.3.20231030 vsliveshare|ms-|1.0.5892 tmlanguage|ped|1.0.0 material-icon-theme|PKi|4.31.0 prisma|Pri|5.5.2 binary-viewer|Qia|1.1.1 webgl-glsl-editor|rac|1.3.6 vscode-commons|red|0.0.6 vscode-xml|red|0.26.1 vscode-yaml|red|1.14.0 LiveServer|rit|5.7.9 rust-analyzer|rus|0.4.1714 markdown-preview-enhanced|shd|0.8.10 code-spell-checker|str|3.0.1 svelte-vscode|sve|107.12.0 even-better-toml|tam|0.19.2 vscode-sourcemaps-navigator|vlk|0.0.3 vscode-icons|vsc|12.6.0 volar|Vue|1.8.22 vscode-css-variables|vun|2.6.3 change-case|wma|1.0.0 markdown-all-in-one|yzh|3.5.1 html-css-class-completion|Zig|1.20.0 (1 theme extensions excluded)
jrieken commented 10 months ago

function would either return a location for userland symbols or open the browser to the docs for native symbols

Please don't open a browser for docs. Your extension might run remotely, e.g inside a docker container or headless server, and opening a browser there leads to nothing. Instead return an http url and see what happens. I'd rather support VS Code handling those.

Also note that any other extension can invoke your provider via API commands - so rule of thumb is to only return data and have no "side effects"

since Ctrl gets hit a lot while the cursor is essentially hovering over random symbols.

Yeah, that's a feature of previewing a resource inside the hover. Nothing we are going to change.

jrieken commented 10 months ago

If this is intended behavior, provideDefinition should include an additional "context" argument describing how it got called, so that extensions can differentially handle that case.

Disagree - a provider should not know how its results are going to be used. It's a data only contract, similar to a HTTP GET call. However, I am accepting a feature request to accept/handle http-references

adam-coster commented 10 months ago

@jrieken fair enough!

I would have preferred to be able to just send a URL back but I couldn't get that to do anything. The types indicate that a Location instance is all that can be returned by provideDefinition, and VSCode always tried to open those as files even when the Uri had an HTTP protocol.

I ended up using a separate command for this and messing with hotkeys a bit so that I could get the desired behavior without relying on side effects. It's a little clunky, but gets the job done.