1048598 - Terminal-spew: "System JS : WARNING resource://gre/modules/commonjs/toolkit/loader.js:410 - test for equality (==) mistyped as assignment (=)?"

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Daniel Holbert [:dholbert]]

Noticed these go by in my terminal-spew, on my debug Firefox build today (using mozilla-central):
{
System JS : WARNING resource://gre/modules/commonjs/toolkit/loader.js:410 - test for equality (==) mistyped as assignment (=)?
System JS : WARNING resource://gre/modules/commonjs/toolkit/loader.js:412 - test for equality (==) mistyped as assignment (=)?
System JS : WARNING resource://gre/modules/commonjs/toolkit/loader.js:419 - test for equality (==) mistyped as assignment (=)?
System JS : WARNING resource://gre/modules/commonjs/toolkit/loader.js:421 - test for equality (==) mistyped as assignment (=)?
System JS : WARNING resource://gre/modules/commonjs/toolkit/loader.js:462 - test for equality (==) mistyped as assignment (=)?
}

This is for code like...
> 409   let resolvedPath;
> 410   if (resolvedPath = loadAsFile(fullId))
> 411     return stripBase(rootURI, resolvedPath);
http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/toolkit/loader.js#409

...and it looks like the assignment is intentional, so the warnings are spurious.

I suspect we can silence the warnings by adding another layer of parenthesis.  That works for the equivalent warning in GCC, at least. (It's a hint to the compiler/interpreter "Yes, this is an assignment; this extra layer of parenthesis indicates that I'm interpreting the result of the assignment as a boolean condition.")

Comment 1

[Tomislav Jovanovic :zombie]

this doesn't happen in non-debug builds, right?

(that's a 7 months old "uplift sdk" bug, we have a new one each week)

Comment 2

[Daniel Holbert [:dholbert]]

I believe this is debug-only, yes.

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1570

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

The if/elses are intentional -- only one of the statements should run, by checking the validity of the resolution after each attempt. With these changes, if it can load via directory, that'll replace the first `if` of loading it as a file. This means if we have a `require("./x")`, it should first attempt to load x.js, and then x/ as a dir. If both exist, this means it'll default to a directory rather than a file -- this is not what we want. Not to mention the last previous `else` attempts to look it up as a node_modules dependency, which this will always do. I think this will fail tests if we test for these cases, which I believe we do.

The parentheses changes look fine to quell the warning messages, though, but pretty sure the `else` removing will break.

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Additionally, I think the ifs in the node module lookup loop should be if/else if for the same reasons, but guessing we don't have a test case where a node_module is using require("./x") where x is both a js file and a directory

Comment 6

[Daniel Holbert [:dholbert]]

You seem to be saying that the "else" keywords affect the logic here -- but I don't think that's possible, since else-after-return is strictly unnecessary. At least, the first entry in the Mozilla Coding Style C++ is the following:
> Don't put an else right after a return. Delete the else,
> it's unnecessary and increases indentation level.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Of course, this bug here is about JS, not C++, but (unless I'm missing some fancy JS feature) this C++ guideline should be equally true of JS.

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

You're right -- didn't notice the return

**coffee**

Comment 8

[poiru]

I missed this and filed bug 1049312 yesterday. It takes a different approach (it merely adds the extra parenthesis) and also fixes a couple of other things.

If Erik's pull request is merged, I'll rebase my pull request for the the other warnings.

Comment 9

[Daniel Holbert [:dholbert]]

Looks like poiru's fix for this (on bug 1049312) was already merged, per bug 1049312 comment 2.

So, as-filed, this bug can be resolved as a dupe of bug 1049312.

(However, we probably should still land the "fix else-after-return" aspect of the patch that we had here. Erik, feel free to just retarget this bug to be about that, if you like; or, maybe you'd prefer to spin off a separate bug for that.)

Comment 10

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/24162a4fa5e91c28be395e56beeac0f7e2e818c3
Bug 1048598 removing an unnecessary else statement r=@jsantell