-
Notifications
You must be signed in to change notification settings - Fork 0
feat: glob pattern support in executionEnvironments root #1
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,6 +7,9 @@ | |||||||||||||||||||||||||
| * Class that holds the configuration options for the analyzer. | ||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| import { globSync } from 'node:fs'; | ||||||||||||||||||||||||||
| import { matchesGlob } from 'node:path/posix'; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| import { ImportLogger } from '../analyzer/importLogger'; | ||||||||||||||||||||||||||
| import { getPathsFromPthFiles } from '../analyzer/pythonPathUtils'; | ||||||||||||||||||||||||||
| import * as pathConsts from '../common/pathConsts'; | ||||||||||||||||||||||||||
|
|
@@ -27,7 +30,7 @@ import { PythonVersion, latestStablePythonVersion } from './pythonVersion'; | |||||||||||||||||||||||||
| import { ServiceKeys } from './serviceKeys'; | ||||||||||||||||||||||||||
| import { ServiceProvider } from './serviceProvider'; | ||||||||||||||||||||||||||
| import { Uri } from './uri/uri'; | ||||||||||||||||||||||||||
| import { FileSpec, getFileSpec, isDirectory } from './uri/uriUtils'; | ||||||||||||||||||||||||||
| import { FileSpec, getFileSpec, getWildcardRoot, isDirectory } from './uri/uriUtils'; | ||||||||||||||||||||||||||
| import { userFacingOptionsList } from './stringUtils'; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // prevent upstream changes from sneaking in and adding errors using console.error, | ||||||||||||||||||||||||||
|
|
@@ -66,6 +69,8 @@ export class ExecutionEnvironment { | |||||||||||||||||||||||||
| // tools or playgrounds). | ||||||||||||||||||||||||||
| skipNativeLibraries: boolean; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| rootGlob?: string; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Default to "." which indicates every file in the project. | ||||||||||||||||||||||||||
| constructor( | ||||||||||||||||||||||||||
| name: string, | ||||||||||||||||||||||||||
|
|
@@ -1526,13 +1531,21 @@ export class ConfigOptions { | |||||||||||||||||||||||||
| // execution environment is used. | ||||||||||||||||||||||||||
| findExecEnvironment(file: Uri): ExecutionEnvironment { | ||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||
| this.executionEnvironments.find((env) => { | ||||||||||||||||||||||||||
| const envRoot = Uri.is(env.root) ? env.root : this.projectRoot.resolvePaths(env.root || ''); | ||||||||||||||||||||||||||
| return file.startsWith(envRoot); | ||||||||||||||||||||||||||
| }) ?? this.getDefaultExecEnvironment() | ||||||||||||||||||||||||||
| this.executionEnvironments.find((env) => this._fileMatchesEnvironment(file, env)) ?? | ||||||||||||||||||||||||||
| this.getDefaultExecEnvironment() | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private _fileMatchesEnvironment(file: Uri, env: ExecutionEnvironment): boolean { | ||||||||||||||||||||||||||
| if (env.rootGlob !== undefined) { | ||||||||||||||||||||||||||
| const relative = this.projectRoot.getRelativePath(file)?.slice(2); | ||||||||||||||||||||||||||
| if (relative === undefined) return false; | ||||||||||||||||||||||||||
|
Comment on lines
+1541
to
+1542
|
||||||||||||||||||||||||||
| const relative = this.projectRoot.getRelativePath(file)?.slice(2); | |
| if (relative === undefined) return false; | |
| const relativePath = this.projectRoot.getRelativePath(file); | |
| if (relativePath === undefined) return false; | |
| const relative = relativePath.startsWith('./') ? relativePath.slice(2) : relativePath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getRelativePath always starts with ./ — the implementation is ['.', ...components].join('/'), so the prefix is hardcoded. When the file is not a child, it returns undefined (handled by the ?. chain). The .slice(2) pattern is used throughout the codebase.
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path normalization logic assumes that getRelativePath always returns a path in POSIX format (with forward slashes). However, on Windows systems, if the internal implementation uses backslashes, this could cause matching failures. Verify that the paths are consistently normalized to use forward slashes before passing to matchesGlob, which expects POSIX-style paths.
| return matchesGlob(relative, env.rootGlob + '/**'); | |
| const posixRelative = relative.replace(/\\/g, '/'); | |
| return matchesGlob(posixRelative, env.rootGlob + '/**'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getRelativePath always returns a ./-prefixed path — the implementation joins with ['.', ...components].join('/'), so the ./ prefix is hardcoded. Confirmed by unit tests (uri.test.ts:865) which assert ./d/e/f. No backslash path is possible here.
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The findExecEnvironment method is called frequently during analysis (on every file access), and with this change it will perform glob pattern matching via matchesGlob and relative path computation for every file when glob patterns are used. This could impact performance in large projects. Consider implementing caching of the file-to-environment mapping, or at least caching the relative path computation since projectRoot and file paths don't change during analysis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous code ran file.matchesRegex(env.rootFileSpec.regExp) per file access — same per-call cost. matchesGlob is a native Node.js function with no measurable performance difference.
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern concatenation env.rootGlob + '/**' assumes the glob pattern should match all descendant files. However, this changes the semantics of the original glob pattern. For example, if a user specifies 'src/*/utils' to match only immediate children, this will be transformed to 'src/*/utils/**', which would match all descendants. Consider whether the pattern should be matched as-is, or document this behavior clearly if the transformation is intentional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By design — root in execution environments means "all files under directories matching this pattern". Without /**, a file like src/foo/utils/helper.py would not match root src/*/utils. The /** suffix is necessary to match descendants.
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a glob pattern in extraPaths matches no directories, the result is that no paths are added to the extraPaths array. While this might be intentional, it could lead to silent failures where the user expects paths to be added but none are. Consider adding a warning when a glob pattern matches zero paths to help users debug their configuration.
| for (const match of globSync(path, { cwd: configDirUri.getFilePath() })) { | |
| const matches = globSync(path, { cwd: configDirUri.getFilePath() }); | |
| if (!matches || matches.length === 0) { | |
| console.error( | |
| `Config "extraPaths" glob pattern "${path}" did not match any directories (cwd="${configDirUri.getFilePath()}").` | |
| ); | |
| return; | |
| } | |
| for (const match of matches) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glob patterns legitimately match nothing in some environments (e.g. **/fixtures in a project without fixture directories). Warning on zero matches would be noisy. Silently skipping is the correct behaviour.
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the root pattern is just "**", getWildcardRoot will return the project root, and the glob pattern matching will attempt to match all files in the project. However, this pattern might not be meaningful for execution environments and could lead to confusion. Consider documenting this behavior or validating against patterns that are too broad.
| if (envObj.root && typeof envObj.root === 'string') { | |
| if (envObj.root && typeof envObj.root === 'string') { | |
| // When the root pattern is just "**", getWildcardRoot will return the project root, | |
| // and the glob pattern matching will attempt to match all files in the project. | |
| // This can be broader than intended for an execution environment, so document this | |
| // behavior for users configuring executionEnvironments. | |
| if (envObj.root.trim() === '**') { | |
| console.error( | |
| `Config executionEnvironments index ${index}: root value "**" will match all files in the project; ` + | |
| 'consider using a more specific root.' | |
| ); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By design — ** matching all files is standard glob semantics. If a user writes root: "**", they intend to match everything. Warning on valid input would be over-engineering.
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,7 +12,11 @@ import assert from 'assert'; | |||||||||
| import { AnalyzerService } from '../analyzer/service'; | ||||||||||
| import { deserialize, serialize } from '../backgroundThreadBase'; | ||||||||||
| import { CommandLineOptions, DiagnosticSeverityOverrides } from '../common/commandLineOptions'; | ||||||||||
| import { ConfigOptions, ExecutionEnvironment, getStandardDiagnosticRuleSet } from '../common/configOptions'; | ||||||||||
| import { | ||||||||||
| ConfigOptions, | ||||||||||
| ExecutionEnvironment, | ||||||||||
| getStandardDiagnosticRuleSet, | ||||||||||
| } from '../common/configOptions'; | ||||||||||
| import { ConsoleInterface, NullConsole } from '../common/console'; | ||||||||||
| import { TaskListPriority } from '../common/diagnostic'; | ||||||||||
| import { combinePaths, normalizePath, normalizeSlashes } from '../common/pathUtils'; | ||||||||||
|
|
@@ -740,4 +744,76 @@ describe(`config test'}`, () => { | |||||||||
| shouldRunAnalysis: () => true, | ||||||||||
| }); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| describe('glob root support', () => { | ||||||||||
| function setupExecEnvConfig(roots: ({ root: string } & Record<string, unknown>)[]) { | ||||||||||
| const cwd = UriEx.file(normalizePath(process.cwd())); | ||||||||||
| const configOptions = new ConfigOptions(cwd); | ||||||||||
| const json = { executionEnvironments: roots }; | ||||||||||
| const fs = new TestFileSystem(false); | ||||||||||
| const console = new ErrorTrackingNullConsole(); | ||||||||||
| const sp = createServiceProvider(fs, console); | ||||||||||
| configOptions.initializeFromJson(json, cwd, sp, new NoAccessHost()); | ||||||||||
| configOptions.setupExecutionEnvironments(json, cwd, console); | ||||||||||
| return { cwd, configOptions, console }; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| test.each([ | ||||||||||
| 'src', '**/tests', 'src/*/utils', 'src/test?', '***/tests', ' ', | ||||||||||
|
||||||||||
| 'src', '**/tests', 'src/*/utils', 'src/test?', '***/tests', ' ', | |
| 'src', '**/tests', 'src/*/utils', 'src/test?', '***/tests', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional edge case coverage — the test documents that truthy whitespace strings pass through the if (envObj.root && typeof envObj.root === 'string') gate. Whether to add trim/validation is a separate discussion.
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test includes '***/tests' which is not a valid glob pattern (three asterisks). Consider either removing this test case if it's meant to test invalid patterns, or replacing it with a valid pattern if it's meant to test valid patterns.
| 'src', '**/tests', 'src/*/utils', 'src/test?', '***/tests', ' ', | |
| 'src', '**/tests', 'src/*/utils', 'src/test?', '**/*/tests', ' ', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional — the test proves matchesGlob handles unusual input gracefully rather than crashing. The *** pattern is processed without error (confirmed by passing tests). This is robustness coverage, not an endorsement of the pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from using
execEnv.root?.toString()as a lookup key to using array indices introduces a subtle correctness issue. With glob roots, multiple execution environments could have the samerootURI (the project root), making the old approach unreliable. However, the new index-based approach assumes the execution environments array order and contents remain stable between whenensurePartialStubPackagesis called and when it's handled in the background thread. If the execution environments are modified or reordered between these points, this could lead to the wrong environment being used. Consider adding validation or documentation about this assumption.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By design — execution environments are set once during config loading and are immutable during analysis. The old
root.toString()key approach was broken for glob roots since multiple envs can share the same wildcard root URI. Index-based lookup is the correct fix for this.