feat: support DisableBuiltin("$env") to hide the $env variable - #949
feat: support DisableBuiltin("$env") to hide the $env variable#949boinger wants to merge 4 commits into
Conversation
Sorry, something went wrong.
$env is a hardcoded special variable, not a regular builtin, so
DisableBuiltin("$env") previously had no effect. Check config.Disabled
before each $env special-case handler in the checker and compiler.
When disabled, $env falls through to normal identifier resolution
(errors in strict mode as "unknown name $env"), consistent with how
DisableBuiltin works for regular builtins.
Fixes expr-lang#710
There was a problem hiding this comment.
Pull request overview
This PR makes the hardcoded special variable $env respect DisableBuiltin("$env"), allowing users to hide $env (e.g., to avoid naming conflicts or to restrict environment access) and have it fall back to normal identifier resolution.
Changes:
- Guard $env special-case handling in the checker so it only applies when $env is not disabled.
- Guard $env bytecode emission (OpLoadEnv) in the compiler so it only applies when $env is not disabled.
- Add regression tests covering $env disabled/enabled behaviors across identifier, member access, call, and get($env, ...) paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| checker/checker.go | Adds !config.Disabled["$env"] guards around $env special handling so disabling causes normal name resolution/errors. |
| compiler/compiler.go | Prevents OpLoadEnv emission when $env is disabled, aligning runtime behavior with checker resolution. |
| test/issues/710/issue_test.go | Adds coverage for strict and non-strict behavior when $env is disabled, plus default enabled regressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
sanmaxdev
left a comment
There was a problem hiding this comment.
Checked the disabled $env path across the checker and compiler. In non-strict mode it now compiles as a normal unresolved identifier instead of emitting OpLoadEnv, while the default path remains unchanged.
Verified:
- go test ./...
- go vet ./...
- go test -race -count=1 ./checker ./compiler ./test/issues/710
- git diff --check upstream/master...HEAD
Sorry, something went wrong.
|
Will merge it soon. |
Sorry, something went wrong.
Summary
$env is a hardcoded special variable with custom handling in the checker and compiler. It's not in the Builtins map, so DisableBuiltin("$env") previously had no effect.
This PR checks config.Disabled["$env"] before each $env special-case handler. When disabled, $env falls through to normal identifier resolution, which errors in strict mode as "unknown name $env". This is consistent with how DisableBuiltin works for regular builtins.
Changes
Use case
Users embedding expr as a DSL may have domain variables that conflict with $env, or may want to restrict access to the full environment object for sandboxing.
Fixes #710