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

Decide on undefined vs null #70020

Closed
bpasero opened this issue Mar 8, 2019 · 13 comments
Closed

Decide on undefined vs null #70020

bpasero opened this issue Mar 8, 2019 · 13 comments
Assignees
Labels
debt Code quality issues under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Mar 8, 2019

Once we are done with the strict null work I would like to consider using just undefined and not undefined + null in our code.

TypeScript for example explicitly states in their coding guidelines, do not use null: https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines#null-and-undefined

I personally feel like null should not be used but just undefined and we should adopt our code towards undefined.

@Microsoft/code thoughts?

@bpasero bpasero added the under-discussion Issue is under discussion for relevance, priority, approach label Mar 8, 2019
@bpasero bpasero added this to the On Deck milestone Mar 8, 2019
@bpasero bpasero added the debt Code quality issues label Mar 8, 2019
@bpasero
Copy link
Member Author

bpasero commented Mar 11, 2019

@Microsoft/vscode for input

@jrieken
Copy link
Member

jrieken commented Mar 11, 2019

Yes, 👍 for undefined because that's what you get when doing array[offBoundIndex] or map.get(badKey). There are a few exceptions like regexp.exec but I do think that undefined is the default JavaScript-way

@joaomoreno
Copy link
Member

I'm not so sure this is feasible.

My feeling is that undefined is the most used, since it is also the default variable value: let foo;, or what you get when you delete map[key]. So, we're definitely going to have to deal with it eventually.

But I'm pretty sure a lot of other library calls can null, not just the RegExp example. document.querySelection('does not exist') returns null, for example.

Here's how it works in my mind:

  • I use undefined to represent the absence of a value;
  • I use null to represent nothingness.

For example:

interface Node<T> {
	value: T | null;
	next?: Node<T>;
}

A node can hold a value T or it can hold nothing. It shouldn't hold an undefined value, since we want nodes to always hold something... even if that something is nothing. Yet, as part of a linked list, a node may or may not have a next node defined.

🤷‍♂️

@jrieken
Copy link
Member

jrieken commented Mar 11, 2019

That node sample falls apart for Node<null | undefined> but yeah I get your point. Generally, I try to be JavaScript'ish (and DOM is a lib that browsers provide, unlike Math or Map which JS itself defines) but I also understand that it will be hard to reach total happiness on the topic. What we should avoid is IMO is the following

function compare(a: SomeInterface | null, b: SomeInterface | null) { ... }

compare(someValue, map.has(someKey) ? map.get(someKey) : null)

In that sample the compare function is too strict in what it accepts, making it cumbersome to use. In general the rule "be relaxed in what you accept, be strict in what you return" should apply.

@bpasero
Copy link
Member Author

bpasero commented Mar 11, 2019

I am not sure we can fully eliminate null in our system, I get the idea for some use cases. But I think undefined should be the general thing and null the exception if there is good reasons for it. At least in my code I feel that I sometimes return null even though it could as well be undefined and that now triggers something | null annotations through the strict null work with no real reasoning behind it.

One thing I remember from ES5 and TS default values (function foo(something = true)) for parameters is that if you pass in undefined, the default parameter would be used, but if you pass in null it would not. Not sure how that changed with ES6, but this was often a source of errors for me.

@Tyriar
Copy link
Member

Tyriar commented Mar 11, 2019

I think "prefer undefined over null" should be as strict as we should get here.

@mjbvz
Copy link
Collaborator

mjbvz commented Mar 11, 2019

If you need to convert null to undefined or convert undefined to null in order to make strict null checking happy, please use the withNullAsUndefined and withUndefinedAsNull helper functions I added. These functions makes sure the conversion is consistent and also tags places that we should investigate de-nulling

@dbaeumer
Copy link
Member

Regarding

interface Node<T> {
	value: T | null;
	next?: Node<T>;
}

I still do this with undefined in the following way:

interface Node<T> {
	value: T | undefined;
	next?: Node<T>;
}

since it is different from

interface Node<T> {
	value?: T;
	next?: Node<T>;
}

in terms of object creation. The first one forces

{
   value: undefined
}

whereas the second allows {} to be a valid value for Node which cause a lot of problems.

@bpasero
Copy link
Member Author

bpasero commented Mar 15, 2019

Relevant issue in the TS repository where they also explain why they were able to avoid | null in TS. Basically because they do not rely on DOM APIs: microsoft/TypeScript#9653 (comment)

@alexdima
Copy link
Member

Before making a decision to go with undefined, I would like to ask that someone reach out to the v8 mailing list or create a definitive benchmark that shows that using undefined in object fields has the same performance impact as using null, specifically around hidden classes and deoptimizations.

I have spent a lot of time in the past in IRHydra (which is now unfortunately defunkt) and have painstakingly went through editor hot paths and deoptimization errors and made sure to not mutate the v8 hidden classes that are "hot". That often meant ensuring each and every field is initialized in the ctor, with null where pointers would later be used.

I would be terribly disappointed if all that work goes to waste because of a subjective preference towards undefined.

In other words, before we even consider expressing personal preference on the topic, we should make sure the runtime works the same way for both undefined and null.

@dbaeumer
Copy link
Member

dbaeumer commented Mar 28, 2019

From https://www.heise.de/developer/artikel/JavaScript-Engines-Performance-Steigerung-der-Browser-4264792.html

fast.js

(() => {
  const han = {firstname: "Han", lastname: "Solo"};
  const luke = {firstname: "Luke", lastname: "Skywalker"};
  const leia = {firstname: "Leia", lastname: "Organa"};
  const obi = {firstname: "Obi-Wan", lastname: "Kenobi"};
  const yoda = {firstname: "", lastname: "Yoda"};

  const people = [
    han, luke, leia, obi,
    yoda, luke, leia, obi
  ];

  const getName = (person) => person.lastname;

  console.time("engine");
  for(var i = 0; i < 1000 * 1000 * 1000; i++) {
    getName(people[i & 7]);
  }
  console.timeEnd("engine");
})();

slow.js

(() => {
  const han = {firstname: "Han", lastname: "Solo", spacecraft: "Falcon"};
  const luke = {firstname: "Luke", lastname: "Skywalker", job: "Jedi"};
  const leia = {firstname: "Leia", lastname: "Organa", gender: "female"};
  const obi = {firstname: "Obi-Wan", lastname: "Kenobi", retired: true};
  const yoda = {lastname: "Yoda"};

  const people = [
    han, luke, leia, obi,
    yoda, luke, leia, obi
  ];

  const getName = (person) => person.lastname;

  console.time("engine");
  for(var i = 0; i < 1000 * 1000 * 1000; i++) {
    getName(people[i & 7]);
  }
  console.timeEnd("engine");
})();

fast-undefined.js

(() => {
  const han = {firstname: "Han", lastname: "Solo", gender: undefined, job: undefined, retired: undefined, spacecraft: "Falcon"};
  const luke = {firstname: "Luke", lastname: "Skywalker", gender: undefined, job: "Jedi", retired: undefined, spacecraft: undefined};
  const leia = {firstname: "Leia", lastname: "Organa", gender: "female", job: undefined, retired: undefined, spacecraft: undefined};
  const obi = {firstname: "Obi-Wan", lastname: "Kenobi", gender: undefined, job: undefined, retired: true, spacecraft: undefined};
  const yoda = {firstname: undefined, lastname: "Yoda", gender: undefined, jon: undefined, retired: undefined, spacecraft: undefined};

  const people = [
    han, luke, leia, obi,
    yoda, luke, leia, obi
  ];

  const getName = (person) => person.lastname;

  console.time("engine");
  for(var i = 0; i < 1000 * 1000 * 1000; i++) {
    getName(people[i & 7]);
  }
  console.timeEnd("engine");
})();

fast-null.js

(() => {
  const han = {firstname: "Han", lastname: "Solo", gender: null, job: null, retired: null, spacecraft: "Falcon"};
  const luke = {firstname: "Luke", lastname: "Skywalker", gender: null, job: "Jedi", retired: null, spacecraft: null};
  const leia = {firstname: "Leia", lastname: "Organa", gender: "female", job: null, retired: null, spacecraft: null};
  const obi = {firstname: "Obi-Wan", lastname: "Kenobi", gender: null, job: null, retired: true, spacecraft: null};
  const yoda = {firstname: null, lastname: "Yoda", gender: null, jon: null, retired: null, spacecraft: null};

  const people = [
    han, luke, leia, obi,
    yoda, luke, leia, obi
  ];

  const getName = (person) => person.lastname;

  console.time("engine");
  for(var i = 0; i < 1000 * 1000 * 1000; i++) {
    getName(people[i & 7]);
  }
  console.timeEnd("engine");
})();

On my machine fast-undefined and fast-null show the same performance. Please note that it is the best to initialize properties with default values matching the type (e.g. '' for string, false for boolean). V8 still deoptimizes when the type of a property changes.

@dbaeumer
Copy link
Member

dbaeumer commented Mar 28, 2019

Interestingly the fastest is to have classes with the right initializers:

class.js

(() => {
  class Person {
    constructor({
      firstname = undefined,
      lastname = undefined,
      spaceship = undefined,
      job = undefined,
      gender = undefined,
      retired = undefined
    } = {}) {
      Object.assign(this, {
        firstname,
        lastname,
        spaceship,
        job,
        gender,
        retired
      });
    }
  }
  const han = new Person({
    firstname: "Han",
    lastname: "Solo",
    spaceship: "Falcon"
  });
  const luke = new Person({
    firstname: "Luke",
    lastname: "Skywalker",
    job: "Jedi"
  });
  const leia = new Person({
    firstname: "Leia",
    lastname: "Organa",
    gender: "female"
  });
  const obi = new Person({
    firstname: "Obi-Wan",
    lastname: "Kenobi",
    retired: true
  });
  const yoda = new Person({ lastname: "Yoda" });
  const people = [han, luke, leia, obi, yoda, luke, leia, obi];
  const getName = person => person.lastname;
  console.time("engine");
  for (var i = 0; i < 1000 * 1000 * 1000; i++) {
    getName(people[i & 7]);
  }
  console.timeEnd("engine");
})();

@bpasero bpasero removed this from the On Deck milestone Jun 4, 2019
renesansz added a commit to renesansz/vscode that referenced this issue Aug 19, 2019
* Use bat on Windows

* debt: use action bar from the panel view

* Fix microsoft/vscode-remote-release/issues/1066

* fix label

* fix: event's jsdoc typo

* Move cleanRemoteAuthority function to common location

* microsoft#78168 strict init

* microsoft#78168 strict init

* microsoft#78168 strict init

* fixes microsoft#77797

* Split formatted text renderer into own file

* Split MarkdownRenderOptions from FormattedTextRenderOptions

* Rename htmlContentRenderer -> markdownRenderer

* web - do not loose state on unload

* open exter - move into opener service

* add extension kind web

* add action only in remote

* Improve microsoft#79047 fix after feedback

* use `vscode-remote`-endpoint when importing scripts

* Added preserveCase in search side bar

* test build schedule

* Revert "test build schedule"

This reverts commit 9d4ba37.

* better check for worker/web extension

* debt - IExtensionDescription#main should be relative like all other file references

* move windowsIPC => common

* very basic support to load multiple files

* microsoft#69108
Move IWorkspaceStatsService to common
Introduce a simple service for web

* Fix microsoft#69108

* tests -remove unused services

* Move getHashedRemotesFromUri to IWorkspaceStatsService

* add and use `getWorkerBootstrapUrl`, don't use default worker factory anymore

* - use strings for view zone ids
- make it unlikely that a new view instance would accept a view zone id from a previous view instance
- remove that the find widget removes a view zone id from another view
(fixes microsoft#71745)

* add `extensionHostWorker` entry point, fixes https://github.com/microsoft/vscode-internalbacklog/issues/738

* Update uglify-es (microsoft#79044)

* Revert "Work around minifier bug (microsoft#79044)"

This reverts commit 6371cad.

* expose product configuration in product service

* use product service

* Simplify terminal commands

* Improve typings, use async

* uses getSelections and simplifed the return

* Update distro

* migrate keys from legacy layout microsoft#79020

* fix tests

* Register driver

* Teardown on sigint

* Update markdown grammar

* Still show fix all actions for fix-all actions that can fix multiple errors with multple different diagnostics

* Base web user data dir off normal one

* Update distro

* Use new browser none arg

* Make sure we compare fully normalized error codes when checking for fix all actions

* Don't include closing ] in folded range

Fixes microsoft#79142

* Register Remote Explorer when there are contributions.

* Use undefined instead of null in IEditorOpeningEvent and IOpenEditorOverride

* Change openEditor to return undefined instead of null

For microsoft#70020

* Improve jsdoc section of walkthrough

Fixes microsoft#71023

As discussed in microsoft#75033

* Add installer assets for OSS (microsoft#79045)

* Revert "Update uglify-es (microsoft#79044)"

This reverts commit e677c03.

* Work around minifier bug (microsoft#79044)

* debt - avoid some StrictNullOverride

* debt - move issue service out of common since it will not be supported in the web for now

* debt - make diagnostics service only accessible from node

* update distro

* tests - add more debug for randomly failing getUntitledWorkspacesSync

* fix mispell

* 💄

* update distro

* Simplify tasks command

* docs: fix type (microsoft#79129)

* web - add todo for potentially cyclic dependency

* Multi-select in custom tree view (microsoft#78625)

Part of microsoft#76941

The first argument is now the element that the command is executed on. The second argurment is an array of the other selected items

* update distro

* web - enable feedback contribution

* debt - more tests diag

* do not look for executables in web

* Move extension tips service to web and enable extension recommendations

* use process.setImmediate

* minor polish

* fixes microsoft#79168

* fix exports trap

* debug: prevent expression.value being undefined

fixes microsoft#79169

* update distro

* web - implement credentials provider and add API

* web - workaround clipboard issue with selection type

* better exports trapping

* fix process layer-breaker

* debt - avoid process dependency in common

* rawDebugSession: do not use process

microsoft#79210

* remove proposed API `vscode.commands.onDidExecuteCommand`

* Fix microsoft#79206

* callStack view:  do not show thread when there is only one to be compact

fixes microsoft#79121

* fix compile error

* inline product configuration in produt service

* update distro

* Fix strict error

* fix tests

* Do not expand session tree node when selecting it

fixes microsoft#79184

* Fix issue with CustomExecutions not working through tasks.executeTask (microsoft#79132)

* Do not clear out the map, and track the provided execution during execute task

* Clear provided custom executions map on execution complete

* Save last custom execution

* use safeprocess env

* Remove click code from puppeteer driver, move interfaces to common

* Reduce diff

* fix typos

* web - reuse require interceptor logic

* remove FakeCommonJSSelf

* Fix dispatch keybinding when called multiple times

* Fix layer breakage

Part of microsoft#79210

* Fixes microsoft#78975: Look for autoclosing pairs in edits coming in from suggestions

* Remove smoke tests from CI for now

* Reduce diff

* Add setting to toggle new octicon style

* Update distro

* Move puppeteer to dev deps

* Run smoke tests for darwin/linux in CI

* Make display name consistent

* Fix indent

* Improve token regex

* Add connectionToken

* update distro

* Update search stop icon

* Update checkmark so they look more like a ✓

* introduce RemoteAuthorities

* remote explorer and contribution under proposed api

* Disable smoke tests on Linux

Puppeteer needs special user setup in order to launch

* Strict init

microsoft#78168

* Strict init and mark events readonly

microsoft#78168

* Remove extra null checks in coalesce

The type system should catch these now

* Add telemetry+warning for webviews that don't have a content security policy

Fixes microsoft#79248

* Fixing comment

* Update distro

* Update distro

* Don't dispose of added object in already disposed of case

Fixes microsoft#77192

See microsoft#77192 for discussion

* Mark readonly

* Use const enums

* Marking fields readonly

* Remove webview svg whitelist

This is no longer required

* Removing test for disposable store

We have disabled this behavior

* build - disable smoketest

* Enable new Octicons style by default

* fix web platform check

* better worker error logging

* make sure to prepend vs/nls

* 💄

* fix: keep the two "Copy Path" behavior consistent

When using remote, this "Copy Path" function of SearchAction will keep the remote prefix while the FileCommand will not.

Change-Id: Ide00d2da5a695d0adbe87622643c7a600dd46432

* Load Octicons through ts instead of css import

* update distro

* Fixes microsoft#79166

* web - synchronise global state changes

* explorer input black magic

fixes microsoft#78153

* fix microsoft#72417

* Fix smoke tests

* Revert "build - disable smoketest"

This reverts commit c23cacd.

* Scope new variable to a string

* Fallback to default when cwd var cannot be resolved

Fixes microsoft#79281

* fix error when dismissing snippets picker

* add fetch file system provider

* add IStaticExtensionsService, add `staticExtensions` to embedder API

* make staticExtensions optional

* Indicate web in smoke test step

* Update octicon css logic

* Fix Octicon icons on Linux

* debug: introduce data breakpoints

* Introduce and adopt asCSSUrl

* Fixes microsoft/vscode-remote-release#687:
- remove ipc message that passes over the resolved authority
- don't go through the protocol handler when hitting 127.0.0.1

* Introduce machine overridable setting

* Seti uses TS-icon for jsonc-language. Fixes microsoft#78261

* Pick up TS rc

* xterm-addon-search@0.2.0-beta5

- Better align search with how monaco does it

Fixes microsoft#78281

* Format file

* Update distro

* Remove not null suppression

* Use dispoablestore

* Use closure instead of parameters

* Use type for IExtensionPointHandler and mark array readonly

* Marking arrays readonly in ExtensionPointUserDelta

* web - storage/lifecycle 💄

* Fix microsoft#79326, cleanup rendering Octicons bugs on Windows/Linux

* fix Markdown Preview scroll remains same after clicking on some other link

* Fix trivial zsh completion typo

* Improvements

* Drop Configure Trusted Domains command for now

* 💄

* Drop details & on -> from

* Address feedbacks

* Fix services

* Storage instead of setting. Add command for configuring

* Don't add star to quickpick

* Update UX

* Build errors and test failures

* 💄

* debt - fix some layer breakers

* Added buildReplaceStringWithCasePreserved generic function to be  used in both search side and the find side

* 💄 imports

* refactor: GlobalStorageDatabaseChannel should dependents in IStorageMainService (microsoft#79387)

* 💄

* build - have different worker extension host entrypoint, copy worker bootstrap file

* fix microsoft#76573

* fix path

* update distro

* Localization test fails on remote smoketest. Fixes microsoft#78412

* Fix microsoft/vscode-remote-release/issues/1027

* fix scope when falling back to original load function

* Wording update

* Fix: Markdown Preview scroll remains same after clicking on some other link microsoft#78465

Improves the behavior on how markdown preview behaves when clicking a link

* microsoft#79429

* TSLint: show a warning when accessing node.js globals in common|browser (microsoft#79222)

* trivial first cut

* document where globals are from

* improve rule detection

* fix "gulp tslint" task

* share rules

* enable more rules

* also add a rule for DOM

* Update trustedDomain default config

* tslint - disable some rules with comments (microsoft#79454)

* Drop unneeded action registration

* update distro

* Support using csharp in markdown preview to identify c# code blocks

* sync diagnostics, fixes microsoft#47292

* fixes microsoft#79280

* tests - more diagnostics for getUntitledWorkspaceSync() random failures

* debt - reduce dependencies diff to E6 branch

* update distro

* debt - menubar service should not live in common (microsoft#79181)

* distro

* Unify trusted domain language and use quick pick item id

* Update browser telemetry common properties

* microsoft#79454 Fix warnings

* Fix microsoft#79429

* Fix hygiene check
@mjbvz mjbvz added this to the October 2019 milestone Sep 23, 2019
@bpasero
Copy link
Member Author

bpasero commented Oct 8, 2019

Everyone decides as they like. I personally will go with "undefined".

@bpasero bpasero closed this as completed Oct 8, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

7 participants