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

Nominal unique type brands #33038

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

@weswigham
Copy link
Member

@weswigham weswigham commented Aug 23, 2019

Fixes #202
Fixes #4895

We've talked about this on and off for the last three years, and it was a major reason we chose to use unique symbol for the individual-symbol-type, since we wanted to reuse the operator for a nominal tag later. What this PR allows:

type NormalizedPath = unique string;
type AbsolutePath = unique string;
type NormalizedAbsolutePath = NormalizedPath & AbsolutePath;

declare function isNormalizedPath(x: string): x is NormalizedPath;
declare function isAbsolutePath(x: string): x is AbsolutePath;
declare function consumeNormalizedAbsolutePath(x: NormalizedAbsolutePath): void;


const p = "/a/b/c";
if (isNormalizedPath(p)) {
    if (isAbsolutePath(p)) {
        consumeNormalizedAbsolutePath(p);
    }
}

unique T (where T is any type) is allowed in any position a type is allowed, and nominally tags T with a marker that makes only T's that have come from that location be assignable to the resulting type.

This is done by adding a new NominalBrand type flag, which is a type with no structure which is unique to each symbol it is manufactured from. This is then mixed into the argument type to unique type via intersection, which is what produces all useful relationships. (The brand can have an alias if it is directly constructed via type MyBrand = unique unknown)

This does so much with so little - this reduces the jankiness written into types to enable nominalness with unique symbols or enums, while adding zero new assignability rules.

So, why bring this up now? I was thinking about how "brands" work today, with something like type MyBrand<T> = T & {[myuniquesym]: void} where T could then become a literal type like "a". We've wanted, for awhile, to be able to more eagerly reduce an intersection of an object literal and a primitive to never (to make subtype reduction and intersection reduction produce less jank and recognize more types as mutually exclusive), but these "brand" patterns keep stopping us. (Heck, we use em internally.) Well, if we ever want to change object types to actually mean object, then we're going to need to provide an alternative for the brand pattern, and ideally that alternative needs to be available for awhile. So looking on the horizon to breaks we could take into 4.0 in 9 months, this simplification of branding would be up there, provided we've had the migration path available for awhile. So I'm trying to get the conversation started on this before we're too close to that deadline to plan something like that. Plus #202 is up there on our list of all-time most requested issues, and while we've always been open to it, we've never put forward a proposal of our own - well, here one is.

On unique symbol

unique symbol's current behavior takes priority for syntactically exactly unique symbol, however if a nominal subclass of symbols is actually desired, one can write unique (symbol) to get a nominally branded symbol type instead (or symbol & unique unknown - it's exactly the same thing). The way a unique symbol behaves is like a symbol that is locked to a specific declaration, and has special abilities when it comes to narrowing and control flow because of that. Nominally branded types are more flexible in their usage but do not have the strong control-flow linkage with a host statement that unique symbols do (in fact, they don't necessarily assume a value exists at all), so there is very much reason for them to coexist. They're similar enough, that I'm pretty comfortable sharing syntax between the two.

Alternative considerations

While I've used the unique type operator here, like we've oft spoken of, on implementation, it's become plain to me that I don't need to specify an argument to unique. We could just expose unique as a unique type factory on it's own, and dispense with the indirection. The "uniqueness" we apply internally isn't actually tied to the input type argument through anything more than an intersection type, so simply shortening unique unknown to unique and reserving the argument form for just unique symbols may be preferable. All the same patterns would be possible, one would just need to write string & unique instead of unique string, thus dispensing with the sugar. It depends on the perceived complexity, I think. However, despite being exactly the same, string & unique is somehow uglier and harder to look at than unique string, which is why I've kept it around for now. It's probably worth discussing, though.

What this draft would still need to be completed:

  • New error messages for any errors involving brands (One or more unique brands is missing from type A with related information pointing at the brand location, rather than the current error involving unique unknown)
  • More tests exercising indexed access and exploring how indexed accesses on branded types are constructed (specifically, what should be done if you (attempt to) index nothing but a union of unique unknown brands)
  • Tests for unique (symbol) declaration emit (to ensure it's not rewritten as unique symbol)
  • Discussion on if keyof <unique brand type> should be never (as it is now), since the brand is top-ish (and contains no structure information itself), or if it should be preserved as an abstract keyof <unique brand> type, so that brand can apply keys-of-branded-types constraints in constraint positions
  • 🚲 🏠
@fatcerberus
Copy link

@fatcerberus fatcerberus commented Aug 23, 2019

While I've used the unique type operator here, like we've oft spoken of, on implementation, it's become plain to me that I don't need to specify an argument to unique.

I don't understand this part. If these are meant to replace branded types, then:

type UString = unique string;
type BString = string & { __brand: true };

declare let ustr: UString;
declare let bstr: BString;
let str1: string = bstr;  // ok because branded string is a subtype of string (this is generally desirable).
let str2: string = ustr;  // could be ok because unique string is also a string.
let num: number = ustr;  // never ok, should be error because unique string is NOT a number!

But if we just had unique without the argument, there would be no way to make this distinction. unique would just end up being a nominal unknown (effectively an opaque Skolem constant) which doesn't sound that useful to me?

Loading

@weswigham
Copy link
Member Author

@weswigham weswigham commented Aug 23, 2019

But if we just had unique without the argument, there would be no way to make this distinction.

It's useful because you can then intersect it with something. Which is all unique string is doing under the hood right now - making that intersection for you.

Loading

@fatcerberus
Copy link

@fatcerberus fatcerberus commented Aug 23, 2019

Yeah, I need to read more closely. I just noticed the string & unique bit before you posted. But I don't like this because unique by itself isn't a type. It would be a special marker that doesn't work by itself as a type (or at least, doesn't make sense as one - what would it mean to take an argument of form arg: unique, e.g.), which I find very weird to then use in a position that nominally (no pun intended!) accepts only type operands. I guess there's precedent with ThisType<T>, but... I don't know, it rubs me the wrong way.

It might be represented as an intersection under the hood, but that strikes me as unnecessarily exposing implementation details.

Loading

@weswigham
Copy link
Member Author

@weswigham weswigham commented Aug 23, 2019

unique is essentially ust shorthand for unique unknown under the current model, and it does function fully as a standalone type.

Loading

@cevek
Copy link

@cevek cevek commented Aug 23, 2019

Is this proposal allows to assign literals to variable/param which has branded type?

type UserId = unique string
function foo(param: UserId) {}

foo("foo") // ok
var x: UserId = "foo"; // ok

let s = "str";
var y: UserId = s; // not ok
foo(s) // not ok

Loading

@goodmind
Copy link

@goodmind goodmind commented Aug 23, 2019

How would you make nominal classes with this?

Loading

@weswigham
Copy link
Member Author

@weswigham weswigham commented Aug 23, 2019

Not easily. You'd need to mix a distinct nominal brand into every class declaration, like via a property you don't use of type unique unknown or similar, and then ensure that your inheritance trees always override such a field (so a parent and child don't appear nominally identical).

I'd avoid it, if possible, tbh. Nominal classes sound like a pain :P

Loading

@weswigham
Copy link
Member Author

@weswigham weswigham commented Aug 23, 2019

Is this proposal allows to assign literals to variable/param which has branded type?

Branded types can only be "made" either via cast or typeguard, as I said in the OP, so no. This is because the typesystem doesn't know what invariants a given brand is meant to maintain, and can't implicitly know if some literal satisfies them.

Loading

@jack-williams
Copy link
Collaborator

@jack-williams jack-williams commented Aug 23, 2019

Would something like the following ever be meaningful? Probably not right..

interface Parent extends unique unknown { }
interface ChildA extends (Parent & unique unknown) { }
interface ChildB extends (Parent & unique unknown) { }

Loading

@AnyhowStep
Copy link
Contributor

@AnyhowStep AnyhowStep commented Aug 23, 2019

This is probably obvious to everyone but unique T shouldn't replace branding.
There are scenarios where branding is the only viable solution (at the moment).

For example,

//Can be replaced with `unique number`
type LessThan256 = number & { __rangeLt : 256 }
//Cannot be replaced with `unique number`
type LessThan<N extends number> = number & { __rangeLt : N }

More complicated example here,
#15480 (comment)


There are a few reasons why I'm generally against the idea of unique T.
(And nominal typing, and instanceof, and using symbols)

Cross-library interop

Library A may have type Radian = unique number.
Library B may have type Radian = unique number.

Both types will be considered different, even though they have the same name and declaration.

If libraries start using this unique T all over the place, you'll start needing unsafe casts (as libA.Radian) more often. This means you can accidentally write myDegree as libA.Radian and cast incorrectly. Whoops!

So, one starts thinking that a no-op casting function would be safer,

function libARadianToLibBRadian (rad : libA.Radian) : libB.Radian {
  return rad as libB.Radian;
}

This is safer because you won't accidentally convert myDegree:libA.Degree to libB.Radian

But if you have N libraries with their own Radian type, you may end up needing up to N^2 functions to convert between each of the Radian types from each library.


Cross-version interop

It's happened to me a bunch where I've had the same package, but at different versions, within a single project.

So,

v1.0.0's type Radian = unique number would be considered different from,
v1.1.0's type Radian = unique number

Now you need a casting function... Even though it's the same package.


With brands, if two libraries use the same brands, even if they're different types, they'll still be assignable to each other. (As long as they don't use unique symbol)

Library A may have type Rad = number & { __isRadian : void }.
Library B may have type Radian = number & { __isRadian : void }.

Even though they're different types, they're assignable to each other. No casting needed.
v1.0.0 and v1.1.0 of type Rad and type Radian will work with each other fine.


As an aside, I vaguely remember something from many, many years ago. I can't find it through Google anymore, though.

There was discussion about adding syntax to C++ to make typedef create a new type (rather than just functioning as a type alias),

typedef double Radian;

And this was rejected outright because of the issues I listed above.

Two libraries with their own typedef double Radian; type would be incompatible, the N^2 problem, etc.

Loading

@fatcerberus
Copy link

@fatcerberus fatcerberus commented Aug 23, 2019

Re: nominal classes - classes are already nominal if they contain any private members. Just throwing that out there. 😃

Loading

@be5invis
Copy link

@be5invis be5invis commented Aug 24, 2019

So we can finally have things like this? @weswigham

// Low-end refinement type :)
type NonEmptyArray<A> = unique ReadonlyArray<A>
function isNonEmpty(a: ReadonlyArray<A>): a is NonEmptyArray<A> {
    return a.length > 0
}

// INTEGERS (sort of)
type integer = unique number
function isInteger(a: number) { return a === a | 0 }

Loading

@AnyhowStep
Copy link
Contributor

@AnyhowStep AnyhowStep commented Aug 24, 2019

@fatcerberus It's also why I avoid classes entirely and avoid private members if I do have them =P

Loading

@AnyhowStep
Copy link
Contributor

@AnyhowStep AnyhowStep commented Aug 25, 2019

Hmm...

const normalizedPathBrand = Symbol();
const absolutePathBrand = Symbol();

type NormalizedPath = string & typeof normalizedPathBrand;
type AbsolutePath = string & typeof absolutePathBrand;
type NormalizedAbsolutePath = NormalizedPath & AbsolutePath;

declare function isNormalizedPath(x: string): x is NormalizedPath;
declare function isAbsolutePath(x: string): x is AbsolutePath;
declare function consumeNormalizedAbsolutePath(x: NormalizedAbsolutePath): void;


const p = "/a/b/c";
consumeNormalizedAbsolutePath(p); //Error
if (isNormalizedPath(p)) {
    consumeNormalizedAbsolutePath(p); //Error
    if (isAbsolutePath(p)) {
        consumeNormalizedAbsolutePath(p); //OK
    }
}

Playground


I guess the downside to this is that it's not actually a symbol.

But Symbol doesn't have very methods one would accidentally use.
image

Loading

@jack-williams
Copy link
Collaborator

@jack-williams jack-williams commented Aug 26, 2019

If cross version compatibility really is an issue then an alternate solution would be to make naming explicit:

Let the type unknown "name" denote the set of all values with label name. This would make the intersection type approach mandatory so a nominal path must now be written:

type NormalizedPathOne = string & unique "NormalizedPath";

where an unlabelled unique denotes a generative type as defined in the OP.

type NormalizedPathTwo = string & unique // some label that we don't care about that is auto-generated.

so while two declarations of NormalizedPathTwo produce distinct types, two declarations of NormalizedPathOne produce identical types.

FWIW I have no real preference---for me the big win of this feature is being able reduce empty intersections more aggressively.

Discussion on if keyof unique brand type ....

IMO, for all brand oblivious operations a unique type should be equivalent to unknown (which is what is proposed AFAIK).

Loading

@resynth1943
Copy link

@resynth1943 resynth1943 commented Aug 27, 2019

I've implemented Opaque types like so:

type Opaque<V> = V & { readonly __opq__: unique symbol };

type AccountNumber = Opaque<number>;
type AccountBalance = Opaque<number>;

function createAccountNumber (): AccountNumber {
    return 2 as AccountNumber;
}

function getMoneyForAccount (accountNumber: AccountNumber): AccountBalance {
    return 4 as AccountBalance;
}

getMoneyForAccount(100); // -> error

Loading

@AnyhowStep
Copy link
Contributor

@AnyhowStep AnyhowStep commented Aug 27, 2019

@resynth1943

Your version breaks given the following,

type Opaque<V> = V & { readonly __opq__: unique symbol };

type NormalizedPath = Opaque<string>;
type AbsolutePath = Opaque<string>;
type NormalizedAbsolutePath = NormalizedPath & AbsolutePath;

declare function isNormalizedPath(x: string): x is NormalizedPath;
declare function isAbsolutePath(x: string): x is AbsolutePath;
declare function consumeNormalizedAbsolutePath(x: NormalizedAbsolutePath): void;


const p = "/a/b/c";
consumeNormalizedAbsolutePath(p); //Error
if (isNormalizedPath(p)) {
    consumeNormalizedAbsolutePath(p); //Expected Error, Actual OK
    if (isAbsolutePath(p)) {
        consumeNormalizedAbsolutePath(p); //OK
    }
}

Playground

Contrast with,
#33038 (comment)

Loading

@resynth1943
Copy link

@resynth1943 resynth1943 commented Aug 31, 2019

@AnyhowStep I know, but that's how I'm currently creating opaque types. I hope this Pull Request will incorporate this into the language, and make it even better than my implementation.

Loading

@mohsen1
Copy link
Contributor

@mohsen1 mohsen1 commented Sep 3, 2019

Nominal types are pretty useful. The example I often use is the APIs that take latitude/longitude and bugs that are result of mixing up latitude with longitude which are both numbers. By making those unique types we can avoid that class of bugs.

However, unique types can cause so much pain when you have to keep importing those types to simply use an API. So I'm hoping that at least primitive types are assignable to unique primitives where I can still call my functions like this:

// lib.ts
export type Lat = unique number;
export type Lng = unique number;
export function distance(lat: Lat, lng: Lng): number;

// usage.ts
import { distance } from 'lib.ts';
distance(1234, 5678); // no need to asset types

As @AnyhowStep mentioned cross-lib and cross-version conflicting unique types can also be a source of pain. Can we limit uniqueness scope somehow? Would that be a viable solution?

Loading

@weswigham weswigham marked this pull request as ready for review Sep 6, 2019
@weswigham
Copy link
Member Author

@weswigham weswigham commented Sep 6, 2019

#33290 is now open as well, so we can have a real conversation on what the non-nominal explicit tag would look like, and if we'd prefer it.

Loading

@weswigham
Copy link
Member Author

@weswigham weswigham commented Sep 7, 2019

BTW, this is now a dueling features type situation (though I've authored both) - we won't accept both a #33290 style brand and this PR's style brand, only one of the two (and we're leaning towards #33290 on initial discussion). We'll get to debating it within the team hopefully during our next design meeting (next friday), but y'all should express ideas, preferences, and reasoning therefore within both PRs.

Loading

@weswigham
Copy link
Member Author

@weswigham weswigham commented Sep 9, 2019

Only repo contributors have permission to request that @typescript-bot pack this

Loading

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Sep 9, 2019

Heya @weswigham, I've started to run the tarball bundle task on this PR at 13968b0. You can monitor the build here. It should now contribute to this PR's status checks.

Loading

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Sep 9, 2019

Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/43239/artifacts?artifactName=tgz&fileId=CC1A42393FB48F1DFF21222516C1DB28571C1E5134B16E3B234461EABAECAE1D02&fileName=/typescript-3.7.0-insiders.20190909.tgz"
    }
}

and then running npm install.

Loading

@xiaoxiangmoe
Copy link

@xiaoxiangmoe xiaoxiangmoe commented Sep 9, 2019

type integer = unique number
function isInteger(a: number): a is integer { return a === (a | 0) }

function Interger(a:number) {
    if(!isInteger(a)) throw new Error("not an integer");
    return a;
}

Loading

mheiber added a commit to mheiber/TypeScript that referenced this issue Dec 23, 2019
Implements private instance fields on top of the class properties refactor.

This commit also includes incorporation of PR feedback on the
checker, parser, and transformer in order to make the rebase
manageable.

Co-Authored-By: Max Heiber <max.heiber@gmail.com>
Co-Authored-By: Ron Buckton <ron.buckton@microsoft.com>

Signed-off-by: Joey Watts <jwatts43@bloomberg.net>
Signed-off-by: Max Heiber <max.heiber@gmail.com>

Incorporate PR feedback

Fix checker crash with new block scoped bindings

Add classExpressionInLoop test

Update baselines for private name errors

Apply suggestions from code review

Fix privateNameFieldCallExpression test

Remove unnecessary comment

Fix PropertyAccessEntityNameExpression type

Remove PrivateName from PropertyNameLiteral

Add createPrivateName

Remove PrivateName type from textSourceNode

Add Debug asserts for invalid syntax kinds

Don't output private name syntax when undeclared

Make PrivateName extend Node

Update baselines for public API

Fix completions in language server

Fix fourslash test crash

intern private name descriptions

undo expensive node.name.parent assignment

Back the way things were on `master`: only
assign node.name.parent when we need to

Add tests for private names and JSDoc

Patch hoverOverPrivateName fourslash test

Fix goToDefinition for private-named fields

remove Debug.fail for private name outside class

Remove Debug.fail in binder.ts::`getDeclarationName`.
It turns out this code path *does* get hit (intentionally).

For best error messages, return `undefined` and rely
on the parser generating a good error message

"Private names are not allowed outside class bodies"

Add rbuckton test cases for private names

These test cases specifically exercise where
we needed to use name-mangling. They are
cases where private names have the same
description.

- private names no conflict when inheriting
- private names distinct even when
the two classes have the same name

check dup instance+static private identifiers

class A {
    #foo;
    static #foo;
}

not allowed because static and private names
share the same lexical scope
https://tc39.es/proposal-class-fields/#prod-ClassBody

refactor getPropertyForPrivateName, rel spans

refactor getPropertyForPrivateName so
it is easier to read (use findAncestor instead
of loop).

Fix bugs in getPropertyForPrivateName:
- make it work with deeply-nested classes with
and without shadowing
- make it catch shadowing when the conflict is
between static and instance
private name descriptions (these can actually
clash)

And add related spans to diagnostics
for getPropertyForPrivateName

catch conflicts between static and instance
private identifiers:
- cannot have an instance and static private identifier
  with the same spelling, as there is only one namespace
  for private names

rename 'PrivateName' to 'PrivateIdentifier'

to match the change of wording in the spec
prposal:

https://tc39.es/proposal-class-fields/#sec-syntax

The rename in the spec was to avoid confusion
between the AST Node PrivateIdentifier
and the internal spec type PrivateName.

So one uses the [[Description]] of a
PrivateIdentifier to look up the PrivateName
for a given scope.

This corresponds closely to our implementation
in the binder and checker:
- we mangle PrivateIdentifier.escapedText to
get a `key` which we use to get the symbol
for a property

f

getLiteralTypeFromProperty-check privateIdentifier

rename and simplify privateNameAndAny test case

make it clearer that all this test is showing is
that we allow accessing arbitrary private identifiers
on `any`.

Note: we could have something more sound here by
checking that there is a private-named field declaration
in a class scope above the property access.

Discussion:
https://github.com/microsoft/TypeScript/pull/30829/files#r302760015

Fix typo in checker

s/identifer/identifier

remove accidental change

patch fourslash test broken by isPrivateIdentifier

just needed to add a check to see if the symbol
.name is defined

extract reportUnmatchedProperty

per @nsandersn feedback

propertiesRelatedTo was getting to long

pull out the unmatchedProperty reporting into
a seprate function

fix typo in private names test

Fix type soundness with private names

Remove PrivateIdentifier from emit with Expr hint

Fixup helpers and set function name for privates

remove accidentally-committed baselines

These baselines somehow ended up in this pr,
though they have nothing to do with the changes

Revert "getLiteralTypeFromProperty-check privateIdentifier"

This reverts commit bd1155c.

reason: the check for isIdentifier in
getLiteralTypeFromProperty is superfluous because
we do this check in getLiteralTypeFromPropertyName

Update comment in private name uniqueness test 3

I think the comments were not accurate and that we
export the error on `this.#x = child.#x` because:
- private names are lexically scoped: the code in question is not in a
lexical scope under the definition of Child's #x.
- private names are private to their containing class: never inherited

This expected behavior matches that of Chrome Canary and
my understanding of the spec

test private names use before def, circular ref

test private names use before def, circular ref

update diagnosticMessages s/delete/'delete'

per @DanielRosenwasser and @sandersn guidance,
use this style in diagnostic messages:

"operand of a 'delete' operator" (single quotes)

rather than old style:

"operand of a delete operator" (single quotes)

This is for consistency, as we use the new
style in the privateIdentifiers error messages
and it is consistent with our messages about
other operators

incorporate private names early exit feedback

and code style change to use a ternary
instead of if so we can 'const'

require private-named fields to be declared in JS

All fields must be declared in TS files.

In JS files, we typically do not have this requirement.

So special-case private fields, which must always
be declared (even in JS files, per spec)

update debug failure for es2015 private identifier

Co-Authored-By: Ron Buckton <ron.buckton@microsoft.com>

fix checker handling of private name in subclasse

update checker and tests to account for the
following ts features:

- private names do not participate in
the prototype chain, but *are* assigned
in the parent class' constructor. So
parent can access its *own* private fields
on instances of the subclass

Add more tests for private-named fields in JS

add test to hit symbolToExpression w private names

symbolToExpression knows about private names
add a test to exercise this code path

ban private-named static methods (not supported yet)

ban private-named methods (not supported yet)

ban private-named accessors (not supported yet)

fix privateIdentifier fourslash test

change assertion throw to return

Co-Authored-By: Ron Buckton <ron.buckton@microsoft.com>

Update comment in checker.ts re reserved members

Remove case for privateIdentifier in EntityNameExpr

Remove case for privateIdentifier in
EntityNameExpr. That code path is never hit,
and privateIdnetifiers cannot be entity names.

remove unnecessary parentheses

Ban private identifier in enum

As the new test, 'privateNameEnumNoEmit',
shows, the checker now correctly makes
a diagnostic for private identifiers in enums.

However, when noEmit is false we
hit this assertion in the transformer.

This assertion will have to be removed
so that we have a diagnostic here instead
of an assertion error.

When the assertion is removed,
the 'privateNameEnum' baseline
will have to be updated

Fix destructuring assignment, use createCallBinding, remove unneeded helper

Add class private field helpers to external helpers

Remove private identifier enum assertion, use missing identifiers

Fix hash map inefficiency by only using get

Update baselines with empty identifier change

Add privateNameEnum test baselines

Fix private identifier destructuring (array assignment defaults, unique names)

Use createPrivateIdentifierAssignment in destructuring transform

Fix lint error

Separate destructuring target visitor from regular visitor

Fix destructuring assignment with this bindings

Fix destructuring assignment with this bindings

Fix syntax error with destructuring assignment output

Disallow private identifiers in property signatures

remove duplicate test baselines

Add tests for undeclarated private identifiers

remove unnecessary cast

Nicer error message for mis-placed hashbang

Workaround v8 bug with destructured assignments

Optimize private identifier stack lookup

Avoid the cost of performing an array lookup to look at the top of the
private identifier environment stack.

Change function name to be more specific

Changes "getOperatorForCompoundAssignment" to
"getNonAssignmentOperatorForCompoundAssignment" now that this
function is accessible to the entire compiler.

Improve test case for private name assignment

Adds a compound assignment test case for a class with private names
being declared on the left-hand-side of the assignment expression.

Remove extra non-private-field test

Remove isPrivateIdentifierAssignmentExpression helper

Don't transform private names in ESNext target

Preserve private fields initialized to functions

Move function expressions to outer scope for efficiency

Add WeakMap collision check

Modify potential WeakMap collision condition

Fix this property assignment binding with private names

Add correct error message for WeakMap collision

Add error for statements before super with private identifiers

Refactor getPropertyForPrivateIdentifier

Add test for private identifier fields initialized to class exprs

Fix shebang errors

Fix private errors on index signatures

Add codefix for missing private property

Update error messages for unsupported private features

Fix inheritance-related errors

Fixes inheritance-related errors with private identifiers by resolving
properties from base classes. Private identifiers do not show up as
properties on a union type, so those do not type-check.

Add test for interface extending class with private access

Remove debugging log

Remove name assignment from private named functions

Rename to getPrivateIdentifierPropertyOfType

Fix index signature test comment

Fix test target syntax error

Change error messages

patch private identifiers outside class bodies

Add test for private identifier with ooo super

Add test for a class with a private identifier
with a non-preambly (for example, not a comment)
statement before 'super':

should error, saying 'super' must come first

Fix nits

incorporate PR feedback

Incorporate checker feedback

- reorganize if statements in checkFunctionOrConstructorSymbol
- remove overload for getPrivateIdentifierPropertyOfType

reorganize if statements in checkFunctionOrConstructorSymbol

test for private names with JSX

use getPropertyOftype in getPropertyForPrivateIdentifier

getPrivateIdentifierPropertyOfType error on synthetic

make getPrivateIdentifierPropertyOfType  error
if given a node that is not from the parse tree

Simplify checkPrivateIdentifierPropertyAccess

use getPropertiesOfType instead of
rehashing that logic

test for private identifiers w decl merging

fix test target for privateNameDeclarationMerging

update baselines

Fix private identifier ++/-- numeric coercion

Remove 'downleveled' from super error

Fix bad comments in helper call emit

Error on private identifiers in JSX tag names

Add more private identifier tests

Add es2015 target for private name destructured binding test

Add privateNameConstructorSignature test

Add test for navigation bar w private identifier

Remove spurious line from test

// in js file
class A {
    exports.#foo = 3; // should error
}

The above line failed to produce an error
when run using the test harness.

When using tsc or the language server,
we got the expected error message.

Removing the troublesome line, as it
seems to say more about the test runner
than about the code it is testing.

Fix inefficient constructor generation

dts: don't emit type for private-identified field

Do not emit types for private-identified fields
when generating declaration files.

// example.ts
export class A {
    #foo: number;
}

// example.d.ts

export declare class A {
    #foo;
}

**This is not ideal!**

The following would be better:

// example.d.ts

export declare unique class A {
    #foo;
}

See discussion:

microsoft#33038 (comment)

notice when private-identified field unused

Notice when private-identified fields are unused,
and implement the same behavior as for unused
private-modified fields.

This is used in the language server to make things
grayed out.

This case generates an error when --noUnusedLocals
flag is passed to tsc:
    - "<name> is declared but never used"

accept baselines

Revert "dts: don't emit type for private-identified field"

This reverts commit e50305d.

Instead of just excluding the type from private identifier
emit, only emit a single private identifier
per class.

This accomplishes nominality while
hiding implementation detail that
is irrelevant to .d.ts consumers

only emit a single private identifier in dts

In dts emit, emit at most one private identifier,
and rename it to `#private`.

refactor getPrivateIdentifierPropertyOfType

- safer check for wehther is parse tree node
- return undefined when not found (instead of
a Debug.fail)

Incorporate PR feedback

Don't rely on parent pointers in transform

Passes context about whether the postfix unary expression value is
discarded down the tree into the visitPostfixUnaryExpression function.

Remove orphaned test baseline files

remove unreachable if

Check `any`-typed private identified fields

Update private identifier incompatible modifier checks

- disallow 'abstract' with private identifiers
- s/private-named-property/private identifier

Add additional call expression test cases

Fix disallow 'abstract' with private identifier

Static private identifiers not inherited

Including this in the PR for private
instance fields even though static
private identifiers are banned.

Reason: this change
improves quality of error messages,
see test case.

Thanks Ron!

Simplifiy private identifier 'any' type handling

Error on private identifier declarations for ES5/ES3

Bind `this` for private identifier property tagged template literals

Fix target for jsdocPrivateName1 test

Update getPrivateIdentifierPropertyOfType API

Make it easier to use by accepting a string
and location, rather than a PrivateIdentifier.

Thanks Ron!

remove orphaned tests

rename test

remove duplicate tests

Remove unrelated error from test

update diagnostic message 'private identifier'

The nodes for hash private fields are now
called 'private identifier'. Update one last
diagnostic message to use the new terminology.

refine solution for static private identifier fields

- use `continue` instead of `filter` for perf
- better naming
- make it clear the current solution will
need to be altered when we lift the ban on
static private identifier fields, including
a test case that should change in the future

Fix display of private identifiers in lang server

Fix bug where private identifiers in completion
tooltips in the playground were showing up
as the symbol table entries (with underscores and such).

microsoft#30829 (comment)
mheiber added a commit to mheiber/TypeScript that referenced this issue Dec 23, 2019
Implements private instance fields on top of the class properties refactor.

This commit also includes incorporation of PR feedback on the
checker, parser, and transformer in order to make the rebase
manageable.

Co-Authored-By: Max Heiber <max.heiber@gmail.com>
Co-Authored-By: Ron Buckton <ron.buckton@microsoft.com>

Signed-off-by: Joey Watts <jwatts43@bloomberg.net>
Signed-off-by: Max Heiber <max.heiber@gmail.com>

Incorporate PR feedback

Fix checker crash with new block scoped bindings

Add classExpressionInLoop test

Update baselines for private name errors

Apply suggestions from code review

Fix privateNameFieldCallExpression test

Remove unnecessary comment

Fix PropertyAccessEntityNameExpression type

Remove PrivateName from PropertyNameLiteral

Add createPrivateName

Remove PrivateName type from textSourceNode

Add Debug asserts for invalid syntax kinds

Don't output private name syntax when undeclared

Make PrivateName extend Node

Update baselines for public API

Fix completions in language server

Fix fourslash test crash

intern private name descriptions

undo expensive node.name.parent assignment

Back the way things were on `master`: only
assign node.name.parent when we need to

Add tests for private names and JSDoc

Patch hoverOverPrivateName fourslash test

Fix goToDefinition for private-named fields

remove Debug.fail for private name outside class

Remove Debug.fail in binder.ts::`getDeclarationName`.
It turns out this code path *does* get hit (intentionally).

For best error messages, return `undefined` and rely
on the parser generating a good error message

"Private names are not allowed outside class bodies"

Add rbuckton test cases for private names

These test cases specifically exercise where
we needed to use name-mangling. They are
cases where private names have the same
description.

- private names no conflict when inheriting
- private names distinct even when
the two classes have the same name

check dup instance+static private identifiers

class A {
    #foo;
    static #foo;
}

not allowed because static and private names
share the same lexical scope
https://tc39.es/proposal-class-fields/#prod-ClassBody

refactor getPropertyForPrivateName, rel spans

refactor getPropertyForPrivateName so
it is easier to read (use findAncestor instead
of loop).

Fix bugs in getPropertyForPrivateName:
- make it work with deeply-nested classes with
and without shadowing
- make it catch shadowing when the conflict is
between static and instance
private name descriptions (these can actually
clash)

And add related spans to diagnostics
for getPropertyForPrivateName

catch conflicts between static and instance
private identifiers:
- cannot have an instance and static private identifier
  with the same spelling, as there is only one namespace
  for private names

rename 'PrivateName' to 'PrivateIdentifier'

to match the change of wording in the spec
prposal:

https://tc39.es/proposal-class-fields/#sec-syntax

The rename in the spec was to avoid confusion
between the AST Node PrivateIdentifier
and the internal spec type PrivateName.

So one uses the [[Description]] of a
PrivateIdentifier to look up the PrivateName
for a given scope.

This corresponds closely to our implementation
in the binder and checker:
- we mangle PrivateIdentifier.escapedText to
get a `key` which we use to get the symbol
for a property

f

getLiteralTypeFromProperty-check privateIdentifier

rename and simplify privateNameAndAny test case

make it clearer that all this test is showing is
that we allow accessing arbitrary private identifiers
on `any`.

Note: we could have something more sound here by
checking that there is a private-named field declaration
in a class scope above the property access.

Discussion:
https://github.com/microsoft/TypeScript/pull/30829/files#r302760015

Fix typo in checker

s/identifer/identifier

remove accidental change

patch fourslash test broken by isPrivateIdentifier

just needed to add a check to see if the symbol
.name is defined

extract reportUnmatchedProperty

per @nsandersn feedback

propertiesRelatedTo was getting to long

pull out the unmatchedProperty reporting into
a seprate function

fix typo in private names test

Fix type soundness with private names

Remove PrivateIdentifier from emit with Expr hint

Fixup helpers and set function name for privates

remove accidentally-committed baselines

These baselines somehow ended up in this pr,
though they have nothing to do with the changes

Revert "getLiteralTypeFromProperty-check privateIdentifier"

This reverts commit bd1155c.

reason: the check for isIdentifier in
getLiteralTypeFromProperty is superfluous because
we do this check in getLiteralTypeFromPropertyName

Update comment in private name uniqueness test 3

I think the comments were not accurate and that we
export the error on `this.#x = child.#x` because:
- private names are lexically scoped: the code in question is not in a
lexical scope under the definition of Child's #x.
- private names are private to their containing class: never inherited

This expected behavior matches that of Chrome Canary and
my understanding of the spec

test private names use before def, circular ref

test private names use before def, circular ref

update diagnosticMessages s/delete/'delete'

per @DanielRosenwasser and @sandersn guidance,
use this style in diagnostic messages:

"operand of a 'delete' operator" (single quotes)

rather than old style:

"operand of a delete operator" (single quotes)

This is for consistency, as we use the new
style in the privateIdentifiers error messages
and it is consistent with our messages about
other operators

incorporate private names early exit feedback

and code style change to use a ternary
instead of if so we can 'const'

require private-named fields to be declared in JS

All fields must be declared in TS files.

In JS files, we typically do not have this requirement.

So special-case private fields, which must always
be declared (even in JS files, per spec)

update debug failure for es2015 private identifier

Co-Authored-By: Ron Buckton <ron.buckton@microsoft.com>

fix checker handling of private name in subclasse

update checker and tests to account for the
following ts features:

- private names do not participate in
the prototype chain, but *are* assigned
in the parent class' constructor. So
parent can access its *own* private fields
on instances of the subclass

Add more tests for private-named fields in JS

add test to hit symbolToExpression w private names

symbolToExpression knows about private names
add a test to exercise this code path

ban private-named static methods (not supported yet)

ban private-named methods (not supported yet)

ban private-named accessors (not supported yet)

fix privateIdentifier fourslash test

change assertion throw to return

Co-Authored-By: Ron Buckton <ron.buckton@microsoft.com>

Update comment in checker.ts re reserved members

Remove case for privateIdentifier in EntityNameExpr

Remove case for privateIdentifier in
EntityNameExpr. That code path is never hit,
and privateIdnetifiers cannot be entity names.

remove unnecessary parentheses

Ban private identifier in enum

As the new test, 'privateNameEnumNoEmit',
shows, the checker now correctly makes
a diagnostic for private identifiers in enums.

However, when noEmit is false we
hit this assertion in the transformer.

This assertion will have to be removed
so that we have a diagnostic here instead
of an assertion error.

When the assertion is removed,
the 'privateNameEnum' baseline
will have to be updated

Fix destructuring assignment, use createCallBinding, remove unneeded helper

Add class private field helpers to external helpers

Remove private identifier enum assertion, use missing identifiers

Fix hash map inefficiency by only using get

Update baselines with empty identifier change

Add privateNameEnum test baselines

Fix private identifier destructuring (array assignment defaults, unique names)

Use createPrivateIdentifierAssignment in destructuring transform

Fix lint error

Separate destructuring target visitor from regular visitor

Fix destructuring assignment with this bindings

Fix destructuring assignment with this bindings

Fix syntax error with destructuring assignment output

Disallow private identifiers in property signatures

remove duplicate test baselines

Add tests for undeclarated private identifiers

remove unnecessary cast

Nicer error message for mis-placed hashbang

Workaround v8 bug with destructured assignments

Optimize private identifier stack lookup

Avoid the cost of performing an array lookup to look at the top of the
private identifier environment stack.

Change function name to be more specific

Changes "getOperatorForCompoundAssignment" to
"getNonAssignmentOperatorForCompoundAssignment" now that this
function is accessible to the entire compiler.

Improve test case for private name assignment

Adds a compound assignment test case for a class with private names
being declared on the left-hand-side of the assignment expression.

Remove extra non-private-field test

Remove isPrivateIdentifierAssignmentExpression helper

Don't transform private names in ESNext target

Preserve private fields initialized to functions

Move function expressions to outer scope for efficiency

Add WeakMap collision check

Modify potential WeakMap collision condition

Fix this property assignment binding with private names

Add correct error message for WeakMap collision

Add error for statements before super with private identifiers

Refactor getPropertyForPrivateIdentifier

Add test for private identifier fields initialized to class exprs

Fix shebang errors

Fix private errors on index signatures

Add codefix for missing private property

Update error messages for unsupported private features

Fix inheritance-related errors

Fixes inheritance-related errors with private identifiers by resolving
properties from base classes. Private identifiers do not show up as
properties on a union type, so those do not type-check.

Add test for interface extending class with private access

Remove debugging log

Remove name assignment from private named functions

Rename to getPrivateIdentifierPropertyOfType

Fix index signature test comment

Fix test target syntax error

Change error messages

patch private identifiers outside class bodies

Add test for private identifier with ooo super

Add test for a class with a private identifier
with a non-preambly (for example, not a comment)
statement before 'super':

should error, saying 'super' must come first

Fix nits

incorporate PR feedback

Incorporate checker feedback

- reorganize if statements in checkFunctionOrConstructorSymbol
- remove overload for getPrivateIdentifierPropertyOfType

reorganize if statements in checkFunctionOrConstructorSymbol

test for private names with JSX

use getPropertyOftype in getPropertyForPrivateIdentifier

getPrivateIdentifierPropertyOfType error on synthetic

make getPrivateIdentifierPropertyOfType  error
if given a node that is not from the parse tree

Simplify checkPrivateIdentifierPropertyAccess

use getPropertiesOfType instead of
rehashing that logic

test for private identifiers w decl merging

fix test target for privateNameDeclarationMerging

update baselines

Fix private identifier ++/-- numeric coercion

Remove 'downleveled' from super error

Fix bad comments in helper call emit

Error on private identifiers in JSX tag names

Add more private identifier tests

Add es2015 target for private name destructured binding test

Add privateNameConstructorSignature test

Add test for navigation bar w private identifier

Remove spurious line from test

// in js file
class A {
    exports.#foo = 3; // should error
}

The above line failed to produce an error
when run using the test harness.

When using tsc or the language server,
we got the expected error message.

Removing the troublesome line, as it
seems to say more about the test runner
than about the code it is testing.

Fix inefficient constructor generation

dts: don't emit type for private-identified field

Do not emit types for private-identified fields
when generating declaration files.

// example.ts
export class A {
    #foo: number;
}

// example.d.ts

export declare class A {
    #foo;
}

**This is not ideal!**

The following would be better:

// example.d.ts

export declare unique class A {
    #foo;
}

See discussion:

microsoft#33038 (comment)

notice when private-identified field unused

Notice when private-identified fields are unused,
and implement the same behavior as for unused
private-modified fields.

This is used in the language server to make things
grayed out.

This case generates an error when --noUnusedLocals
flag is passed to tsc:
    - "<name> is declared but never used"

accept baselines

Revert "dts: don't emit type for private-identified field"

This reverts commit e50305d.

Instead of just excluding the type from private identifier
emit, only emit a single private identifier
per class.

This accomplishes nominality while
hiding implementation detail that
is irrelevant to .d.ts consumers

only emit a single private identifier in dts

In dts emit, emit at most one private identifier,
and rename it to `#private`.

refactor getPrivateIdentifierPropertyOfType

- safer check for wehther is parse tree node
- return undefined when not found (instead of
a Debug.fail)

Incorporate PR feedback

Don't rely on parent pointers in transform

Passes context about whether the postfix unary expression value is
discarded down the tree into the visitPostfixUnaryExpression function.

Remove orphaned test baseline files

remove unreachable if

Check `any`-typed private identified fields

Update private identifier incompatible modifier checks

- disallow 'abstract' with private identifiers
- s/private-named-property/private identifier

Add additional call expression test cases

Fix disallow 'abstract' with private identifier

Static private identifiers not inherited

Including this in the PR for private
instance fields even though static
private identifiers are banned.

Reason: this change
improves quality of error messages,
see test case.

Thanks Ron!

Simplifiy private identifier 'any' type handling

Error on private identifier declarations for ES5/ES3

Bind `this` for private identifier property tagged template literals

Fix target for jsdocPrivateName1 test

Update getPrivateIdentifierPropertyOfType API

Make it easier to use by accepting a string
and location, rather than a PrivateIdentifier.

Thanks Ron!

remove orphaned tests

rename test

remove duplicate tests

Remove unrelated error from test

update diagnostic message 'private identifier'

The nodes for hash private fields are now
called 'private identifier'. Update one last
diagnostic message to use the new terminology.

refine solution for static private identifier fields

- use `continue` instead of `filter` for perf
- better naming
- make it clear the current solution will
need to be altered when we lift the ban on
static private identifier fields, including
a test case that should change in the future

Fix display of private identifiers in lang server

Fix bug where private identifiers in completion
tooltips in the playground were showing up
as the symbol table entries (with underscores and such).

microsoft#30829 (comment)
Signed-off-by: Max Heiber <max.heiber@gmail.com>
mheiber added a commit to bloomberg/TypeScript that referenced this issue Dec 24, 2019
Implements private instance fields on top of the class properties refactor.

This commit also includes incorporation of PR feedback on the
checker, parser, and transformer in order to make the rebase
manageable.

Co-Authored-By: Max Heiber <max.heiber@gmail.com>
Co-Authored-By: Ron Buckton <ron.buckton@microsoft.com>

Signed-off-by: Joey Watts <jwatts43@bloomberg.net>
Signed-off-by: Max Heiber <max.heiber@gmail.com>

Incorporate PR feedback

Fix checker crash with new block scoped bindings

Add classExpressionInLoop test

Update baselines for private name errors

Apply suggestions from code review

Fix privateNameFieldCallExpression test

Remove unnecessary comment

Fix PropertyAccessEntityNameExpression type

Remove PrivateName from PropertyNameLiteral

Add createPrivateName

Remove PrivateName type from textSourceNode

Add Debug asserts for invalid syntax kinds

Don't output private name syntax when undeclared

Make PrivateName extend Node

Update baselines for public API

Fix completions in language server

Fix fourslash test crash

intern private name descriptions

undo expensive node.name.parent assignment

Back the way things were on `master`: only
assign node.name.parent when we need to

Add tests for private names and JSDoc

Patch hoverOverPrivateName fourslash test

Fix goToDefinition for private-named fields

remove Debug.fail for private name outside class

Remove Debug.fail in binder.ts::`getDeclarationName`.
It turns out this code path *does* get hit (intentionally).

For best error messages, return `undefined` and rely
on the parser generating a good error message

"Private names are not allowed outside class bodies"

Add rbuckton test cases for private names

These test cases specifically exercise where
we needed to use name-mangling. They are
cases where private names have the same
description.

- private names no conflict when inheriting
- private names distinct even when
the two classes have the same name

check dup instance+static private identifiers

class A {
    #foo;
    static #foo;
}

not allowed because static and private names
share the same lexical scope
https://tc39.es/proposal-class-fields/#prod-ClassBody

refactor getPropertyForPrivateName, rel spans

refactor getPropertyForPrivateName so
it is easier to read (use findAncestor instead
of loop).

Fix bugs in getPropertyForPrivateName:
- make it work with deeply-nested classes with
and without shadowing
- make it catch shadowing when the conflict is
between static and instance
private name descriptions (these can actually
clash)

And add related spans to diagnostics
for getPropertyForPrivateName

catch conflicts between static and instance
private identifiers:
- cannot have an instance and static private identifier
  with the same spelling, as there is only one namespace
  for private names

rename 'PrivateName' to 'PrivateIdentifier'

to match the change of wording in the spec
prposal:

https://tc39.es/proposal-class-fields/#sec-syntax

The rename in the spec was to avoid confusion
between the AST Node PrivateIdentifier
and the internal spec type PrivateName.

So one uses the [[Description]] of a
PrivateIdentifier to look up the PrivateName
for a given scope.

This corresponds closely to our implementation
in the binder and checker:
- we mangle PrivateIdentifier.escapedText to
get a `key` which we use to get the symbol
for a property

f

getLiteralTypeFromProperty-check privateIdentifier

rename and simplify privateNameAndAny test case

make it clearer that all this test is showing is
that we allow accessing arbitrary private identifiers
on `any`.

Note: we could have something more sound here by
checking that there is a private-named field declaration
in a class scope above the property access.

Discussion:
https://github.com/microsoft/TypeScript/pull/30829/files#r302760015

Fix typo in checker

s/identifer/identifier

remove accidental change

patch fourslash test broken by isPrivateIdentifier

just needed to add a check to see if the symbol
.name is defined

extract reportUnmatchedProperty

per @nsandersn feedback

propertiesRelatedTo was getting to long

pull out the unmatchedProperty reporting into
a seprate function

fix typo in private names test

Fix type soundness with private names

Remove PrivateIdentifier from emit with Expr hint

Fixup helpers and set function name for privates

remove accidentally-committed baselines

These baselines somehow ended up in this pr,
though they have nothing to do with the changes

Revert "getLiteralTypeFromProperty-check privateIdentifier"

This reverts commit bd1155c.

reason: the check for isIdentifier in
getLiteralTypeFromProperty is superfluous because
we do this check in getLiteralTypeFromPropertyName

Update comment in private name uniqueness test 3

I think the comments were not accurate and that we
export the error on `this.#x = child.#x` because:
- private names are lexically scoped: the code in question is not in a
lexical scope under the definition of Child's #x.
- private names are private to their containing class: never inherited

This expected behavior matches that of Chrome Canary and
my understanding of the spec

test private names use before def, circular ref

test private names use before def, circular ref

update diagnosticMessages s/delete/'delete'

per @DanielRosenwasser and @sandersn guidance,
use this style in diagnostic messages:

"operand of a 'delete' operator" (single quotes)

rather than old style:

"operand of a delete operator" (single quotes)

This is for consistency, as we use the new
style in the privateIdentifiers error messages
and it is consistent with our messages about
other operators

incorporate private names early exit feedback

and code style change to use a ternary
instead of if so we can 'const'

require private-named fields to be declared in JS

All fields must be declared in TS files.

In JS files, we typically do not have this requirement.

So special-case private fields, which must always
be declared (even in JS files, per spec)

update debug failure for es2015 private identifier

Co-Authored-By: Ron Buckton <ron.buckton@microsoft.com>

fix checker handling of private name in subclasse

update checker and tests to account for the
following ts features:

- private names do not participate in
the prototype chain, but *are* assigned
in the parent class' constructor. So
parent can access its *own* private fields
on instances of the subclass

Add more tests for private-named fields in JS

add test to hit symbolToExpression w private names

symbolToExpression knows about private names
add a test to exercise this code path

ban private-named static methods (not supported yet)

ban private-named methods (not supported yet)

ban private-named accessors (not supported yet)

fix privateIdentifier fourslash test

change assertion throw to return

Co-Authored-By: Ron Buckton <ron.buckton@microsoft.com>

Update comment in checker.ts re reserved members

Remove case for privateIdentifier in EntityNameExpr

Remove case for privateIdentifier in
EntityNameExpr. That code path is never hit,
and privateIdnetifiers cannot be entity names.

remove unnecessary parentheses

Ban private identifier in enum

As the new test, 'privateNameEnumNoEmit',
shows, the checker now correctly makes
a diagnostic for private identifiers in enums.

However, when noEmit is false we
hit this assertion in the transformer.

This assertion will have to be removed
so that we have a diagnostic here instead
of an assertion error.

When the assertion is removed,
the 'privateNameEnum' baseline
will have to be updated

Fix destructuring assignment, use createCallBinding, remove unneeded helper

Add class private field helpers to external helpers

Remove private identifier enum assertion, use missing identifiers

Fix hash map inefficiency by only using get

Update baselines with empty identifier change

Add privateNameEnum test baselines

Fix private identifier destructuring (array assignment defaults, unique names)

Use createPrivateIdentifierAssignment in destructuring transform

Fix lint error

Separate destructuring target visitor from regular visitor

Fix destructuring assignment with this bindings

Fix destructuring assignment with this bindings

Fix syntax error with destructuring assignment output

Disallow private identifiers in property signatures

remove duplicate test baselines

Add tests for undeclarated private identifiers

remove unnecessary cast

Nicer error message for mis-placed hashbang

Workaround v8 bug with destructured assignments

Optimize private identifier stack lookup

Avoid the cost of performing an array lookup to look at the top of the
private identifier environment stack.

Change function name to be more specific

Changes "getOperatorForCompoundAssignment" to
"getNonAssignmentOperatorForCompoundAssignment" now that this
function is accessible to the entire compiler.

Improve test case for private name assignment

Adds a compound assignment test case for a class with private names
being declared on the left-hand-side of the assignment expression.

Remove extra non-private-field test

Remove isPrivateIdentifierAssignmentExpression helper

Don't transform private names in ESNext target

Preserve private fields initialized to functions

Move function expressions to outer scope for efficiency

Add WeakMap collision check

Modify potential WeakMap collision condition

Fix this property assignment binding with private names

Add correct error message for WeakMap collision

Add error for statements before super with private identifiers

Refactor getPropertyForPrivateIdentifier

Add test for private identifier fields initialized to class exprs

Fix shebang errors

Fix private errors on index signatures

Add codefix for missing private property

Update error messages for unsupported private features

Fix inheritance-related errors

Fixes inheritance-related errors with private identifiers by resolving
properties from base classes. Private identifiers do not show up as
properties on a union type, so those do not type-check.

Add test for interface extending class with private access

Remove debugging log

Remove name assignment from private named functions

Rename to getPrivateIdentifierPropertyOfType

Fix index signature test comment

Fix test target syntax error

Change error messages

patch private identifiers outside class bodies

Add test for private identifier with ooo super

Add test for a class with a private identifier
with a non-preambly (for example, not a comment)
statement before 'super':

should error, saying 'super' must come first

Fix nits

incorporate PR feedback

Incorporate checker feedback

- reorganize if statements in checkFunctionOrConstructorSymbol
- remove overload for getPrivateIdentifierPropertyOfType

reorganize if statements in checkFunctionOrConstructorSymbol

test for private names with JSX

use getPropertyOftype in getPropertyForPrivateIdentifier

getPrivateIdentifierPropertyOfType error on synthetic

make getPrivateIdentifierPropertyOfType  error
if given a node that is not from the parse tree

Simplify checkPrivateIdentifierPropertyAccess

use getPropertiesOfType instead of
rehashing that logic

test for private identifiers w decl merging

fix test target for privateNameDeclarationMerging

update baselines

Fix private identifier ++/-- numeric coercion

Remove 'downleveled' from super error

Fix bad comments in helper call emit

Error on private identifiers in JSX tag names

Add more private identifier tests

Add es2015 target for private name destructured binding test

Add privateNameConstructorSignature test

Add test for navigation bar w private identifier

Remove spurious line from test

// in js file
class A {
    exports.#foo = 3; // should error
}

The above line failed to produce an error
when run using the test harness.

When using tsc or the language server,
we got the expected error message.

Removing the troublesome line, as it
seems to say more about the test runner
than about the code it is testing.

Fix inefficient constructor generation

dts: don't emit type for private-identified field

Do not emit types for private-identified fields
when generating declaration files.

// example.ts
export class A {
    #foo: number;
}

// example.d.ts

export declare class A {
    #foo;
}

**This is not ideal!**

The following would be better:

// example.d.ts

export declare unique class A {
    #foo;
}

See discussion:

microsoft#33038 (comment)

notice when private-identified field unused

Notice when private-identified fields are unused,
and implement the same behavior as for unused
private-modified fields.

This is used in the language server to make things
grayed out.

This case generates an error when --noUnusedLocals
flag is passed to tsc:
    - "<name> is declared but never used"

accept baselines

Revert "dts: don't emit type for private-identified field"

This reverts commit e50305d.

Instead of just excluding the type from private identifier
emit, only emit a single private identifier
per class.

This accomplishes nominality while
hiding implementation detail that
is irrelevant to .d.ts consumers

only emit a single private identifier in dts

In dts emit, emit at most one private identifier,
and rename it to `#private`.

refactor getPrivateIdentifierPropertyOfType

- safer check for wehther is parse tree node
- return undefined when not found (instead of
a Debug.fail)

Incorporate PR feedback

Don't rely on parent pointers in transform

Passes context about whether the postfix unary expression value is
discarded down the tree into the visitPostfixUnaryExpression function.

Remove orphaned test baseline files

remove unreachable if

Check `any`-typed private identified fields

Update private identifier incompatible modifier checks

- disallow 'abstract' with private identifiers
- s/private-named-property/private identifier

Add additional call expression test cases

Fix disallow 'abstract' with private identifier

Static private identifiers not inherited

Including this in the PR for private
instance fields even though static
private identifiers are banned.

Reason: this change
improves quality of error messages,
see test case.

Thanks Ron!

Simplifiy private identifier 'any' type handling

Error on private identifier declarations for ES5/ES3

Bind `this` for private identifier property tagged template literals

Fix target for jsdocPrivateName1 test

Update getPrivateIdentifierPropertyOfType API

Make it easier to use by accepting a string
and location, rather than a PrivateIdentifier.

Thanks Ron!

remove orphaned tests

rename test

remove duplicate tests

Remove unrelated error from test

update diagnostic message 'private identifier'

The nodes for hash private fields are now
called 'private identifier'. Update one last
diagnostic message to use the new terminology.

refine solution for static private identifier fields

- use `continue` instead of `filter` for perf
- better naming
- make it clear the current solution will
need to be altered when we lift the ban on
static private identifier fields, including
a test case that should change in the future

Fix display of private identifiers in lang server

Fix bug where private identifiers in completion
tooltips in the playground were showing up
as the symbol table entries (with underscores and such).

microsoft#30829 (comment)
Signed-off-by: Max Heiber <max.heiber@gmail.com>
DanielRosenwasser added a commit that referenced this issue Dec 27, 2019
* Fix display of private names in language server

Signed-off-by: Joseph Watts <jwatts43@bloomberg.net>
Signed-off-by: Max Heiber <max.heiber@gmail.com>

* Parse private names

Signed-off-by: Max Heiber <max.heiber@gmail.com>

* update parser error recovery tests to use ¬ not #

The intent of the tests seemed to be to
regiment the behavior of the parser
when a weird symbol is encountered.

The `#` is now taken by private identifiers,
so `¬` seems like a good new choice for
keeping the diff small, since it also fits in
16 bits (wide emojis would be treated
as multiple characters, since the scanner
uses ++).

Signed-off-by: Max Heiber <max.heiber@gmail.com>

* Private Name Support in the Checker (#5)

- [x] treat private names as unique:
    - case 1: cannot say that a variable is of a class type unless the variable points to an instance of the class
        - see [test](https://github.com/mheiber/TypeScript/tree/checker/tests/cases/conformance/classes/members/privateNames/privateNamesUnique.ts)
    - case 2: private names in class hierarchies do not conflict
        - see [test](https://github.com/mheiber/TypeScript/tree/checker/tests/cases/conformance/classes/members/privateNames/privateNamesNoConflictWhenInheriting.ts)
- [x] `#constructor` is reserved
    - see [test](https://github.com/mheiber/TypeScript/tree/checker/tests/cases/conformance/classes/members/privateNames/privateNameConstructorReserved.ts)
    - check in `bindWorker`, where every node is visited
- [x] Accessibility modifiers can't be used with private names
    - see [test](https://github.com/mheiber/TypeScript/tree/checker/tests/cases/conformance/classes/members/privateNames/privateNamesNoAccessibilityModifiers.ts)
    - implemented in `checkAccessibilityModifiers`, using `ModifierFlags.AccessibilityModifier`
- [x] `delete #foo` not allowed
- [x] Private name accesses not allowed outside of the defining class
    - see test: https://github.com/mheiber/TypeScript/tree/checker/tests/cases/conformance/classes/members/privateNames/privateNameNotAccessibleOutsideDefiningClass.ts
    - see [test](https://github.com/mheiber/TypeScript/tree/checker/tests/cases/conformance/classes/members/privateNames/privateNamesNoDelete.ts)
    - implemented in `checkDeleteExpression`
- [x] Do [the right thing](https://gist.github.com/mheiber/b6fc7adb426c2e1cdaceb5d7786fc630) for nested classes

mv private name tests together

more checker tests for private names

update naming and cleanup for check private names

for private name support in the checker:
- make names more consistent
- remove unnecessary checks
- add utility function to public API
- other small cleanup

Move getPropertyNameForUniqueESSymbol to utility

for consistency with other calculation of
special property names (starting with __),
move the calculation of property names for
unique es symbols to `utilities.ts`.

private name tests strict+es6

Update private name tests to use 'strict'
type checking and to target es6 instead of
default. Makes the js output easier to read
and tests more surface area with other
checker features.

error message for private names in obj literals

Disallow decorating private-named properties
because the spec is still in flux.

Signed-off-by: Max Heiber <max.heiber@gmail.com>

* Add private instance field transformation, pr feedback

Implements private instance fields on top of the class properties refactor.

This commit also includes incorporation of PR feedback on the
checker, parser, and transformer in order to make the rebase
manageable.

Co-Authored-By: Max Heiber <max.heiber@gmail.com>
Co-Authored-By: Ron Buckton <ron.buckton@microsoft.com>

Signed-off-by: Joey Watts <jwatts43@bloomberg.net>
Signed-off-by: Max Heiber <max.heiber@gmail.com>

Incorporate PR feedback

Fix checker crash with new block scoped bindings

Add classExpressionInLoop test

Update baselines for private name errors

Apply suggestions from code review

Fix privateNameFieldCallExpression test

Remove unnecessary comment

Fix PropertyAccessEntityNameExpression type

Remove PrivateName from PropertyNameLiteral

Add createPrivateName

Remove PrivateName type from textSourceNode

Add Debug asserts for invalid syntax kinds

Don't output private name syntax when undeclared

Make PrivateName extend Node

Update baselines for public API

Fix completions in language server

Fix fourslash test crash

intern private name descriptions

undo expensive node.name.parent assignment

Back the way things were on `master`: only
assign node.name.parent when we need to

Add tests for private names and JSDoc

Patch hoverOverPrivateName fourslash test

Fix goToDefinition for private-named fields

remove Debug.fail for private name outside class

Remove Debug.fail in binder.ts::`getDeclarationName`.
It turns out this code path *does* get hit (intentionally).

For best error messages, return `undefined` and rely
on the parser generating a good error message

"Private names are not allowed outside class bodies"

Add rbuckton test cases for private names

These test cases specifically exercise where
we needed to use name-mangling. They are
cases where private names have the same
description.

- private names no conflict when inheriting
- private names distinct even when
the two classes have the same name

check dup instance+static private identifiers

class A {
    #foo;
    static #foo;
}

not allowed because static and private names
share the same lexical scope
https://tc39.es/proposal-class-fields/#prod-ClassBody

refactor getPropertyForPrivateName, rel spans

refactor getPropertyForPrivateName so
it is easier to read (use findAncestor instead
of loop).

Fix bugs in getPropertyForPrivateName:
- make it work with deeply-nested classes with
and without shadowing
- make it catch shadowing when the conflict is
between static and instance
private name descriptions (these can actually
clash)

And add related spans to diagnostics
for getPropertyForPrivateName

catch conflicts between static and instance
private identifiers:
- cannot have an instance and static private identifier
  with the same spelling, as there is only one namespace
  for private names

rename 'PrivateName' to 'PrivateIdentifier'

to match the change of wording in the spec
prposal:

https://tc39.es/proposal-class-fields/#sec-syntax

The rename in the spec was to avoid confusion
between the AST Node PrivateIdentifier
and the internal spec type PrivateName.

So one uses the [[Description]] of a
PrivateIdentifier to look up the PrivateName
for a given scope.

This corresponds closely to our implementation
in the binder and checker:
- we mangle PrivateIdentifier.escapedText to
get a `key` which we use to get the symbol
for a property

f

getLiteralTypeFromProperty-check privateIdentifier

rename and simplify privateNameAndAny test case

make it clearer that all this test is showing is
that we allow accessing arbitrary private identifiers
on `any`.

Note: we could have something more sound here by
checking that there is a private-named field declaration
in a class scope above the property access.

Discussion:
https://github.com/microsoft/TypeScript/pull/30829/files#r302760015

Fix typo in checker

s/identifer/identifier

remove accidental change

patch fourslash test broken by isPrivateIdentifier

just needed to add a check to see if the symbol
.name is defined

extract reportUnmatchedProperty

per @nsandersn feedback

propertiesRelatedTo was getting to long

pull out the unmatchedProperty reporting into
a seprate function

fix typo in private names test

Fix type soundness with private names

Remove PrivateIdentifier from emit with Expr hint

Fixup helpers and set function name for privates

remove accidentally-committed baselines

These baselines somehow ended up in this pr,
though they have nothing to do with the changes

Revert "getLiteralTypeFromProperty-check privateIdentifier"

This reverts commit bd1155c.

reason: the check for isIdentifier in
getLiteralTypeFromProperty is superfluous because
we do this check in getLiteralTypeFromPropertyName

Update comment in private name uniqueness test 3

I think the comments were not accurate and that we
export the error on `this.#x = child.#x` because:
- private names are lexically scoped: the code in question is not in a
lexical scope under the definition of Child's #x.
- private names are private to their containing class: never inherited

This expected behavior matches that of Chrome Canary and
my understanding of the spec

test private names use before def, circular ref

test private names use before def, circular ref

update diagnosticMessages s/delete/'delete'

per @DanielRosenwasser and @sandersn guidance,
use this style in diagnostic messages:

"operand of a 'delete' operator" (single quotes)

rather than old style:

"operand of a delete operator" (single quotes)

This is for consistency, as we use the new
style in the privateIdentifiers error messages
and it is consistent with our messages about
other operators

incorporate private names early exit feedback

and code style change to use a ternary
instead of if so we can 'const'

require private-named fields to be declared in JS

All fields must be declared in TS files.

In JS files, we typically do not have this requirement.

So special-case private fields, which must always
be declared (even in JS files, per spec)

update debug failure for es2015 private identifier

Co-Authored-By: Ron Buckton <ron.buckton@microsoft.com>

fix checker handling of private name in subclasse

update checker and tests to account for the
following ts features:

- private names do not participate in
the prototype chain, but *are* assigned
in the parent class' constructor. So
parent can access its *own* private fields
on instances of the subclass

Add more tests for private-named fields in JS

add test to hit symbolToExpression w private names

symbolToExpression knows about private names
add a test to exercise this code path

ban private-named static methods (not supported yet)

ban private-named methods (not supported yet)

ban private-named accessors (not supported yet)

fix privateIdentifier fourslash test

change assertion throw to return

Co-Authored-By: Ron Buckton <ron.buckton@microsoft.com>

Update comment in checker.ts re reserved members

Remove case for privateIdentifier in EntityNameExpr

Remove case for privateIdentifier in
EntityNameExpr. That code path is never hit,
and privateIdnetifiers cannot be entity names.

remove unnecessary parentheses

Ban private identifier in enum

As the new test, 'privateNameEnumNoEmit',
shows, the checker now correctly makes
a diagnostic for private identifiers in enums.

However, when noEmit is false we
hit this assertion in the transformer.

This assertion will have to be removed
so that we have a diagnostic here instead
of an assertion error.

When the assertion is removed,
the 'privateNameEnum' baseline
will have to be updated

Fix destructuring assignment, use createCallBinding, remove unneeded helper

Add class private field helpers to external helpers

Remove private identifier enum assertion, use missing identifiers

Fix hash map inefficiency by only using get

Update baselines with empty identifier change

Add privateNameEnum test baselines

Fix private identifier destructuring (array assignment defaults, unique names)

Use createPrivateIdentifierAssignment in destructuring transform

Fix lint error

Separate destructuring target visitor from regular visitor

Fix destructuring assignment with this bindings

Fix destructuring assignment with this bindings

Fix syntax error with destructuring assignment output

Disallow private identifiers in property signatures

remove duplicate test baselines

Add tests for undeclarated private identifiers

remove unnecessary cast

Nicer error message for mis-placed hashbang

Workaround v8 bug with destructured assignments

Optimize private identifier stack lookup

Avoid the cost of performing an array lookup to look at the top of the
private identifier environment stack.

Change function name to be more specific

Changes "getOperatorForCompoundAssignment" to
"getNonAssignmentOperatorForCompoundAssignment" now that this
function is accessible to the entire compiler.

Improve test case for private name assignment

Adds a compound assignment test case for a class with private names
being declared on the left-hand-side of the assignment expression.

Remove extra non-private-field test

Remove isPrivateIdentifierAssignmentExpression helper

Don't transform private names in ESNext target

Preserve private fields initialized to functions

Move function expressions to outer scope for efficiency

Add WeakMap collision check

Modify potential WeakMap collision condition

Fix this property assignment binding with private names

Add correct error message for WeakMap collision

Add error for statements before super with private identifiers

Refactor getPropertyForPrivateIdentifier

Add test for private identifier fields initialized to class exprs

Fix shebang errors

Fix private errors on index signatures

Add codefix for missing private property

Update error messages for unsupported private features

Fix inheritance-related errors

Fixes inheritance-related errors with private identifiers by resolving
properties from base classes. Private identifiers do not show up as
properties on a union type, so those do not type-check.

Add test for interface extending class with private access

Remove debugging log

Remove name assignment from private named functions

Rename to getPrivateIdentifierPropertyOfType

Fix index signature test comment

Fix test target syntax error

Change error messages

patch private identifiers outside class bodies

Add test for private identifier with ooo super

Add test for a class with a private identifier
with a non-preambly (for example, not a comment)
statement before 'super':

should error, saying 'super' must come first

Fix nits

incorporate PR feedback

Incorporate checker feedback

- reorganize if statements in checkFunctionOrConstructorSymbol
- remove overload for getPrivateIdentifierPropertyOfType

reorganize if statements in checkFunctionOrConstructorSymbol

test for private names with JSX

use getPropertyOftype in getPropertyForPrivateIdentifier

getPrivateIdentifierPropertyOfType error on synthetic

make getPrivateIdentifierPropertyOfType  error
if given a node that is not from the parse tree

Simplify checkPrivateIdentifierPropertyAccess

use getPropertiesOfType instead of
rehashing that logic

test for private identifiers w decl merging

fix test target for privateNameDeclarationMerging

update baselines

Fix private identifier ++/-- numeric coercion

Remove 'downleveled' from super error

Fix bad comments in helper call emit

Error on private identifiers in JSX tag names

Add more private identifier tests

Add es2015 target for private name destructured binding test

Add privateNameConstructorSignature test

Add test for navigation bar w private identifier

Remove spurious line from test

// in js file
class A {
    exports.#foo = 3; // should error
}

The above line failed to produce an error
when run using the test harness.

When using tsc or the language server,
we got the expected error message.

Removing the troublesome line, as it
seems to say more about the test runner
than about the code it is testing.

Fix inefficient constructor generation

dts: don't emit type for private-identified field

Do not emit types for private-identified fields
when generating declaration files.

// example.ts
export class A {
    #foo: number;
}

// example.d.ts

export declare class A {
    #foo;
}

**This is not ideal!**

The following would be better:

// example.d.ts

export declare unique class A {
    #foo;
}

See discussion:

#33038 (comment)

notice when private-identified field unused

Notice when private-identified fields are unused,
and implement the same behavior as for unused
private-modified fields.

This is used in the language server to make things
grayed out.

This case generates an error when --noUnusedLocals
flag is passed to tsc:
    - "<name> is declared but never used"

accept baselines

Revert "dts: don't emit type for private-identified field"

This reverts commit e50305d.

Instead of just excluding the type from private identifier
emit, only emit a single private identifier
per class.

This accomplishes nominality while
hiding implementation detail that
is irrelevant to .d.ts consumers

only emit a single private identifier in dts

In dts emit, emit at most one private identifier,
and rename it to `#private`.

refactor getPrivateIdentifierPropertyOfType

- safer check for wehther is parse tree node
- return undefined when not found (instead of
a Debug.fail)

Incorporate PR feedback

Don't rely on parent pointers in transform

Passes context about whether the postfix unary expression value is
discarded down the tree into the visitPostfixUnaryExpression function.

Remove orphaned test baseline files

remove unreachable if

Check `any`-typed private identified fields

Update private identifier incompatible modifier checks

- disallow 'abstract' with private identifiers
- s/private-named-property/private identifier

Add additional call expression test cases

Fix disallow 'abstract' with private identifier

Static private identifiers not inherited

Including this in the PR for private
instance fields even though static
private identifiers are banned.

Reason: this change
improves quality of error messages,
see test case.

Thanks Ron!

Simplifiy private identifier 'any' type handling

Error on private identifier declarations for ES5/ES3

Bind `this` for private identifier property tagged template literals

Fix target for jsdocPrivateName1 test

Update getPrivateIdentifierPropertyOfType API

Make it easier to use by accepting a string
and location, rather than a PrivateIdentifier.

Thanks Ron!

remove orphaned tests

rename test

remove duplicate tests

Remove unrelated error from test

update diagnostic message 'private identifier'

The nodes for hash private fields are now
called 'private identifier'. Update one last
diagnostic message to use the new terminology.

refine solution for static private identifier fields

- use `continue` instead of `filter` for perf
- better naming
- make it clear the current solution will
need to be altered when we lift the ban on
static private identifier fields, including
a test case that should change in the future

Fix display of private identifiers in lang server

Fix bug where private identifiers in completion
tooltips in the playground were showing up
as the symbol table entries (with underscores and such).

#30829 (comment)
Signed-off-by: Max Heiber <max.heiber@gmail.com>

* fix privateIdentifier w !useDefineForClassFields

Signed-off-by: Max Heiber <max.heiber@gmail.com>

* Disallow PrivateIdentifier in Optional Chains

Signed-off-by: Max Heiber <max.heiber@gmail.com>

* restrict privateIdentifier completions correctly

Don't autocomplete privateIdentifiers in
places where they are not allowed.

Signed-off-by: Max Heiber <max.heiber@gmail.com>

* make PrivateIdentifier not a PropertyNameLiteral

Signed-off-by: Max Heiber <max.heiber@gmail.com>

* Added test.

* Accepted baselines.

* Update symbol serializer to understand private fields in JS `.d.ts` emit.

* Accepted baselines.

* fix for private field no initializer esnext

Signed-off-by: Max Heiber <max.heiber@gmail.com>

* fix private fields .d.ts emit for JS w expando

fix bug where the following in a JS file
would lead to a `#private` in the .d.ts:

```js
class C {
    constructor () {
        this.a = 3
    }
}
```

Co-authored-by: Joey Watts <joey.watts.96@gmail.com>
Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>
@sandersn sandersn added this to Bête Noire in Pall Mall Jan 30, 2020
@ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Jan 31, 2020

Will this support unique interface types as well?

Loading

@michaeljota
Copy link

@michaeljota michaeljota commented Feb 17, 2020

I like this proposal more than the other one. This is more explicit and less verbose. I understand the concerns raised here, but I think they should be considered features, and this should be used carefully by the community. I understand this is not always the case, and people could start using and misusing this for everything, but this is as for all the features that Typescript introduces. They are not always used as intended.

One particularly seems to be a good example of why this proposal is good for me.

Cross-library interop

Library A may have type Radian = unique number.
Library B may have type Radian = unique number.

And they are different. Libary A can update is definition tomorrow to type Radian = bigint, and that would be ok. I understand that if I have two nominal types, that both are numbers, seems wrong that they can't assign each other, but is that what this is about? Having someway to declare a unique type?

If I can declare a unique type that can be assigned to a different unique type because they have the same shape, are they unique then?

Loading

@AnyhowStep
Copy link
Contributor

@AnyhowStep AnyhowStep commented Feb 17, 2020

This PR leads to the Tower of Babel problem more easily, because of cross-library interop.

This comment is about regex-validated string types, but also somewhat applies here,
#6579 (comment)

Loading

@Shou
Copy link

@Shou Shou commented May 11, 2020

@AnyhowStep

But if you have N libraries with their own Radian type, you may end up needing up to N^2 functions to convert between each of the Radian types from each library.

You don't have to create N^2 converting functions, just one (in theory, but practically a bit more due to IfEquals). In Haskell, Conor McBride's newtype library uses over and the like to allow conversion between newtypes. Now, we don't have typeclasses, but we could still do something similar here:

const coerce: <A, B>(a: A) => IfEquals(UnUnique<A>, UnUnique<B>, B, never)
  = (a) => a as any

Where the IfEquals function is taken from here, and UnUnique would be some utility type alike ReturnType that grabs the underlying type. We could also use as ReturnType<typeof coerce> instead of as any but it makes no difference here. Now you can convert between any uniques that share the same underlying type:

coerce<libA.Radian, libB.Radian>(foo)

Loading

@AnyhowStep
Copy link
Contributor

@AnyhowStep AnyhowStep commented May 13, 2020

coerce<,>(), foo as libB.Radian, and the N^2 conversion functions are all unsafe, of course.

However, the N^2 explicit functions is "safer" because more effort had to go into creating them. (And one might not want all N^2 possible ways to coerce, and not create those functions).

Having a coerce<,>() function is much closer to the as operator, and is more "unsafe" because it lowers the barrier to casting types.

That's my personal opinion, anyway.


In a team project, someone might use coerce<libA.Radian, libB.Radian>(foo) and it might actually be invalid and cause run-time problems. But it might slip by the review process because coerce<,>() is used everywhere.

However, if a new function were to be written for libAToLibB(), I think it will stand out more than invoking a generic function. (Could still slip by, though)

But you're absolutely right that, technically, N^2 conversion functions are not necessary for unique type brands. It's just that you do open up the need to think about the N^2 possible conversions, and whether they each really make sense or not.

Loading

@shicks
Copy link
Contributor

@shicks shicks commented May 28, 2020

To what extent does #31894 satisfy the same requirements as this proposal? I know @DanielRosenwasser mentioned in #38510 that while nominal brands would be nice, but that it's not clear whether it's counter to the goals of 4.0 at the moment.

That said, it seems to me that one could easily enough write

placeholder type NormalizedPathBrand;
placeholder type AbsolutePathBrand;
type NormalizedPath = string & NormalizedPathBrand;
type AbsolutePath = string & AbsolutePathBrand;

where the brands are module-local and never intended to be implemented by any concrete type. Branded types already need explicit casts to instantiate them, so it's easy enough to make them. Given that that proposal seems to have more traction, does it make this one obsolete?

Loading

@isiahmeadows
Copy link

@isiahmeadows isiahmeadows commented May 28, 2020

Is there a status update on this PR?

Loading

@isiahmeadows
Copy link

@isiahmeadows isiahmeadows commented Oct 14, 2020

@weswigham Couple questions:

  1. Could you elaborate further on how unique symbol and unique (symbol) differ with respect to control flow-sensitive typing? I would've thought neither would have special behavior on that front.
  2. Do you have any status update for this? It's been over a year with pretty much radio silence.

Loading

@weswigham
Copy link
Member Author

@weswigham weswigham commented Oct 22, 2020

Could you elaborate further on how unique symbol and unique (symbol) differ with respect to control flow-sensitive typing? I would've thought neither would have special behavior on that front.

unique symbol is a single runtime value. Specifically, the result of an invocation of Symbol() at some location. Because of this, it works as a well-known property name, and acts as a literal. Each place unique symbol is written is a distinct, single symbol. unique (symbol) is "a unique subset of all symbols" and represents an abstract set of potentially multiple symbols. This behaves differently - notably, it isn't usable as well-known property name.

Do you have any status update for this? It's been over a year with pretty much radio silence.

Oh, wow, it's been that long. Uh. Last time I presented it to the team, the response was somewhat lukewarm - there's no real excitement or drive for it right now. So I guess what we're looking to see is overwhelming demand?

Loading

@kachkaev
Copy link

@kachkaev kachkaev commented Oct 22, 2020

So I guess what we're looking to see is overwhelming demand?

Not sure how the demand is properly measured, but this PR is in top 5 upvoted open PRs at the moment. Just saying 😁

Loading

@isiahmeadows
Copy link

@isiahmeadows isiahmeadows commented Nov 2, 2020

@weswigham Thanks for the quick explanation! I see why both are necessary, now. Also, I definitely recommend looking into @kachkaev's comment above.

Loading

@alex-weej
Copy link

@alex-weej alex-weej commented Jan 14, 2021

Just used branding to very quickly identify the inconsistencies in a code base that was using string for 3 different logical dynamically valued types. Consider this my +1...

Loading

@ProdigySim
Copy link

@ProdigySim ProdigySim commented Nov 30, 2021

Just a bit of data: The last codebase I was in changed from using unique symbol to string-based tag types due to their simplicity in creating compatible types across repos.

We wanted to move to a code generation system for our web API types. We had backend code that used nominal types ("tinytypes") in Scala which was the source of our branding.

Using unique-symbol based types had a couple of drawbacks here:

  1. We would need to have every nominal type exposed on the API to be predefined somewhere in TypeScript
  2. If the backend & frontend nominal type names mismatched at all, we would need to write custom mapping.
  3. One potential solution would be to have a shared library with all our "global" nominal types. But, this opens the possibility for "Dreaded Diamond" dependency patterns on these types, which would lead to incompatible nominal types (different "unique symbols" from different versions of the library)

We ended up converting all of our nominal types to string-based tag types overnight to support this project. They solved all of these problems:

  1. API code gen can just emit a generic Opaque<T, 'tag'> and not worry about references/imports
  2. If backend and frontend nomenclature mismatch, we can negotiate on the value of the 'tag' string, without any user-facing implications.
  3. Don't need a shared library in this solution. If we did opt for one, the dreaded diamond would not be an issue unless the type tags actually changed--which would probably mark an important compatibility change.

tl;dr string-based tag types seem a little more flexible with implementation details and are still good enough. They still have the hard problem of naming things, but they are fixable when there are name conflicts. We can also put out recommendations for tag patterns in shared libraries.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Design Meeting Docket
  
Awaiting triage
Linked issues

Successfully merging this pull request may close these issues.