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.

Sign up for GitHub

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

Fix ${workspaceFolder} being added to browse.path if ${workspaceFolder}/* is used. #13324

Closed
wants to merge 2 commits into from
Closed

Fix ${workspaceFolder} being added to browse.path if ${workspaceFolder}/* is used. #13324

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b120b80
Fix ${workspaceFolder} being added if ${workspaceFolder}/* is used.
sean-mcmanus Feb 28, 2025
495932c
Fix linter.
sean-mcmanus Feb 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions Extension/src/LanguageServer/configurations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Colengms Sure, yeah, it sounds like this won't be needed.

configuration.browse.path = configuration.includePath.slice(0);
if (configuration.includePath.findIndex((value: string) =>
!!value.match(/^\$\{(workspaceRoot|workspaceFolder)\}(\\\*{0,2}|\/\*{0,2})?$/g)) === -1
) {
configuration.browse.path.push("${workspaceFolder}");
if (this.rootUri) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to continue to work with ${workspaceFolder}/** because that's the default entry for includePath.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolveVariables does not resolve ${workspaceFolder}. That happens in the native code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bobbrow No, that changed. It resolves it in TypeScript. That's what I saw when I debugged it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Called by updateConfigurationPathsArray, which is called by updateServerOnFolderSettingsChange, which is the function that was modified in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const normalizedRootUriFsPath: string = escapeStringRegExp(this.rootUri.fsPath.replace(/\\/g, "/"));
const regexPattern = `^${normalizedRootUriFsPath}(\\*{0,2}|\/\\*{0,2})?$`;
const regex = new RegExp(regexPattern, "g");
if (configuration.includePath.findIndex((value: string) =>
!!value.match(regex)) === -1
) {
configuration.browse.path.push("${workspaceFolder}");
}
}
} else {
configuration.browse.path = ["${workspaceFolder}"];
Expand Down