3d4e99fc6c
String interpolation and table.getn/et al deprecation has been live for a while. getfenv/setfenv linter diagnostics is going to ship in the next release, so preemptively marking this as Implemented as well.
2.2 KiB
Deprecate getfenv/setfenv
Status: Implemented
Summary
Mark getfenv/setfenv as deprecated
Motivation
getfenv and setfenv are problematic for a host of reasons:
- They allow uncontrolled mutation of global environment, which results in deoptimization; various important performance features like builtin calls or imports are disabled when these functions are used.
- Because of the uncontrolled mutation code that uses getfenv/setfenv can't be typechecked correctly; in particular, injecting new globals is going to produce "unknown globals" warnings, and modifying existing globals can trivially violate soundness wrt type checking
- While these functions can be used for good (once you ignore the issues above), such as custom module systems, statistically speaking they are mostly used to obfuscate code to hide malicious intent.
Design
We will mark getfenv and setfenv as deprecated. The only consequence of this change is that the linter will start emitting warnings when they are used.
Removing support for getfenv/setfenv, while tempting, is not planned in the foreseeable future because it will cause significant backwards compatibility issues.
Drawbacks
There are valid uses for getfenv/setfenv, that include extra logging (in Roblox code this manifests as getfenv(1).script), monkey patching for mocks in unit tests, and custom
module systems that inject globals into the calling environment. We do have a replacement for logging use cases, debug.info, and we do have an officially recommended replacement
for custom module systems, which is to use require that doesn't result in issues that fenv modification carries and can be understood by the type checker, we do not have an
alternative for mocks. As such, testing frameworks that implement mocking via setfenv/getfenv will need to use --!nolint DeprecatedGlobal to avoid this warning.
Alternatives
Besides the obvious alternative "do nothing", we could also consider implementing Lua 5.2 support for _ENV. However, since we do not have a way to load script files other than
via require that doesn't support _ENV, and loadstring is supported but discouraged, we do not currently plan to implement _ENV although it's possible that this will happen
in the future.