feat: glob pattern support in executionEnvironments root#1
feat: glob pattern support in executionEnvironments root#1
Conversation
77caeef to
8f740a9
Compare
8f740a9 to
1563453
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds glob pattern support to the executionEnvironments[].root configuration field in basedpyright. The feature enables users to apply diagnostic rules to files matching a glob pattern while maintaining import resolution from the project root. This decouples diagnostic scoping from import resolution paths.
Changes:
- Adds runtime glob matching for execution environment roots using
matchesGlobfrom Node.js - Changes background analysis to use execution environment indices instead of root path strings for identification
- Updates schema and documentation to describe the new glob pattern support and behavior
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/vscode-pyright/schemas/pyrightconfig.schema.json | Updates JSON schema to document glob pattern support for root and extraPaths fields |
| packages/pyright-internal/src/common/configOptions.ts | Implements glob matching logic for execution environments and glob expansion for extraPaths |
| packages/pyright-internal/src/analyzer/backgroundAnalysisProgram.ts | Changes from root-based to index-based execution environment identification |
| packages/pyright-internal/src/backgroundAnalysisBase.ts | Updates IPC interface to pass execution environment index instead of root string |
| packages/pyright-internal/src/analyzer/importResolver.ts | Changes partial stub scanning check to use path-based lookup instead of environment-based |
| packages/pyright-internal/src/tests/config.test.ts | Adds comprehensive test coverage for glob pattern matching, serialization, and environment selection |
| packages/pyright-internal/src/tests/importResolver.test.ts | Adds tests verifying import resolution behavior for glob vs. plain roots |
| docs/configuration/config-files.md | Documents glob pattern support and updates examples to demonstrate the feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (env.rootGlob !== undefined) { | ||
| const relative = this.projectRoot.getRelativePath(file)?.slice(2); | ||
| if (relative === undefined) return false; | ||
| return matchesGlob(relative, env.rootGlob + '/**'); |
There was a problem hiding this comment.
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.
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.
| } | ||
|
|
||
| test.each([ | ||
| 'src', '**/tests', 'src/*/utils', 'src/test?', '***/tests', ' ', |
There was a problem hiding this comment.
The test includes a whitespace-only string (' ') as a valid root pattern. This doesn't seem like a meaningful pattern and could indicate a validation issue. Consider adding validation to reject empty or whitespace-only root patterns with a helpful error message.
| 'src', '**/tests', 'src/*/utils', 'src/test?', '***/tests', ' ', | |
| 'src', '**/tests', 'src/*/utils', 'src/test?', '***/tests', |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| test.each([ | ||
| 'src', '**/tests', 'src/*/utils', 'src/test?', '***/tests', ' ', |
There was a problem hiding this comment.
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.
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.
| const execEnvIndex = this.configOptions.getExecutionEnvironments().indexOf(execEnv); | ||
| this._backgroundAnalysis?.ensurePartialStubPackages(execEnvIndex); |
There was a problem hiding this comment.
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 same root URI (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 when ensurePartialStubPackages is 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.
| const execEnvIndex = this.configOptions.getExecutionEnvironments().indexOf(execEnv); | |
| this._backgroundAnalysis?.ensurePartialStubPackages(execEnvIndex); | |
| const execEnvs = this.configOptions.getExecutionEnvironments(); | |
| const execEnvIndex = execEnvs.indexOf(execEnv); | |
| if (execEnvIndex >= 0) { | |
| // Note: This relies on the execution environments array remaining stable between | |
| // the time this request is enqueued and when it is processed in the background | |
| // analysis. If that assumption changes, this index-based lookup must be revisited. | |
| this._backgroundAnalysis?.ensurePartialStubPackages(execEnvIndex); | |
| } |
There was a problem hiding this comment.
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.
| private _fileMatchesEnvironment(file: Uri, env: ExecutionEnvironment): boolean { | ||
| if (env.rootGlob !== undefined) { | ||
| const relative = this.projectRoot.getRelativePath(file)?.slice(2); | ||
| if (relative === undefined) return false; | ||
| return matchesGlob(relative, env.rootGlob + '/**'); |
There was a problem hiding this comment.
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.
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.
| @@ -2043,7 +2070,8 @@ export class ConfigOptions { | |||
|
|
|||
| // Validate the root. | |||
| if (envObj.root && typeof envObj.root === 'string') { | |||
There was a problem hiding this comment.
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.
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.
| const relative = this.projectRoot.getRelativePath(file)?.slice(2); | ||
| if (relative === undefined) return false; |
There was a problem hiding this comment.
The logic relative.slice(2) removes the first two characters from the relative path (likely "./" prefix), but this assumes getRelativePath always returns a path starting with "./". If the file is at the project root itself, or if the implementation changes, this could cause incorrect behavior. Consider using a more robust approach such as checking if the path starts with "./" before slicing, or using path manipulation utilities to ensure correctness.
| 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.
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.
| if (env.rootGlob !== undefined) { | ||
| const relative = this.projectRoot.getRelativePath(file)?.slice(2); | ||
| if (relative === undefined) return false; | ||
| return matchesGlob(relative, env.rootGlob + '/**'); |
There was a problem hiding this comment.
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.
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.
| private _resolveExtraPath(path: string, configDirUri: Uri, out: Uri[], console: ConsoleInterface) { | ||
| if (/[*?]/.test(path)) { | ||
| try { | ||
| for (const match of globSync(path, { cwd: configDirUri.getFilePath() })) { |
There was a problem hiding this comment.
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.
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.
Summary
Adds glob pattern support to
executionEnvironments[].root. When a glob is used, import resolution falls back to the project root — decoupling "which files get these diagnostic rules" from "where imports resolve from".Closes DetachHead#668, DetachHead#1042, DetachHead#1217, DetachHead#307
Test plan