Page MenuHomePhabricator

Lua (Scribunto): require incorrectly returns true when a module has a return value of false
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • When loading a module which has a return value of false, require incorrectly returns true.

What happens?:

  • The module returns false instead of true.

What should have happened instead?:

  • require should return false. For comparison, package.loaders[2](modname)() will return false, as expected.

Software version (on Special:Version page; skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):

  • This is down to a bug in the custom require function defined at line 89 of package.lua.
  • When first loaded, a module's key in package.loaded is set to a local function called sentinel at line 100, which essentially acts as a placeholder. The module's actual return value is then loaded as res at line 103.
  • The bug occurs in the if-condition at line 104 (if res then), which should instead be if res ~= nil then.
  • Since modules which return false fail that if-condition, require detects that the value in package.loaded is still sentinel at line 108, and replaces it with the default value true. This is presumably so that modules with no return value are still able to be cached.
  • The if-condition at line 93 should also be changed from if p then to if p ~= nil then, since that's when require initially checks package.loaded to see if a pre-loaded value is available to return.

Event Timeline

Change #1030568 had a related patch set uploaded (by Theknightwho; author: Theknightwho):

[mediawiki/extensions/Scribunto@master] Avoid modules that should return `false` wrongly returning `true`

https://gerrit.wikimedia.org/r/1030568

Change #1030568 merged by jenkins-bot:

[mediawiki/extensions/Scribunto@master] Avoid modules that should return `false` wrongly returning `true`

https://gerrit.wikimedia.org/r/1030568

Pppery claimed this task.
Pppery reassigned this task from Pppery to Theknightwho.
Pppery subscribed.