Skip to content

EnvDeleter should return true #7960

Closed
Closed
EnvDeleter should return true#7960
Labels
v8 engineIssues and PRs related to the V8 dependency.

Description

@iamstolis
iamstolis
opened on Aug 3, 2016
  • 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.

Activity

  1. bnoordhuis commented on Aug 3, 2016

    @bnoordhuis
    bnoordhuis
    on Aug 3, 2016
    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.

  2. bnoordhuis
    added
    v8 engineIssues and PRs related to the V8 dependency.
    on Aug 3, 2016

  3. iamstolis commented on Aug 3, 2016

    @iamstolis
    iamstolis
    on Aug 3, 2016
    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.

  4. bnoordhuis commented on Aug 3, 2016

    @bnoordhuis
    bnoordhuis
    on Aug 3, 2016
    Member

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

  5. iamstolis commented on Aug 3, 2016

    @iamstolis
    iamstolis
    on Aug 3, 2016
    Author

    OK, I have filled https://bugs.chromium.org/p/v8/issues/detail?id=5260

  6. fhinkel commented on Aug 3, 2016

    @fhinkel
    fhinkel
    on Aug 3, 2016
    Contributor

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

  7. bnoordhuis commented on Aug 3, 2016

    @bnoordhuis
    bnoordhuis
    on Aug 3, 2016
    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.

  8. fhinkel commented on Aug 4, 2016

    @fhinkel
    fhinkel
    on Aug 4, 2016
    Contributor

    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.

  9. jasnell
    closed this as completedin ff7a841on Aug 5, 2016

  10. iamstolis commented on Aug 5, 2016

    @iamstolis
    iamstolis
    on Aug 5, 2016
    Author

    Thank you for a quick fix!

  11. cjihrig
    added a commit that references this issue on Aug 10, 2016

    src: make EnvDelete behave like the delete operator

    317ae96
  12. sjarmak
    added a commit that references this issue on Jul 27, 2026

    src: make EnvDelete behave like the delete operator

    ff7a841
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    v8 engineIssues and PRs related to the V8 dependency.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions