Allow browse configurations to be set per workspace folder

[sherryyshi]

In V2 of the API, WorkspaceBrowseConfiguration is set globally for the entire workspace. However, this would lead to a lot of duplication for multi-root workspaces. It would be great to be able to configure the browse.path for each workspace folder via the API.

[busticated]

@sean-mcmanus i'm curious if you have a recommendation for how to handle this situation? should we return false from canProvideBrowseConfiguration() when workspaceFolders.length is greater than 1? something else?

my use case: i manage a number of settings on behalf of my users at the WorkspaceFolder level. these settings impact intellisense behavior (for example, compilerPath may be different per-folder as well as the raw browsePath values).

fwiw, i'm also providing configuration via provideConfigurations(<uris>). since this api exposes the uri of the active file(s), i'm able to work out the folder-specific settings i need to use to build the intellisense configuration values.

[sean-mcmanus]

@busticated I think you should return the same WorkspaceBrowseConfiguraiton for each workspace folder if it's not possible to provide per-folder (i.e. return the superset). From what I see in the API and our code, it seems like the WorkspaceBrowseConfiguration should be settable per-workspaceFolder. @bobbrow Do you know more?

[busticated]

@sean-mcmanus

return the superset

afaict, that's not possible since provideBrowseConfiguration() (docs) needs to return a WorkspaceBrowseConfiguration object (docs) which only allows for a singled compilerPath value. also note that provideBrowseConfiguration() does not receive the uri(s) for either the target workspace folder or the currently active file so there's no obvious way to target the config values to a specific folder / project's .vscode/settings.json

i vaguely recall reading / hearing that browse was something of a legacy thing that you were encouraging people to avoid. can you clarify what benefit it offers above and beyond the SourceFileConfigurationItem / SourceFileConfiguration values?

[sean-mcmanus]

The SourceFileConfiguration is used by IntelliSense for parsing a single translation unit, but the WorkspaceBrowseConfiguration.browsePath is required to have global symbols available (i.e. the Outline view or Go to Definition). The browsePath is recursive, so it only needs to be the root of the paths (whereas the SourceFileConfiguration isn't normally recursive). In the non-custom provider case, we just copy over the includePath to the browse.path, so most users don't have to customize it. For the custom config case, the code isn't taking the include path and adding it to the browse path.

Ah, I see -- if you have different compilerPaths for each workspace (or you have symbols you don't want populated in different workspaces) then giving a superset won't give good results. It seems like we just need to improve the API.

[busticated]

browsePath is required to have global symbols available (i.e. the Outline view or Go to Definition)

running here w/

async canProvideBrowseConfiguration(){
    return false;
}

yet right-click > "Peek Definition" (as well as "Go to Definition" and the "Peek" / "Go to Declaration" options) still appear to work fine 🤔

[sean-mcmanus]

Is that when the result is in a different translation unit? If the result is in the same TU, we can use the results from IntelliSense (SourceFileConfiguration). If the result is in a different TU, maybe we're falling back to a default (I don't see any fall back like that in the code).

[sean-mcmanus]

Ah, I found the fall back code -- if there's no custom browsePath it will use the defaults from the c_cpp_properties (and whatever the config's default compiler is for system paths).

[busticated]

if there's no custom browsePath it will use the defaults from the c_cpp_properties

hm, ok. since i'm using the provideConfigurations() api, will it use the values i provide there? reading this, i see: "If omitted, includePath will be used as the path" so i assume it will?

[sean-mcmanus]

Our current implementation, doesn't use the configuration info from provideConfigurations to provide the browse.path, but we could change it so that if a custom configuration has been provided, then we could either use info from 1 file or combine all of them to create a browse.path (i.e. we could even query all the compilers to form the superset)...we have a work item tracking this for compile_commands.json at microsoft/vscode-cpptools#1715, but the implementation would be similar for custom configuration providers. The "includePath will be used as the path" is referring to when the c_cpp_properties.json is used.

[busticated]

ok, with that in mind here's what i'm thinking:

until there's better API support for setting WorkspaceBrowseConfiguration on a per-folder basis, my canProvideBrowseConfiguration() implementation will only return true when i'm working with a single-root workspace (iow, workspace.workspaceFolders.length === 1).

when i'm in a single-root workspace, i'll return a WorkspaceBrowseConfiguration object with values based on the current folder's .vscode/settings.json data. otherwise, i'll provide nothing and rely on the fallback handling for browsePath, etc.

that sound reasonable?

[sean-mcmanus]

Yeah, that sounds reasonable.

[bobbrow]

Also, if you would rather not write a c_cpp_properties.json file for each of the workspace folders, you can use the C_Cpp.default. settings instead (which are settable per folder).

[busticated]

if you would rather not write a c_cpp_properties.json file for each of the workspace folders

well, we're using the provider APIs to avoid doing that but are you suggesting we should?

[bobbrow]

I was just referring to the multi-root case where you are not providing a browse config. I was thinking that you didn't write c_cpp_properties.json files for folders, so that would mean the only way to get the correct browse information (including differing compilers) set for your folders would be to use the C_Cpp.default. settings.