microsoft / TypeScript Public
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
base: main
Are you sure you want to change the base?
Nominal unique type
brands
#33038
Conversation
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 |
It's useful because you can then intersect it with something. Which is all |
Yeah, I need to read more closely. I just noticed the It might be represented as an intersection under the hood, but that strikes me as unnecessarily exposing implementation details. |
|
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 |
How would you make nominal classes with this? |
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 I'd avoid it, if possible, tbh. Nominal classes sound like a pain :P |
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. |
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) { } |
This is probably obvious to everyone but 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, There are a few reasons why I'm generally against the idea of Cross-library interopLibrary A may have Both types will be considered different, even though they have the same name and declaration. If libraries start using this 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 But if you have Cross-version interopIt'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 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 Library A may have Even though they're different types, they're assignable to each other. No casting needed. 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 double Radian; And this was rejected outright because of the issues I listed above. Two libraries with their own |
Re: nominal classes - classes are already nominal if they contain any private members. Just throwing that out there. |
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 } |
@fatcerberus It's also why I avoid classes entirely and avoid private members if I do have them =P |
If cross version compatibility really is an issue then an alternate solution would be to make naming explicit: Let the type type NormalizedPathOne = string & unique "NormalizedPath"; where an unlabelled type NormalizedPathTwo = string & unique // some label that we don't care about that is auto-generated. so while two declarations of FWIW I have no real preference---for me the big win of this feature is being able reduce empty intersections more aggressively.
IMO, for all brand oblivious operations a |
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 |
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
}
} Contrast with, |
@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. |
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? |
#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. |
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. |
Only repo contributors have permission to request that @typescript-bot pack this |
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. |
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running |
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;
} |
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)
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>
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>
* 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>
Will this support |
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 interopLibrary A may have And they are different. Libary A can update is definition tomorrow to 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? |
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, |
You don't have to create
Where the
|
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 That's my personal opinion, anyway. In a team project, someone might use However, if a new function were to be written for But you're absolutely right that, technically, N^2 conversion functions are not necessary for |
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
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? |
Is there a status update on this PR? |
@weswigham Couple questions:
|
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? |
Not sure how the demand is properly measured, but this PR is in top 5 upvoted open PRs at the moment. Just saying |
@weswigham Thanks for the quick explanation! I see why both are necessary, now. Also, I definitely recommend looking into @kachkaev's comment above. |
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... |
Just a bit of data: The last codebase I was in changed from using 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:
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:
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. |
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:unique T
(whereT
is any type) is allowed in any position a type is allowed, and nominally tagsT
with a marker that makes onlyT
'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 tounique type
via intersection, which is what produces all useful relationships. (The brand can have an alias if it is directly constructed viatype MyBrand = unique unknown
)This does so much with so little - this reduces the jankiness written into types to enable nominalness with
unique symbol
s orenum
s, 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}
whereT
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 tonever
(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 exactlyunique symbol
, however if a nominal subclass of symbols is actually desired, one can writeunique (symbol)
to get a nominally brandedsymbol
type instead (orsymbol & unique unknown
- it's exactly the same thing). The way aunique 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 thatunique symbol
s 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 tounique
. We could just exposeunique
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 shorteningunique unknown
tounique
and reserving the argument form for justunique symbol
s may be preferable. All the same patterns would be possible, one would just need to writestring & unique
instead ofunique 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 thanunique 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:
One or more unique brands is missing from type A
with related information pointing at the brand location, rather than the current error involvingunique unknown
)unique unknown
brands)unique (symbol)
declaration emit (to ensure it's not rewritten asunique symbol
)keyof <unique brand type>
should benever
(as it is now), since the brand is top-ish (and contains no structure information itself), or if it should be preserved as an abstractkeyof <unique brand>
type, so that brand can apply keys-of-branded-types constraints in constraint positionsThe text was updated successfully, but these errors were encountered: