function does not always return a value warning confused by nested function

RESOLVED WORKSFORME

Status

()

Core
JavaScript Engine
--
trivial
RESOLVED WORKSFORME
10 years ago
4 years ago

People

(Reporter: bc, Unassigned)

Tracking

({testcase})

Trunk
x86
All
testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

10 years ago
Created attachment 312385 [details]
testcase

function foo()
{
  return 0;

  function bar()
  {
  }
}
foo();

Warning: function foo does not always return a value
Source File: doesnotreturnavalue.html
Line: 9, Column: 2
Source Code:
}
js> function foo()
{
  return 0;

  function bar()
  {
  }
}
js> foo();
0

No warnings in the error console when I load the testcase either.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite?
Resolution: --- → WORKSFORME

Comment 2

4 years ago
Created attachment 8402968 [details]
screenshot of warning in console

I see this warning today in Firefox 28.0 for Mac when I click on the attached testcase. Regression?

It looks like I’m unable to reopen the issue. Would someone else, please? Thanks.
Flags: needinfo?(general)
(Reporter)

Comment 3

4 years ago
jmjacobs, you probably have javascript.options.strict set to true? I was able to reproduce it in an Linux Nightly/31 on a build with that option set but not without it.

Looking at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions_and_function_scope/Strict_mode it is not clear to me how strict mode should handle this script.

Waldo, do you think this should be reopened?
Flags: needinfo?(general) → needinfo?(jwalden+bmo)

Comment 4

4 years ago
Yes, I have javascript.options.strict set to true. I must have done it a long time ago and forgotten.

Regardless, I’m certain that the warning should not be printed. Neither strict mode (as defined by Mozilla) nor ES6 forbid or even discourage inner function declarations as the last statements in their containing functions. Function declaration hoisting is never dangerous. Only variable hoisting is considered dangerous, since the value is not bound to the name until execution reaches the declaration.

The statement ”strict mode prohibits function statements not at the top level of a script or function” in the strict mode reference doc you linked to just forbids putting function declarations within blocks, loops, case statements, etc.

Comment 5

4 years ago
Strict mode (the spec concept) and the unfortunately-named javascript.options.strict are different concepts.  :-\  Strict mode is a specified behavior.  The strict option is our own half-baked set of heuristics -- sometimes fully principled, sometimes not (as here).  Strict mode should not have anything to say about failure ("failure") of any function to return a value.

This is still valid, but these days (I didn't know this, had to debug it myself) you have to pass -w to the shell to get a warning with options("strict"):

[jwalden@find-waldo-now src]$ dbg/js/src/js -w
js> options("strict"); void 0
js> function f() { return 0; function g() {} }
typein:2:13 strict warning: function f does not always return a value:
typein:2:13 strict warning: function f() { return 0; function g() {} }
typein:2:13 strict warning: .............^

Alternatively there's options("werror"), but that's by its very nature (converting unspecified warning conditions into errors) not-to-spec, so I hesitate to suggest it too much lest people think it's something they should use at all.

The relevant code that would need changing to address this is HasFinalReturn in Parser.cpp.  Honestly, tho, I'm not sure I much like this warning if it's going to try to hackishly determine falling-off-the-endness without some sort of dominator analysis or something to show it's actually possible.  But I have better things to do than argue the case as far I'd need to actually get that to happen, I'm pretty sure.
Status: RESOLVED → REOPENED
Flags: needinfo?(jwalden+bmo)
Resolution: WORKSFORME → ---
(Assignee)

Updated

4 years ago
Assignee: general → nobody

Comment 6

4 years ago
Fixed by removal of the relevant warning in 1046964.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago4 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.