Fix ${workspaceFolder} being added to browse.path if ${workspaceFolder}/* is used. #13324
Conversation
sean-mcmanus
changed the title
| @@ -998,10 +998,15 @@ export class CppProperties { | |||
| } else if (configuration.includePath) { | |||
| // If the user doesn't set browse.path, copy the includePath over. Make sure ${workspaceFolder} is in there though... | |||
There was a problem hiding this comment.
After my current PR on the native side goes in, I want to remove this section, making it the responsibility of the native side to populate the browse paths with the include paths, if the browse path is undefined. That way, I can ensure any include paths ingested via compiler arguments get into the browse path also (if not explicitly set). Since this would be removed soon (before the next insiders, if possible), maybe this fix isn't needed.
There was a problem hiding this comment.
@Colengms Sure, yeah, it sounds like this won't be needed.
| !!value.match(/^\$\{(workspaceRoot|workspaceFolder)\}(\\\*{0,2}|\/\*{0,2})?$/g)) === -1 | ||
| ) { | ||
| configuration.browse.path.push("${workspaceFolder}"); | ||
| if (this.rootUri) { |
There was a problem hiding this comment.
Do we know if this behavior changed due to a change in vscode itself, or something else?
if it was a change in vscode, did we want to still make it work with (workspaceRoot|workspaceFolder) as well in case the newer extension was run on an older version of vscode?
There was a problem hiding this comment.
It needs to continue to work with ${workspaceFolder}/** because that's the default entry for includePath.
There was a problem hiding this comment.
@fearthecowboy @bobbrow The change is that this the ${workspaceFolder} variable is resolved at this point by our own code due to calls to the resolveVariable or whatever it's called.
There was a problem hiding this comment.
resolveVariables does not resolve ${workspaceFolder}. That happens in the native code.
There was a problem hiding this comment.
@bobbrow No, that changed. It resolves it in TypeScript. That's what I saw when I debugged it.
There was a problem hiding this comment.
Called by updateConfigurationPathsArray, which is called by updateServerOnFolderSettingsChange, which is the function that was modified in this PR.
There was a problem hiding this comment.
@bobbrow I recall Colen talking about changing it to not send ${workspaceFolder} to the cpptools side and resolving it on the TypeScript side instead, since it wasn't necessary keep ${workspaceFolder} around.
There was a problem hiding this comment.
Well, I'm not sure when that changed, but I suppose if it has, then yes this code will need to be updated to deal with it. But I think it doesn't make a lot of sense to be inserting ${workspaceFolder} at a point when ${workspaceFolder} is expected to have been resolved away, right? We're in a halfway state now and that causes confusion.
There was a problem hiding this comment.
@bobbrow Sure, but my PR just fixes it so that it doesn't add ${workspaceFolder} -- the pre-existing behavior already adds workspace folder. And note I've moved this to draft since Colen says one of his pending PRs would be causing this code to not be used so it might not be needed.
There was a problem hiding this comment.
The native side will resolve variables (${workspaceRoot}, ${workspaceFolder}, ${vcpkgRoot}) when ingesting browse paths (and include paths, mac framework paths, forced includes, compile commands paths, compiler path, compiler args, and the database filename) from the base configuration. Perhaps processing was added on the TypeScript side to address needing one of these values (compiler path in a build task maybe?) with it already resolved.
This was working with 1.16.3, but then some change caused the ${workspaceFolder} variable to be resolved before it checked for ${workspaceFolder} at this spot, so the regex match would always fail. This just fixes it so that the previously working case works now.