Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EnvDeleter should return true #7960

Closed
iamstolis opened this issue Aug 3, 2016 · 8 comments
Closed

EnvDeleter should return true #7960

iamstolis opened this issue Aug 3, 2016 · 8 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@iamstolis
Copy link

  • Version: 6.2.2
  • Platform: Linux/Ubuntu
  • Subsystem: process.env

Consider the following test-case:

'use strict';
console.log(delete process.env.NON_EXISTING_VARIABLE);

It is weird to see delete operator to return false it strict-mode. It should return true or throw an error there. While this seems to be also a bug in V8 (it should throw an error when the deleter callback returns false in strict-mode) the implementation of EnvDeleter (in src/node.cc) should be fixed as well. It should return true for any environment variable (even the non-existing ones) to match the behaviour of delete operator that is supposed to return true unless non-configurable property is being deleted.

@bnoordhuis
Copy link
Member

I'm inclined to say it's a V8 bug because node.js follows the contract for delete interceptors to the letter. From the doc comment for GenericNamedPropertyDeleterCallback:

The return value is true if the property could be deleted and false otherwise.

@bnoordhuis bnoordhuis added the v8 engine Issues and PRs related to the V8 dependency. label Aug 3, 2016
@iamstolis
Copy link
Author

Yes, the callback should return true 'if the property could be deleted'. The question is: what does it mean that it could be deleted? In JavaScript world this means that the property should not be non-configurable (and it may not even exist). Compare it with

'use strict';
var o = {};
console.log(delete o.nonExistentProperty);

that returns true. That's why I believe EnvDeleter should return true for any environment variable as well.

@bnoordhuis
Copy link
Member

Can you open a V8 bug? I'd like to hear what they have to say.

@iamstolis
Copy link
Author

@fhinkel
Copy link
Member

fhinkel commented Aug 3, 2016

We can use info.ShouldThrowOnError() in EnvDeleter to determine if we're in strict mode.

@bnoordhuis
Copy link
Member

@fhinkel What would you do in that case though? Users can't freeze/seal process.env or put non-configurable properties on it so I think the only thing to do for the deleter is to return true instead of false.

@fhinkel
Copy link
Member

fhinkel commented Aug 4, 2016

I misunderstood, I thought the issue is, that we should throw. If process.env never has non-configurable properties I guess we can always return true.

fhinkel added a commit to fhinkel/node that referenced this issue Aug 4, 2016
According to TC39 specification, the delete
operator returns false or throws
in strict mode, if the property is
non-configurable. It returns true in all other cases.

Process.env can never have non-configurable
properties, thus EnvDelete must always return true. This
is independent of strict mode.

Fixes nodejs#7960
@jasnell jasnell closed this as completed in ff7a841 Aug 5, 2016
@iamstolis
Copy link
Author

Thank you for a quick fix!

cjihrig pushed a commit that referenced this issue Aug 10, 2016
According to TC39 specification, the delete
operator returns false or throws
in strict mode, if the property is
non-configurable. It returns true in all other cases.

Process.env can never have non-configurable
properties, thus EnvDelete must always return true. This
is independent of strict mode.

Fixes: #7960
PR-URL: #7975
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants